feat(sandbox): generate CLI recording shims via record_cli

[alexandrujircan]

Note

Stacked on #72 — based on feat/cli-called-criterion, and targeting it so the diff shows only this change. Retarget to main once #72 merges.

Why

#72 adds cli_called, which reads a JSON Lines invocation log — but nothing in the harness produces one. Every suite has to hand-write a recording mock and get the record shape right, which makes the log format a contract between this repo and each consumer.

That is exactly how contracts drift, and it already drifted inside a single downstream suite: of five mock templates under one skill, two were copies that never gained the JSONL sink, so tasks overlaying them silently record nothing. A file you copy can be half-copied. Config cannot.

What

Declaring a tool under sandbox.record_cli generates a recording shim:

sandbox:
  record_cli:
    - tool: uip
      exit_code: 1
      stderr: "uip: not connected to a tenant in this sandbox.\n"
    - tool: curl                   # so a disobedient agent cannot reach the network

Each shim records the invocation, writes the configured stdout/stderr, and exits with exit_code. Nothing is executed — no network, no auth, no side effects.

The sandbox writes the shims into cli_mocks/, PATH-prepends that directory through the existing mock_path_dirs machinery, and appends records to cli_mocks/calls.jsonl — which cli_called now reads by default. A task sets neither mock_path_dirs nor template_sources nor log:.

Deliberately narrow

It stubs a tool; it does not proxy one, and it serves no per-invocation responses.

An earlier revision of this PR had a mode: passthrough that recorded and then delegated to the real executable. I removed it. Recording a live tool depends on state the harness cannot guarantee — the tool being installed, PATH ordering, usually live credentials — and it was a third of the shim (find_real_tool alone was 26 of 106 lines, plus the only platform-conditional code path) serving 6 of 50 downstream tasks, all of which are already covered by an existing wrapper. It was also the sole reason a record could carry exit: null, an asymmetry that leaked into the criterion's documented contract.

Consequences of dropping it, all improvements: the shim is 68 lines, imports only json/os/sys/time, has no branch on platform, and every record carries a real exit code. Suites needing a proxy or a fixture set keep a hand-written mock under mock_path_dirs — and re-adding mode later is a non-breaking defaulted field if a second consumer asks.

Decisions worth reviewing

• A .cmd twin ships beside each shim for Windows PATHEXT lookup.
• The log is seeded empty. A correct run that legitimately calls nothing must satisfy max_count: 0, while a missing log (mock never ran, or wrote elsewhere) must still fail. feat(criteria): add cli_called for structured invocation matching #72's checker treats those differently, so they have to stay distinguishable.
• stdin is never read. Reading it would block whenever the sandbox leaves stdin attached to an open pipe, hanging the task until its timeout.
• Name collisions raise. If a mock_path_dirs entry already provides an executable of the same name, setup fails loudly instead of letting directory order decide which one runs.
• The rendered shim imports nothing from coder_eval and is pure ASCII — it runs inside a sandbox where this package is not installed, under whatever python3 is on PATH there.

Testing

21 new tests in tests/test_sandbox_record_cli.py. The load-bearing one is the round trip — generate a shim, actually execute it, then grade the log it wrote with cli_called and no log: configured. That contract is only testable because writer and reader now ship together.

Also covered: .cmd twin generation, PATH ordering, collision rejection, seeded-empty log, quoted arguments with spaces and multi-line heredoc payloads surviving as single argv elements, several tools sharing one log tagged by tool, append ordering, negative guards on both an empty and a populated log, model validation including path-traversal rejection on tool, and assertions that the rendered shim is pure ASCII, imports nothing from this package, and contains no subprocess/exec/popen/system call.

• make lint — 166 passed; ruff check clean; pyright back to its 3 pre-existing openai_codex import errors.
• Full suite: 3625 passed, 5 failed — the same 5 that fail on main at cc2cfc7 (3× test_reports_stats_nonfinite, 2× test_sandbox symlink tests needing Windows privileges). One run reported a 6th failure in one of those already privilege-flaky symlink tests; it did not reproduce across two further runs.

Run on Windows/Python 3.13.9, so Linux CI is the real check.

Downstream effect

UiPath/skills maintains five recorders across its IXP suite. This replaces two of them — the offline uip and curl mocks in its base template — covering 42 of its 50 IXP tasks, which also stop repeating log: mocks/calls.jsonl on every criterion.

The other three stay hand-written, and each for a reason this PR scopes out deliberately:

• One records then delegates to the live CLI (6 integration/e2e tasks) — the passthrough case removed above.
• Two are response-serving dispatchers (case "$1 $2 $3" returning canned JSON per verb) that overlay the base template for 2 tasks whose correct path starts with a read.

That is the intended split rather than a shortfall: the tasks needing a fixture set keep a mock, and the collision check means a task cannot half-adopt record_cli while still shipping its own uip — it fails loudly at setup instead.

🤖 Generated with Claude Code

[github-actions]

Claude finished @alexandrujircan's task in 1m 18s —— View job

---

Code Review in Progress

Todo List:

• Read review guidelines (.github/code_review.md)
• Check project conventions (CLAUDE.md)
• Get full diff from base branch
• Read all changed files completely
• Perform cross-file consistency checks
• Analyze design decisions
• Post comprehensive review

Starting review...

[uipreliga]

Review: coder_eval — pr:73

Scope: pr:73 · branch feat/sandbox-record-cli · ce8e5ea · 2026-08-03T17:33Z · workflow variant

Change class: complex — generates executable shim scripts into the sandbox, mutates PATH resolution order, adds a new SandboxConfig list field with a validator, and changes an existing criterion field from required to defaulted; correctness requires reasoning about path containment, collision handling, and cross-platform execution

Overall a strong, well-factored PR (9/10) — clean layering, a 310-line dedicated test module, and no critical or security-breaking defects — but the new record_cli recorder concentrates its risk exactly where an eval harness can least afford it: a log preserved across runs, a guard that raises ValueError instead of its intended RuntimeError on macOS (2 tests red at HEAD), a shape-only tool validator that permits PATH poisoning/interpreter recursion and uncompilable shim source, and a traceless pass on log-write failure — four paths that can change a task's score or final_status for byte-identical agent output, all fixable with small local edits before merge.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9.2 / 10	0	0	1	3	record_cli collision guard has coverage gaps — extensionless-name-only match (misses .cmd twins) and no check against template content already in cli_mocks/
2. Type Safety	8.4 / 10	0	1	1	1	Unvalidated tool name is interpolated raw into the generated shim's module docstring (and .cmd twin), allowing broken or injected shim source
3. Test Health	8.5 / 10	0	0	3	0	record_cli's declared merge strategy is the only SandboxConfig merge field missing from the test_merge_strategy_annotations parametrize list
4. Security	9.4 / 10	0	0	1	1	record_cli has no reserved-name guard: tool: python3 makes the generated shim's own #!/usr/bin/env python3 re-resolve to itself, an infinite exec loop that hangs the task; tool: git/uv/curl silently shadow those binaries for run_command criteria too
5. Architecture & Design	9.4 / 10	0	0	1	1	parse_log is a test-only, divergent duplicate of the JSON-Lines reader in criteria/cli_called.py
6. Error Handling & Resilience	8.4 / 10	0	1	1	1	Seeded recorder log is preserved rather than truncated, so a prior run's invocations score the current run (DIRECT_WRITE / reused --run-dir)
7. API Surface & Maintainability	9.9 / 10	0	0	0	1	The task guide never states that exit_code defaults to 1, so - tool: curl silently makes the shadowed tool fail
8. Evaluation Harness Quality	9 / 10	0	1	0	0	Collision guard compares an UNRESOLVED sandbox root via relative_to(), raising pathlib ValueError instead of the intended RuntimeError — two tests red on macOS

Overall Score: 9 / 10 · Weakest Axis: Type Safety at 8.4 / 10
Totals: 🔴 0 · 🟠 3 · 🟡 8 · 🔵 8 across 8 axes.

Blockers

1. [Axis 2] Unvalidated tool name is interpolated raw into the generated shim's module docstring (and .cmd twin), allowing broken or injected shim source (src/coder_eval/models/sandbox.py:364) — The only guard on tool is if "/" in v or "\\" in v or v in {".", ".."}: (models/sandbox.py:364), whose docstring justifies it purely as a path check ("The shim is written as <RECORD_CLI_DIR>/<tool>"). But the same value is ALSO interpolated unquoted into generated Python source at cli_recorder.py:23 — """Recording shim for {tool} - generated by coder_eval SandboxConfig.record_cli. — and into a Windows batch line at sandbox.py:524 (f'python "%~dp0{spec.tool}" %*'). Only the TOOL = {tool!r} binding is repr-escaped. Verified empirically against PR HEAD: RecordedCli(tool='a"""b') validates, and compile(render_recorder(spec)) raises SyntaxError: unterminated string literal (detected at line 14); a name such as x") or __import__("os").system(...) or (" (no /, so it passes the validator) is embedded as executable code. The model's own tests only probe path-shaped names (tests/test_sandbox_record_cli.py:253 — ["../evil", "a/b", "a\\b", ".", "..", "", " uip"]), so the quote case is untested. Fix: constrain the field at the schema level to what an executable name can actually be — tool: str = Field(pattern=r"^[A-Za-z0-9._+-]+$", ...) (or the equivalent check in validate_tool_name) — and additionally escape the docstring interpolation ({tool!r} or strip it from the docstring) so render_recorder cannot emit invalid source. Cross-axis: also a security finding (task-YAML-to-code injection) and an axis-1/8 finding (silent harness break scored as agent failure). A CEnnn lint rule forbidding a bare {name} (non-!r) placeholder in a code-generating template is mechanically detectable.
2. [Axis 6] Seeded recorder log is preserved rather than truncated, so a prior run's invocations score the current run (DIRECT_WRITE / reused --run-dir) (src/coder_eval/sandbox.py:511) — The seeding guard is conditional:

log_path = self.sandbox_dir / RECORD_CLI_LOG
if not log_path.exists():          # <-- sandbox.py:511
    log_path.write_text("", encoding="utf-8")

At setup time nothing in this run has written the log yet, so the guard's ONLY effect is to preserve a log left by a previous run — and the shim appends, so records accumulate. PreservationMode.DIRECT_WRITE (the default for driver: docker, orchestration/config.py:26) runs the sandbox directly in run_dir/artifacts/<task_id>, which the orchestrator deliberately does not clear (orchestrator.py:1039-1053: "DIRECT_WRITE deliberately does NOT clear the target dir, so a reused --run-dir (or --resume) can leave a prior run's files ... and silently perturb file-based criteria").

Reproduced at PR HEAD: pre-seed <target>/cli_mocks/calls.jsonl with one uip ixp projects delete proj-1 record, then Sandbox(...).setup(target_dir=target) -> the stale line survives verbatim, and CliCalledCriterion(verb='ixp projects delete', min_count=1) scores 1.0 with zero agent activity in this run. Same agent output, different score depending on whether the run dir was reused — the scoring-correctness class.

Fix: seed unconditionally (log_path.write_text("", encoding="utf-8")), and ideally clear the whole cli_mocks/ directory before regenerating so stale shims for tools no longer in record_cli don't stay on PATH. Add a test that pre-populates the log and asserts it is empty after setup(target_dir=...).
3. [Axis 8] Collision guard compares an UNRESOLVED sandbox root via relative_to(), raising pathlib ValueError instead of the intended RuntimeError — two tests red on macOS (src/coder_eval/sandbox.py:497) — The guard builds its message with

                        f"'{rel}' already provides one ({clash.relative_to(self.sandbox_dir)}). "

but clash derives from user_dir = self._resolve_within_sandbox(rel, ...), which returns (self.sandbox_dir / rel).resolve() (sandbox.py:305) — a symlink-resolved path — while self.sandbox_dir is stored unresolved (Path(tempfile.mkdtemp(...)) or the caller's target_dir). When the sandbox root traverses a symlink, relative_to raises before the raise RuntimeError(msg) on line 501 ever runs. Verified at PR HEAD: uv run pytest tests/test_sandbox_record_cli.py gives ValueError: '/private/var/.../mocks/uip' is not in the subpath of '/var/.../' for TestGeneration::test_collision_with_user_mock_raises, and the routed coverage report shows line 501 uncovered — the documented RuntimeError is never actually raised anywhere in the suite. macOS is the everyday case (/var → /private/var); on Linux any --run-dir under a symlinked mount (e.g. /data → /mnt/data) under DIRECT_WRITE hits it too. Fix: compute the root once as root = self.sandbox_dir.resolve() and use clash.relative_to(root) (or clash.name), and keep the test asserting pytest.raises(RuntimeError, match="already provides one") so line 501 is genuinely covered.

Non-blocking, but please consider before merge

1. [Axis 1] record_cli collision guard has coverage gaps — extensionless-name-only match (misses .cmd twins) and no check against template content already in cli_mocks/ (src/coder_eval/sandbox.py:493) — src/coder_eval/sandbox.py:455-458 justifies the new PATH ordering with an absolute claim:

   Generated recorders go FIRST: _generate_cli_recorders refuses to

   generate a shim whose name a user mock dir already provides, so this

   order can never silently shadow a task's own mock

But the guard at line 493 is clash = user_dir / spec.tool / if clash.exists(): — it never checks f"{spec.tool}.cmd", even though line 524 writes (recorder_dir / f"{spec.tool}.cmd") into the directory that is now prepended ahead of the user's. Failure scenario: on a Windows host, a task with mock_path_dirs: ["mocks"] containing mocks/uip.cmd plus record_cli: [{tool: uip}] passes setup with no error; PATHEXT resolves uip to cli_mocks/uip.cmd (first on PATH) instead of the task's own mock, silently changing what the agent runs and what gets graded — exactly the outcome the comment promises is impossible, on the only platform the .cmd twin exists for. Extend the guard to the whole generated name set (spec.tool, plus f"{spec.tool}.cmd", and ideally the other PATHEXT extensions .bat/.exe), or drop the absolute wording from the comment and document the residual case.
2. [Axis 2] RecordedCli.exit_code has no POSIX range constraint — exit_code: 256 makes the shim exit 0 while the log records 256 (src/coder_eval/models/sandbox.py:338) — exit_code: int = Field(default=1, ...) (models/sandbox.py:338-344) accepts any integer, but the rendered shim ends in sys.exit(main(sys.argv)) (cli_recorder.py:88) so the value is truncated mod 256 by the OS. Verified at PR HEAD: RecordedCli(tool='uip', exit_code=256) validates, and running the rendered shim gives actual exit status for exit_code=256: $?=0 while the emitted record is {"ts": ..., "tool": "uip", "argv": ["--foo", "bar"], "exit": 256}. So a task author who configures a failing tool silently gets a succeeding one — the agent observes exit 0, changes behaviour, and the score changes; the log's exit field also stops describing the process's real status. exit_code=-1 is likewise accepted (→ real status 255). This model's neighbours already constrain their numeric fields (ResourceLimits.max_cpus/max_pids use gt=0, models/sandbox.py:36/:42), so add ge=0, le=255.
3. [Axis 3] record_cli's declared merge strategy is the only SandboxConfig merge field missing from the test_merge_strategy_annotations parametrize list (src/coder_eval/models/sandbox.py:422) — grep -rln "record_cli\|RecordedCli" tests/ tasks/ experiments/ docs/ returns only tests/test_sandbox_record_cli.py and docs/TASK_DEFINITION_GUIDE.md — no task YAML, no experiment YAML, no resolver test. The field at models/sandbox.py:422 is record_cli: list[RecordedCli] | None = MergeField(strategy="replace", ...) and its own description asserts "Replaced (not merged) across config layers, like mock_path_dirs", yet: (1) tests/test_merge_strategy_annotations.py carries a HARDCODED parametrize list that includes (SandboxConfig, "mock_path_dirs", "replace") at line 34 but has no record_cli row, so the annotation is unguarded; (2) there is no test_experiment_resolver.py case exercising the 5 layers (default → exp defaults → task → variant → CLI) for it; (3) there is no -D sandbox.record_cli=... override test; (4) no test parses a task YAML containing sandbox: record_cli: into a TaskDefinition, so the documented YAML shape in docs/TASK_DEFINITION_GUIDE.md is only prose. This is the shared-rubric Review Criterion 10 gap. Add the (SandboxConfig, "record_cli", "replace") row plus one resolver test asserting a variant-level record_cli replaces (does not append to) a task-level one.
4. [Axis 3] No test executes a shim by bare name through PATH, so the #!/usr/bin/env python3 shebang and the .cmd twin's body are unprotected (the chmod and PATH-order claims do not hold) (tests/test_sandbox_record_cli.py:37) — The only invoker is the helper at tests/test_sandbox_record_cli.py:35-43:
return subprocess.run([sys.executable, str(shim), *args], ...)
That runs the script through an explicit interpreter with an absolute path, so three mechanisms the agent actually depends on are never exercised: the #!/usr/bin/env python3 shebang, the +x bit applied at src/coder_eval/sandbox.py:517 (shim.chmod(shim.stat().st_mode | 0o111)), and PATH resolution of a bare uip against the prepended cli_mocks/ directory. Drop the chmod, or the shebang, or break the PATH prepend, and all 29 tests still pass. Add one POSIX test that runs subprocess.run(["uip", "projects", "list"], env={**os.environ, "PATH": f"{recorder_dir}{os.pathsep}{os.environ['PATH']}"}) and then grades the log. Relatedly, the .cmd twin is asserted only to exist — tests/test_sandbox_record_cli.py:55 is assert (recorder_dir / "uip.cmd").is_file() — its contents (python "%~dp0uip" %*, CRLF line endings) are never asserted or run, so a malformed batch line ships silently.
5. [Axis 3] validate_tool_name reject-branch tests miss the names that clobber the feature's own generated artifacts (tests/test_sandbox_record_cli.py:253) — The parametrize at tests/test_sandbox_record_cli.py:253 is ["../evil", "a/b", "a\\b", ".", "..", "", " uip"] — it covers every branch of validate_tool_name (src/coder_eval/models/sandbox.py:362-365) but no name that collides with an artifact the generator itself writes into cli_mocks/. Both cases are real and reproduce today:

1. RecordedCli(tool="calls.jsonl") — validates fine, then sandbox.py:514-516 writes the shim source over the seeded log. Verified: after Sandbox(...).setup(), cli_mocks/calls.jsonl is 2261 bytes beginning #!/usr/bin/env python3 / """Recording shim for calls.jsonl .... The log every cli_called criterion reads by default is destroyed with no error.
2. record_cli: [RecordedCli(tool="uip.cmd"), RecordedCli(tool="uip")] — verified cli_mocks/ ends up as ['calls.jsonl', 'uip', 'uip.cmd', 'uip.cmd.cmd'] with uip.cmd containing @echo off\nREM Generated by coder_eval Sa..., i.e. the uip.cmd shim was silently overwritten by the uip batch twin. Order-dependent clobber.

Add "calls.jsonl" (and a <tool>.cmd-vs-<tool> pair) to the reject cases and extend validate_tool_name to reject LOG_FILENAME and .cmd-suffixed names, or make the generator refuse to overwrite an existing file in cli_mocks/.
6. [Axis 4] record_cli has no reserved-name guard: tool: python3 makes the generated shim's own #!/usr/bin/env python3 re-resolve to itself, an infinite exec loop that hangs the task; tool: git/uv/curl silently shadow those binaries for run_command criteria too (src/coder_eval/models/sandbox.py:364) — validate_tool_name (models/sandbox.py:362-366) checks only shape — if "/" in v or "\\" in v or v in {".", ".."}: raise ... — never identity. Nothing rejects python, python3, env, sh, git, uv, node, and the generated dir is prepended AHEAD of everything (sandbox.py:459-462, resolved.append(generated) before the mock_path_dirs loop).

The shim's own interpreter is resolved through that same poisoned PATH: cli_recorder.py:22 is #!/usr/bin/env python3, and the Windows twin at sandbox.py:523 is f'python "%~dp0{spec.tool}" %*' — a bare python, looked up in a directory the harness just put first on PATH.

VERIFIED at PR HEAD: rendered render_recorder(RecordedCli(tool="python3", exit_code=7)) to ./python3, chmod +x, then PATH="$PWD:$PATH" timeout 5 python3 -c "print('hi')" → exit 124 (killed by timeout). env re-resolves python3 to the shim, which re-execs env, forever; the command never returns. tool: python on Windows gives the same recursion through the .cmd twin. Under driver: tempdir none of ResourceLimits.max_pids/max_cpus is enforced (ResourceLimits docstring, models/sandbox.py:20-22), so this spins until the task timeout.

Second-order integrity impact worth stating: the recorder dir does not stay confined to the agent. orchestrator.py:1084 passes resolved_mock_path_dirs as env_path_prepend to agent.start(...), and _sync_sandbox_command_path_with_agent later re-uses the agent's SDK PATH as Sandbox._command_base_path, which _build_run_command_env prepends (sandbox.py:909). So a record_cli entry for git/python/uv also shadows those binaries for every run_command criterion, silently changing the score.

Fix: reject a reserved set in validate_tool_name (at minimum python, python3, env, sh, bash, node, git, uv, cmd), and make the shim independent of PATH — emit #!<sys.executable> (or os.path.realpath(sys.executable)) instead of #!/usr/bin/env python3, and use that absolute interpreter in the .cmd line rather than a bare python. Add a test asserting RecordedCli(tool="python3") raises. CVSS:3.1/AV:L/AC:L/PR:H/UI:N/S:U/C:N/I:N/A:H
7. [Axis 5] parse_log is a test-only, divergent duplicate of the JSON-Lines reader in criteria/cli_called.py (src/coder_eval/cli_recorder.py:104) — cli_recorder.py:104 def parse_log(text: str) -> list[dict[str, object]]: is shipped in src/coder_eval/ but called from nowhere in src/ — grep -rn "parse_log" src/ at PR HEAD returns exactly one hit, its own definition; the only callers are tests/test_sandbox_record_cli.py (lines 117, 149, 161, 173, 187, 310). Its docstring (lines 107-108) asserts it is "Shared with tests and any caller that wants the log without duplicating the JSON-Lines handling in :mod:coder_eval.criteria.cli_called", but criteria/cli_called.py was NOT changed to use it and still carries its own copy at lines 209-223 (records: list[dict[str, Any]] = [] / malformed = 0 / for line in content.splitlines(): ...). The two copies have already drifted at birth: the criterion counts non-JSON lines AND non-dict JSON into malformed and surfaces that in details ("Skipped N unparseable log line(s)", line 258), whereas parse_log silently continues on both. So the module that claims to be the single source of truth for the log format is the one no production code reads — a reader can safely change parse_log believing they changed grading behaviour. Either make CliCalledChecker._check_impl consume parse_log (moving the malformed count into it, e.g. returning tuple[list[dict[str, Any]], int]) so there is one parser, or delete parse_log from the shipped package, move the helper into tests/, and drop the misleading docstring sentence.
8. [Axis 6] Recorder shim drops log records on OSError with a bare pass (cli_recorder.py:68-69), turning an unwritable log into an "agent never ran the command" score (src/coder_eval/cli_recorder.py:68) — In the rendered shim:

        with open(LOG_PATH, "a", encoding="utf-8", newline="\n") as handle:
            handle.write(json.dumps(entry) + "\n")
    except OSError:
        pass          # <-- cli_recorder.py:68-69

The narrow OSError scope is right (I verified the ensure_ascii claim holds: json.dumps({'a': '\udcff'}) -> '{"a": "\\udcff"}', so a surrogate from undecodable argv cannot raise, and the ASCII-only payload cannot hit UnicodeEncodeError on the utf-8 write). The problem is the pass: this log is the sole evidence the cli_called criterion scores on, and a dropped record is invisible to everyone.

Reproduced at PR HEAD: chmod 444 the seeded log, run the shim once -> the shim exits 0 with empty stderr, and SuccessChecker.check(CliCalledCriterion(verb='ixp projects configure-model', min_count=1)) returns score=0.0, error=None, details="0 invocation(s) matched ...; 0 invocation(s) recorded in 'cli_mocks/calls.jsonl'" — byte-identical to the agent never having run the command. The criterion is already careful to distinguish a missing log (harness fault, error= set) from an empty one; an unwritable log falls back into the "agent did nothing" bucket instead.

Fix: keep the command working (correctly best-effort), but leave a trace — on OSError, attempt one open(LOG_PATH + ".error", "a") sentinel (wrapped in its own try/except), and/or sys.stderr.write("coder_eval recorder: log write failed: %r\n" % exc). Then have cli_called surface the sentinel as error= rather than score 0. Also bind the exception (except OSError as exc) so the message is available at all.

Nits

1. [Axis 1] Recorder log filename calls.jsonl is declared twice (LOG_FILENAME vs RECORD_CLI_LOG) with nothing tying writer to reader (src/coder_eval/cli_recorder.py:19) — cli_recorder.py:19 declares LOG_FILENAME = "calls.jsonl" (the path the generated shim writes: LOG_PATH = os.path.join(SHIM_DIR, {log_filename!r})), while src/coder_eval/models/sandbox.py:314-315 independently declares RECORD_CLI_DIR = "cli_mocks" / RECORD_CLI_LOG = f"{RECORD_CLI_DIR}/calls.jsonl" (the path the sandbox seeds at sandbox.py:509 and the path CliCalledCriterion.log now defaults to). The two literals must agree for the feature to work at all, but no code or assertion connects them — cli_recorder.py already imports from coder_eval.models, so it can derive the name instead: LOG_FILENAME = PurePosixPath(RECORD_CLI_LOG).name. Filed Low rather than Medium because the round-trip test test_cli_called_grades_the_generated_log_with_no_log_path_configured (tests/test_sandbox_record_cli.py:190) would fail on drift; the duplication is still a second source of truth a reader has to notice.

2. [Axis 1] resolved_mock_path_dirs docstring no longer matches the ordering it returns (omits the prepended recorder dir) (src/coder_eval/sandbox.py:441) — The property's docstring (sandbox.py:440-444) was left untouched by the change: "Absolute paths of configured mock dirs that exist on disk. / Returned in the order they appear in SandboxConfig.mock_path_dirs". As of this PR the first element can be cli_mocks/, which appears in record_cli, not in mock_path_dirs (lines 459-462), and the same drift hits _prepare_mock_path_dirs one method up: its docstring at line 422 still says "Apply +x to plain files in each mock_path_dirs entry" while it now also chmods +x every file in the generated recorder dir — including the calls.jsonl log. Update both summaries to mention the generated recorder directory and that it is returned first; the explanatory rationale is already in the body comment at 455-458 but is invisible to anyone reading only the docstring.

3. [Axis 1] shim.chmod(...) duplicates the _prepare_mock_path_dirs +x pass that runs immediately afterwards (src/coder_eval/sandbox.py:517) — sandbox.py:209-214 already sequences the two steps and says so:

   Generate recording shims for record_cli tools (before the +x pass

   below, which also covers them)

   self._generate_cli_recorders()
   self._prepare_mock_path_dirs()

_prepare_mock_path_dirs (line 433-436) iterates self.resolved_mock_path_dirs, which now includes the recorder dir, and ORs in 0o111 for every plain file under it — so shim.chmod(shim.stat().st_mode | 0o111) at line 517 is dead work. Either drop line 517 and rely on the documented +x pass, or keep the chmod and drop the "which also covers them" clause so only one mechanism is described as owning the bit.
4. [Axis 2] record_cli: list[RecordedCli] | None accepts duplicate tool entries; the later shim silently overwrites the earlier one, so the effective exit_code/stdout depends on list order (src/coder_eval/models/sandbox.py:422) — record_cli: list[RecordedCli] | None = MergeField(strategy="replace", ...) (models/sandbox.py:422-423) has no uniqueness constraint on tool, while sandbox.py:514-516 writes every entry to the same name (shim = recorder_dir / spec.tool; shim.write_text(render_recorder(spec), ...)). Verified at PR HEAD: SandboxConfig(record_cli=[RecordedCli(tool='uip', exit_code=0), RecordedCli(tool='uip', exit_code=7)]) validates and yields [('uip', 0), ('uip', 7)], so the agent silently gets the last entry's exit code. Note the PR is deliberately strict about the other collision — _generate_cli_recorders raises RuntimeError when a mock_path_dirs entry provides the same name (sandbox.py:493-501) — so leaving the intra-list collision silent is inconsistent with the guard it just added. Add a @model_validator(mode="after") on SandboxConfig rejecting duplicate record_cli[*].tool values.
5. [Axis 4] recorder_dir / spec.tool is never containment-checked, so a Windows drive-relative tool name writes the shim outside cli_mocks/ (src/coder_eval/sandbox.py:515) — Every other task-author-supplied path in this module goes through _resolve_within_sandbox (mock_path_dirs at sandbox.py:464, starter_files at 554, mount_point at 346), but the tool name does not: shim = recorder_dir / spec.tool (sandbox.py:515) and (recorder_dir / f"{spec.tool}.cmd") (sandbox.py:525) join an unvalidated string straight onto the resolved dir.

On POSIX the "/" in v or "\\" in v check at models/sandbox.py:364 holds the line. On Windows — which this PR explicitly targets, per sandbox.py:475-476 "A .cmd twin is written beside it so a bare uip also resolves through Windows PATHEXT lookup" — pathlib's drive-relative semantics defeat it. VERIFIED at PR HEAD: RecordedCli(tool="D:evil") is ACCEPTED, and PureWindowsPath(r"C:\sandbox\cli_mocks") / "D:evil" → D:evil, i.e. the sandbox prefix is discarded and an executable file is written to the current directory of drive D. (Same-drive C:evil is harmless: it yields C:\sandbox\cli_mocks\evil.) On Windows the sandbox is rooted under Path.home() (sandbox.py:52-54), so a second drive letter escapes containment. Also accepted and worth closing at the same seam: a\x00b (validator passes; write_text then dies with ValueError: embedded null byte mid-setup()), -rf, ~, ....

Fix: replace the shape checks with an allowlist in validate_tool_name — re.fullmatch(r'[A-Za-z0-9._+-]{1,64}', v) rejects drive-relative, NUL, control, and leading-- names in one rule — and, defensively, route the shim path through self._resolve_within_sandbox(f"{RECORD_CLI_DIR}/{spec.tool}", field="record_cli tool") so it obeys the same containment invariant as every neighbouring path. CVSS:3.1/AV:L/AC:H/PR:H/UI:N/S:U/C:N/I:L/A:N
6. [Axis 5] Feature split leaves all recorder filesystem behaviour inside the 1145-line Sandbox class while the new top-level module owns only a string (src/coder_eval/sandbox.py:469) — class Sandbox now spans lines 100-1244 (1145 lines, 36 methods; file grew 1162 → 1244) and already mixes tempdir lifecycle, git/template application, venv + npm provisioning, plugin-tools management, command execution, file IO, preservation and cleanup. This PR adds a seventh concern — generating executable shim source — entirely inside it: sandbox.py:469 def _generate_cli_recorders(self) -> None: owns collision detection, recorder_dir.mkdir (504), log seeding (510-511), shim.write_text + chmod (515-517), and the Windows .cmd twin (518-529), while the new module cli_recorder.py contributes only _TEMPLATE and a 3-line render_recorder. The natural split is the inverse: give cli_recorder.py a generate(recorder_dir: Path, specs: list[RecordedCli]) -> None that owns the on-disk representation (shim + .cmd twin + seeded log, all of which are its format), leaving Sandbox to resolve the path and call it — the same shape _apply_starter_files_source / _apply_repo_source already use for delegated setup steps. While doing so, consider a name that does not read as "records the coder-eval CLI" and does not sit as a bare top-level module immediately adjacent to the unrelated coder_eval/cli/ Typer package.
7. [Axis 6] Deterministic record_cli misconfiguration is categorized as a retryable SANDBOX_SETUP_ERROR and re-attempted three times (src/coder_eval/sandbox.py:501) — raise RuntimeError(msg) (sandbox.py:501) propagates out of setup(), which the orchestrator wraps in execute_with_retry(..., context={"component": "sandbox"}) (orchestrator.py:1059-1064). Verified: categorize_error(RuntimeError(<the collision message>), {"component": "sandbox"}) -> ErrorCategory.SANDBOX_SETUP_ERROR, whose RetryConfig(max_retries=2, initial_delay=10.0, backoff_multiplier=1.5) yields delays of ~10.3s and ~17.0s — so an unfixable YAML conflict burns 3 attempts and ~27s per task, and logs the confusing message three times. (The masked ValueError from finding #1 categorizes identically.) No resource leak: _setup_tempdir's except Exception at sandbox.py:235-244 rmtree's the self-created tempdir before re-raising, and skips it for a caller-supplied target_dir by design.

Fix: raise a non-retryable typed error for task-config faults (e.g. an AgentConfigError-style sibling routed to a max_retries=0 category), or add a record_cli/mock_path_dirs pattern to the sandbox arm of _categorize_by_component.
8. [Axis 7] The task guide never states that exit_code defaults to 1, so - tool: curl silently makes the shadowed tool fail (docs/TASK_DEFINITION_GUIDE.md:447) — Line 447 says only "Each shim records the invocation, writes the configured stdout/stderr, and exits with exit_code", and the example at line 443 uses - tool: curl with no exit_code. The default is failure (exit_code: int = Field(default=1, ...), models/sandbox.py:338-343) — a non-obvious, behaviour-changing default that a task author reading only the guide will not learn (a shimmed git/uv would start failing every call). Add to line 447: "exit_code defaults to 1 — an unconfigured tool looks like a failing one; set exit_code: 0 if the agent should see success." While there, describe tool in prose too; it currently appears only inside the YAML example.

What's Missing

Parallel paths:

• 🟠 Sandbox._refresh_plugin_tools_dir / uip_search_path was not updated for the generated recorder dir: once the orchestrator syncs the agent's PATH back (orchestrator.py:1084 → set_command_base_path → _refresh_plugin_tools_dir, sandbox.py:737), shutil.which("uip", path=uip_search_path) resolves to cli_mocks/uip for the PR's own headline example (record_cli: [{tool: uip}]), which is not inside a node_modules/@uipath tree, so resolve_uipath_plugin_dir returns None and PLUGIN_TOOLS_DIR silently stops being exported to every run_command criterion (the MST-9795 pin). No code, comment, doc or test covers this interaction. (trigger: src/coder_eval/sandbox.py) (restates: Axis 4: record_cli has no reserved-name guard / recorder dir shadows harness binaries on the shared PATH)
• 🟡 The agent_judge sandbox copy was not updated: evaluation/sub_agent.py copytrees the whole sandbox into the judge dir filtered only by ignore_patterns, and cli_mocks was not added to resources/default_ignore_patterns.yaml (nor to the ignore_patterns floor in models/criteria.py), so a Bash-enabled judge now sees harness-generated shim source plus calls.jsonl sitting in the workspace as if the agent had authored them. (trigger: src/coder_eval/sandbox.py)
• 🟡 The reader side of the new contract was not migrated: criteria/cli_called.py keeps its own inline JSON-Lines loop instead of calling the new cli_recorder.parse_log, so the writer's module ships a parser no production path uses and the two already differ on malformed-line accounting. (trigger: src/coder_eval/cli_recorder.py) (restates: Axis 5: parse_log is a test-only, divergent duplicate of the reader in criteria/cli_called.py)

Tests:

• 🟡 The shim's explicitly-designed stdin invariant is unasserted: the docstring and commit message state "stdin is deliberately never read: it would block whenever the sandbox leaves it on an open pipe, hanging the task", but no test in tests/test_sandbox_record_cli.py runs the shim with stdin attached to an open pipe (stdin=subprocess.PIPE, never closed) and asserts it returns — the exact hang the design is protecting against. (trigger: src/coder_eval/cli_recorder.py)
• 🟡 No test exercises setup(target_dir=...) (DIRECT_WRITE) or a second setup() over an already-populated cli_mocks/, so neither the log-seeding branch (if not log_path.exists()) nor the "regenerated on every sandbox setup" claim in the shim docstring is pinned — regeneration over an existing shim and a pre-existing log are both untested. (trigger: src/coder_eval/sandbox.py) (restates: Axis 6: seeded recorder log is preserved rather than truncated (DIRECT_WRITE / reused --run-dir))
• 🟡 No test invokes a shim the way the agent does — bare tool name resolved through the prepended PATH — so the #!/usr/bin/env python3 shebang and the .cmd twin's body (python "%~dp0<tool>" %*, its line endings) are asserted only by file existence. (trigger: tests/test_sandbox_record_cli.py) (restates: Axis 3: no test executes a shim by bare name through PATH; .cmd body unasserted)
• 🔵 No test covers concurrent recording: an agent can run several shimmed commands in parallel (background bash, xargs -P), and every shim appends to the same cli_mocks/calls.jsonl with a plain open(..., "a") + write, which is only atomic up to PIPE_BUF-sized lines — a long argv (multi-KB prompt argument) can interleave and be dropped by parse_log/cli_called as unparseable. Neither a test nor a documented size limit exists. (trigger: src/coder_eval/cli_recorder.py)

Downstream consumers:

• 🟡 The cli_called YAML example was not updated for the field's new default: docs/TASK_DEFINITION_GUIDE.md:798 still reads log: "mocks/calls.jsonl" # Path to the JSON Lines invocation log (required) immediately above the new paragraph at :809 saying it defaults to cli_mocks/calls.jsonl. The same stale "explicit log" shape is the only form shown in the CliCalledCriterion docstring examples (models/criteria.py:382,394), so no surface shows the zero-config form the feature exists to enable. (trigger: docs/TASK_DEFINITION_GUIDE.md)
• 🟡 Making CliCalledCriterion.log optional traded a load-time error for a grade-time one, and no compensating validation was added: a task that omits log and has neither record_cli nor a mock writing to cli_mocks/calls.jsonl now passes plan and fails at check time as a harness fault (missing-log error=). TaskDefinition already carries precedent for exactly this cross-check (check_directory_reference_compatibility, models/tasks.py:532), so a validator rejecting "default log with no producer" is the natural, missing counterpart. (trigger: src/coder_eval/models/criteria.py)
• 🔵 Nothing was updated to account for cli_mocks/ being harness-written content inside the graded workspace: it is preserved into the run artifacts and is visible to workspace-wide run_command criteria (ruff check ., pytest, git status --porcelain cleanliness gates) and to any glob-based file criterion for template/starter-file tasks whose content sits at the sandbox root — neither the docs nor a default ignore entry addresses it. (trigger: src/coder_eval/sandbox.py)

Daily/nightly:

• 🟡 The PR's stated motivation is a downstream suite whose five hand-written recording mocks drifted, but nothing states the blast radius on that nightly suite: no migration note, and a partial migration is a hard failure rather than a no-op — adding record_cli: [{tool: uip}] to a task that still carries its mock_path_dirs mock raises RuntimeError out of setup() (sandbox.py:493-501) and, being categorized as a retryable SANDBOX_SETUP_ERROR, burns three attempts per task before failing. (trigger: src/coder_eval/models/sandbox.py)
• 🟠 The nightly path is the worst case for the log-seeding guard and the PR does not say so: driver: docker defaults to PreservationMode.DIRECT_WRITE (orchestration/config.py:26), which never clears the target dir, so on a reused --run-dir or --resume the previous attempt's recorded invocations survive and score the current run. (trigger: src/coder_eval/sandbox.py) (restates: Axis 6: seeded recorder log is preserved rather than truncated (DIRECT_WRITE / reused --run-dir))

Harness & Lint Improvements

Static checks (lint / type):

• [ce-lint] CE026 — code-generating template placeholders must be !r-converted. New rule tests/lint/rules/ce026_template_placeholders_repr.py, wired into ALL_RULES in tests/lint/runner.py. Forbids a bare {name} field in any module-level string constant in src/coder_eval/ that is Python source (name matches *_TEMPLATE/*_SOURCE, or the literal starts with a #! shebang / is ast.parse-able): every replacement field must use !r or an explicit format spec. src/coder_eval/cli_recorder.py:23 (bare {tool} in the module docstring) and :25 (bare {log_filename}) are the only bare fields in _TEMPLATE — lines 35-41 already use {tool!r}/{exit_code!r}/{log_filename!r} — so the rule fires on exactly the two defective sites with zero noise. # noqa: CE026 for a deliberate non-code template. Prevents: A2-high — unvalidated tool interpolated raw into generated shim source: RecordedCli(tool='a"""\nimport os…') passes validate_tool_name and renders either uncompilable source (silent harness break scored as an agent failure) or executable code (task-YAML → code injection). Same shape at src/coder_eval/sandbox.py:523 (f'python "%~dp0{spec.tool}" %*').
• [ce-lint] CE032 — generated-source templates must render, parse, and pass the src/ rule set. Whole-tree rule wired as a @pytest.mark.lint test class (like CE027–CE031, since it must render before it can parse): for each code-shaped template constant in src/coder_eval/, substitute repr-safe dummy values for every field, ast.parse() the result (a SyntaxError fails the gate), then run the existing AST rules over that tree — plus a pass-only handler check that, unlike CE005, also covers narrow excepts (except OSError: pass) with no explanatory comment. Generated code is currently invisible to ruff, pyright, bandit and every CE rule because it lives inside a string literal. Prevents: A6-medium — src/coder_eval/cli_recorder.py:68-69 except OSError: pass (unbound, no comment) silently drops the log record that is the cli_called criterion's sole evidence, turning an unwritable log into a score-0 "agent never ran the command". Also back-stops A2-high: a template whose rendering cannot parse fails at make lint instead of at agent-invocation time.
• [ce-lint] Extend CE030 (doc-schema parity) to SandboxConfig and RecordedCli. One-line change to DOCUMENTED_MODELS in tests/lint/doc_schema_parity.py, which today registers only TaskDefinition, RunLimits, Dataset, SimulationConfig (all → docs/TASK_DEFINITION_GUIDE.md). SandboxConfig is the third -D-reachable root and is task-authored, yet carries no documentation obligation — which is exactly why this PR could add record_cli plus a whole new nested user-facing model (tool / exit_code / stdout / stderr) with the gate green. Prevents: A7-low — the guide (docs/TASK_DEFINITION_GUIDE.md:447) never states that exit_code defaults to 1, so - tool: curl silently makes the shadowed tool fail every call, and tool appears only inside a YAML example with no prose. CE030 would have failed the build until both were documented.
• [ce-lint] CE033 — a containment root stored on self must be .resolve()d at assignment. New rule tests/lint/rules/ce033_resolved_containment_roots.py in ALL_RULES: flag an assignment to a self.*_dir / self.*_root attribute whose RHS is Path(tempfile.mkdtemp(...)), tempfile.TemporaryDirectory(...), or a bare parameter Name, with no terminal .resolve(). Only 4 Path(tempfile.mkdtemp(...)) sites exist in src/ (sandbox.py:199, codex_agent.py:1170, user_simulator.py:256, docker_runner.py:539), so noise is nil. This fixes the class at the source; the alternative decidable form (X.relative_to(Y) where Y is not .resolve()-terminated — 10 call sites in src/) is a weaker variant. Prevents: A8-high — src/coder_eval/sandbox.py:186-201 stores sandbox_dir unresolved while _resolve_within_sandbox (:305) returns .resolve()d paths, so clash.relative_to(self.sandbox_dir) at :497 raises ValueError instead of the intended RuntimeError at :501. Two tests in tests/test_sandbox_record_cli.py are red on every macOS machine (/var→/private/var) and line 501 has zero coverage; sandbox.py:1022 is the same latent shape.
• [ce-lint] CE034 — in sandbox.py, config-supplied strings must be joined through _resolve_within_sandbox, never the bare / operator. New rule scoped to src/coder_eval/sandbox.py: flag ast.BinOp(op=Div) whose left operand is a sandbox-derived path and whose right operand is not a string literal or module-level constant (i.e. an Attribute such as spec.tool, an f-string built from one, or a subscript), unless the enclosing function is _resolve_within_sandbox itself. The module already routes every other author-supplied path through that containment seam (mock_path_dirs :464, starter_files :554, mount_point :346); the rule makes the convention mechanical. Prevents: A4-low — shim = recorder_dir / spec.tool (sandbox.py:515) and (recorder_dir / f"{spec.tool}.cmd") (:525) skip containment entirely, so a Windows drive-relative tool: "D:evil" (accepted by validate_tool_name) discards the sandbox prefix and writes an executable outside the sandbox; a\x00b likewise reaches write_text and dies mid-setup().
• [ce-lint] CE035 — model fields that escape into the OS must declare a constraint. Whole-tree rule with two checks: (a) any str field on a model in src/coder_eval/models/ whose name appears as the RHS attribute of a Path / join anywhere in src/ must declare pattern= (or be a Literal/enum); (b) any int field whose name matches *exit_code*/*exit_status* must declare ge=0, le=255. Sibling precedent for (b) already exists — ResourceLimits.max_cpus/max_pids use gt=0 (models/sandbox.py:36/:41). Document the cross-model name-collision caveat for (a) in the rule docstring. Prevents: A2-high / A4-low (RecordedCli.tool has only shape checks at models/sandbox.py:364 — no /, no \, not ./.. — so quotes, newlines, NUL, -rf, drive letters and reserved names all pass; pattern=r"^[A-Za-z0-9._+-]+$" closes injection, containment and most reserved-name cases at one seam) and A2-medium (exit_code: int = Field(default=1, …) at models/sandbox.py:338 accepts 256, which the OS truncates to a real status of 0 — an author's failing tool becomes a succeeding one, and the log's exit stops describing the process).
• [ce-lint] CE036 — generated launchers must not invoke a bare interpreter name. New rule: forbid a #!/usr/bin/env <interp> shebang, or a bare python/sh/node command, inside any code-shaped string constant in src/coder_eval/ — a generated artifact must embed an absolute interpreter (sys.executable / os.path.realpath(sys.executable)). Rationale for the docstring: the harness prepends the generated directory to PATH (sandbox.py:459-462 → orchestrator.py:1084), so any bare name in generated content is resolved through a PATH the harness itself just poisoned. Prevents: A4-medium (CVSS AV:L/AC:L/PR:H/UI:N/S:U/C:N/I:N/A:H) — cli_recorder.py:22 #!/usr/bin/env python3 and sandbox.py:523 bare python: tool: python3 makes the shim's own interpreter re-resolve to the shim, an infinite env→shim→env exec loop that hangs the task until the task timeout (reproduced: TimeoutExpired, log never written). ResourceLimits.max_pids/max_cpus are not enforced under driver: tempdir.
• [ce-lint] CE037 — no test-only public helper in the shipped package. Whole-tree rule: a module-level def in src/coder_eval/ with a public name, zero references anywhere in src/, and references in tests/, must be underscore-prefixed, re-exported through a package __init__/__all__, or moved into tests/. This makes CLAUDE.md's "Clean Code: no dead code" mechanical; exemptions go through an explicit EXEMPT map with a reason (mirroring CE030's registry style) for genuinely library-public API. Prevents: A5-medium — src/coder_eval/cli_recorder.py:104 parse_log is called from nowhere in src/ (its own definition is the sole grep -rn parse_log src/ hit); all six callers are in tests/test_sandbox_record_cli.py. Its docstring positions it as the canonical log reader while criteria/cli_called.py:209-223 keeps a second, already-divergent copy (parse_log silently continues where the criterion counts malformed and reports it at :258) — so a maintainer can edit parse_log believing they changed grading behaviour.
• [bandit-codeql] Extend the bandit/CodeQL scan to rendered generated source. Add a step to the security job (.github/workflows/pr-checks.yml bandit invocation, and make verify) that materialises every code-shaped template in src/coder_eval/ into tmp/generated-src/ with dummy values and runs bandit over that directory — the same rendering seam CE032 uses. Today bandit sees _TEMPLATE as an inert string literal, so nothing in the generated shim is scanned, even though the shim is the file the agent actually executes inside the sandbox. Prevents: A2-high (task-YAML → code injection in the rendered shim) and A6-medium (except OSError: pass) — both live entirely inside a string literal and are currently invisible to every configured security and quality scanner.

Harness improvements (not statically reachable):

• Add macOS to the CI test matrix (.github/workflows/pr-checks.yml runs the suite on ubuntu-latest only; there is a windows-smoke job but no darwin runner). Cheaper alternative if a third runner is unacceptable: a session-scoped conftest fixture that points TMPDIR at a symlinked directory for the Linux run, reproducing the resolved/unresolved split deterministically. Why not static: The defect manifests only as a runtime path mismatch under a symlinked temp root; Linux /tmp is a real directory, so the code reads fine and CI stays green. CE033 catches the code shape going forward, but only an OS with a symlinked temp root proves the existing suite passes where developers actually run it. Prevents: A8-high — tests/test_sandbox_record_cli.py is 2 failed / 27 passed on every macOS machine at PR HEAD (ValueError: '/private/var/…' is not in the subpath of '/var/…'), and sandbox.py:501's RuntimeError path has zero coverage in the whole suite. CI approved a file that is red for the author.
• Fresh-sandbox invariant: setup() must own and reset every artifact it generates. Make _generate_cli_recorders clear cli_mocks/ wholesale and seed calls.jsonl unconditionally instead of under if not log_path.exists():, then add a test that pre-populates <target>/cli_mocks/calls.jsonl with a record, runs Sandbox(...).setup(target_dir=target), and asserts the log is empty afterwards. Generalise it as a standing checklist item for any future harness-owned artifact under PreservationMode.DIRECT_WRITE. Why not static: The bug is cross-run filesystem state (a reused --run-dir, or --resume after a mid-run crash), not a code shape — the guard at sandbox.py:511 is locally idiomatic and reads as defensive. Only a test that seeds the directory before setup() distinguishes "preserve" from "reset". Prevents: A6-high — with DIRECT_WRITE (the driver: docker default, orchestration/config.py:26) the orchestrator deliberately does not clear the target dir, the shim appends, and a prior run's uip ixp projects delete record scores the current run: CliCalledCriterion(min_count=1) returns 1.0 with zero agent activity. Same agent output, different score depending on run-dir reuse.
• Generated-artifact / guard parity test. Extract one generated_names(spec) -> set[str] helper (spec.tool, f"{spec.tool}.cmd", plus any future PATHEXT twin), have both the writer and the collision guard consume it, and add a test asserting that the set of names _generate_cli_recorders actually wrote equals the set the guard checked — plus a refusal to overwrite any pre-existing file inside the generator's own directory. Why not static: The written-name set only exists after running the generator against a real directory; no AST rule can relate shim.write_text at one line to clash.exists() at another. This is the same shape as the event-reassembly parity tests the repo already requires (assert over the full name set, not a hand-picked subset). Prevents: A1-medium (the guard checks only user_dir / spec.tool, never f"{spec.tool}.cmd", so on Windows a task's own mocks/uip.cmd is silently shadowed by cli_mocks/uip.cmd — precisely what the "can never silently shadow" comment at sandbox.py:455-458 promises is impossible) and A3-medium (tool: "calls.jsonl" overwrites the seeded log with shim source — reproduced, 2261 bytes; [tool: "uip.cmd", tool: "uip"] leaves uip.cmd clobbered by the batch twin — both silent, both scoring 0).
• Execute the shim the way the agent does: by bare name, through PATH. Add a POSIX test that runs subprocess.run(["uip", "projects", "list"], env={**os.environ, "PATH": f"{recorder_dir}{os.pathsep}{os.environ['PATH']}"}) and then grades the resulting log, plus a content assertion on uip.cmd (python "%~dp0uip" %*, CRLF) instead of the current existence-only check. Why not static: Shebang honouring, the +x bit, and Windows PATHEXT resolution are OS behaviours; no lint rule can assert that a bare tool name resolves to the generated shim. Prevents: A3-medium — the sole invoker (tests/test_sandbox_record_cli.py:39) is [sys.executable, str(shim), *args], an explicit interpreter on an absolute path, so the #!/usr/bin/env python3 line is never exercised and the .cmd body is never run or asserted. A malformed batch line or a broken shebang ships with all 29 tests green.
• Harness-fault vs agent-failure resilience test for the recorder log. chmod 444 the seeded log, invoke the shim, and assert that cli_called reports a harness fault (error= set) rather than score=0.0 — which requires the shim to leave a trace on OSError (a calls.jsonl.error sentinel and/or a stderr line, each in its own try/except) and cli_called to surface it. Generalise as a rule of thumb: any criterion whose evidence the harness itself produces needs an "evidence unavailable" state distinct from "agent did nothing". Why not static: Distinguishing a harness fault from a genuine agent miss is a semantic contract about scoring, and the trigger (unwritable log — ENOSPC, permission change, agent tampering) is runtime environment state. CE032 catches the traceless pass; only this test pins the resulting verdict. Prevents: A6-medium — reproduced: with the log read-only the shim exits with the configured code, stdout/stderr empty, and the criterion returns score=0.0, error=None, details="0 invocation(s) recorded …" — byte-identical to the agent never running the command. The criterion already distinguishes a missing log (harness fault) from an empty one; an unwritable log defeats exactly that distinction.
• Make the merge-strategy table exhaustive by construction. Replace the hardcoded parametrize list in tests/test_merge_strategy_annotations.py:31-48 with one derived from model_fields over the three -D-reachable roots (AgentConfig / RunLimits / SandboxConfig): iterate every field, look it up in an explicit expected-strategy mapping, and fail on any field absent from that mapping. Adding a merge-relevant field then forces an explicit strategy decision in the same change. Why not static: CE014 already enforces that an annotation exists but explicitly "only requires the annotation, not a particular strategy" — the intended strategy is a semantic decision. Only table completeness is checkable, and that needs the runtime Pydantic model registry, not an AST walk. Prevents: A3-medium — record_cli is the one merge-relevant SandboxConfig field missing from that list (mock_path_dirs/replace sits on line 34), so a future edit flipping it to strategy="append" would pass make lint and the entire suite while contradicting the field's own documented contract ("Replaced (not merged) across config layers, like mock_path_dirs", models/sandbox.py:432).
• Reserved-name denylist plus an exec-loop regression test. Reject python, python3, env, sh, bash, node, git, uv, cmd (and the generator's own artifact names) in validate_tool_name, and add a hard-timeout test asserting that a generated shim on a prepended PATH never re-execs itself. Document the shadowing hazard in docs/TASK_DEFINITION_GUIDE.md:434-455, which currently covers only mock_path_dirs collisions. Why not static: The denylist itself is a code fix, but the consequence — an unbounded exec loop, and git/uv/curl being shadowed for every run_command criterion via _sync_sandbox_command_path_with_agent → _build_run_command_env — is runtime PATH-resolution behaviour that only a bounded subprocess test can demonstrate. Prevents: A4-medium — tool: python3 hangs the task until the task timeout with no diagnostic (reproduced: TimeoutExpired, calls.jsonl never written), and tool: git/uv/curl silently changes what every run_command criterion executes, and therefore the score.
• Assert that deterministic task-config faults are non-retryable. Add a test pinning categorize_error (or the raised type) for the record_cli/mock_path_dirs collision to a max_retries=0 category — via a typed non-retryable error in errors/ instead of the bare RuntimeError at sandbox.py:501, or a config-fault pattern in the sandbox arm of _categorize_by_component. Why not static: Retry policy is a runtime mapping from exception + context to a RetryConfig; an AST rule can see the raise RuntimeError(...) but not that it lands in SANDBOX_SETUP_ERROR with max_retries=2. Prevents: A6-low — an unfixable YAML conflict currently burns 3 attempts and ~27s of backoff per task and logs the same confusing message three times, on a fault that cannot possibly succeed on retry.

Top 5 Priority Actions

1. Seed the recorder log unconditionally at /Users/religa/src/coder_eval/src/coder_eval/sandbox.py:511 (drop the if not log_path.exists(): guard, and ideally clear the whole cli_mocks/ dir before regenerating), because under PreservationMode.DIRECT_WRITE or a reused --run-dir the append-mode shim lets a prior run's invocations score the current one — reproduced as cli_called returning 1.0 with zero agent activity in the current run.
2. Resolve the sandbox root once before the containment comparison at /Users/religa/src/coder_eval/src/coder_eval/sandbox.py:497 (clash.relative_to(self.sandbox_dir.resolve()), or just clash.name), since _resolve_within_sandbox returns a symlink-resolved path while sandbox_dir is stored raw — on macOS (/var -> /private/var) this raises a pathlib ValueError out of setup(), turning a deterministic task-config error into a thrice-retried SANDBOX_SETUP_ERROR and leaving the documented RuntimeError at line 501 permanently uncovered with two tests red.
3. Replace the shape-only check in validate_tool_name at /Users/religa/src/coder_eval/src/coder_eval/models/sandbox.py:364 with a strict allowlist (re.fullmatch(r"[A-Za-z0-9._+-]{1,64}", v)) plus a reserved-name set (python/python3/env/sh/bash/node/git/uv/cmd, calls.jsonl, <tool>.cmd), and emit an absolute interpreter instead of #!/usr/bin/env python3 (/Users/religa/src/coder_eval/src/coder_eval/cli_recorder.py:22) and bare python (sandbox.py:523) — today tool: python3 hangs the task in an infinite exec loop, tool: git/uv silently shadows binaries for run_command criteria via orchestrator.py:1084, tool: calls.jsonl overwrites the log the criterion grades, and an embedded \"\"\" renders uncompilable (or executable) shim source at cli_recorder.py:23.
4. Stop swallowing recorder log-write failures at /Users/religa/src/coder_eval/src/coder_eval/cli_recorder.py:68 — bind the exception, write a stderr line and/or a calls.jsonl.error sentinel, and have criteria/cli_called.py surface that as error= — because an unwritable log currently produces output byte-identical to "the agent never ran the command" (score=0.0, error=None), defeating the missing-vs-empty distinction the seeding logic exists to provide.
5. Close the remaining schema and single-source gaps: bound exit_code with ge=0, le=255 at /Users/religa/src/coder_eval/src/coder_eval/models/sandbox.py:338 (256 silently becomes exit 0, so a tool configured to fail is observed succeeding), reject duplicate record_cli[*].tool entries (models/sandbox.py:422) and widen the collision guard to the .cmd twin (/Users/religa/src/coder_eval/src/coder_eval/sandbox.py:493), add the missing (SandboxConfig, "record_cli", "replace") row to tests/test_merge_strategy_annotations.py, and either have CliCalledChecker consume parse_log (/Users/religa/src/coder_eval/src/coder_eval/cli_recorder.py:104) or move that test-only, already-divergent duplicate parser into tests/.

---

Stats: 0 🔴 · 3 🟠 · 8 🟡 · 8 🔵 across 8 axes reviewed.

[uipreliga]

Review

Read the full branch diff and ran the checks locally (macOS).

Verdict: fundamentally correct, architecturally consistent, and genuinely useful. It fills a real gap — asserting on what the agent executed against a shadowed CLI, which command_executed can't do because it matches tool telemetry strings rather than argv. But the branch is red on macOS, and one of the two failures is a real production bug, not a test artifact.

Test status

4041 passed  (whole suite, with the 2 new failures deselected)
tests/test_sandbox_record_cli.py::TestGeneration::test_collision_with_user_mock_raises          FAILED
tests/test_sandbox_record_cli.py::TestGeneration::test_recorder_dir_is_path_prepended_...       FAILED

make check and make lint (166 rules) pass; pyright is clean on the new files. Both failures stem from the /var → /private/var symlink on macOS, so CI on Linux likely stays green.

Must fix

1. relative_to raises ValueError on the collision path (sandbox.py:497) — real bug, hits any symlinked tempdir (macOS).

clash comes from _resolve_within_sandbox, which returns .resolve()d paths (/private/var/...), while self.sandbox_dir is the raw mkdtemp path (/var/...). So the intended friendly error:

record_cli would generate a 'uip' shim, but mock_path_dirs entry 'mocks' already provides one...

never gets built. The user instead gets an opaque ValueError: '/private/var/…' is not in the subpath of '/var/…', retried three times by execute_with_retry, landing as a generic setup ERROR. Fix: clash.relative_to(self.sandbox_dir.resolve()), or just print f"{rel}/{spec.tool}" and skip the computation.

2. test_recorder_dir_is_path_prepended_before_user_mocks compares sandbox_dir / RECORD_CLI_DIR (unresolved) against the resolved value the property returns. Test bug — assert against (sandbox_dir / RECORD_CLI_DIR).resolve().

Worth addressing

3. cli_recorder.parse_log is production code with zero production callers. criteria/cli_called.py re-implements the same JSON-Lines loop inline (because it needs the unusable-record count). Against the repo's stated "no dead code / DRY" principle. Either make parse_log return (records, unusable_count) and have the checker use it, or move it into the test file.

4. Clustered short flags defeat the alias work. -yf parses as one flag named yf, so an aliases: ["y"] predicate misses it. Given how hard the docs argue that a negative guard must not be escapable — and that aliases exists precisely to close the -y/--yes hole — this leaves the same hole one keystroke away. At minimum document it; ideally split single-dash multi-char tokens into single-char flags when no declared name matches.

5. Bare negative positionals are swallowed. -1 (not --x=-1, which is handled correctly) hits token.startswith("-"), becomes a flag named 1, and drops out of positional. Same failure class as the --yes proj-1 bug this branch fixed.

6. tool is interpolated unescaped into the shim's docstring and the .cmd line ({tool}, not {tool!r}). The validator only rejects separators and whitespace, so a name containing """ renders a shim that dies with a SyntaxError. Task-author-controlled, so not a security issue, but this codebase normally validates that shape.

7. Doc contradiction: the cli_called YAML example comments log: as (required), while the paragraph immediately below correctly says it defaults to cli_mocks/calls.jsonl.

8. Note-only: criteria run_command inherits the agent's PATH (set_command_base_path), so a criterion that invokes a shadowed tool appends to the same log after the agent finished, perturbing counts for any later criterion. Probably fine in practice, but worth a sentence in the guide.

What's right about it

• Follows the extension points in CLAUDE.md exactly: model → union → @register_criterion checker → docs → tests. record_cli uses MergeField(strategy="replace") consistently with mock_path_dirs.
• Declared value-binding instead of heuristic is the key design call, and it's the correct one. The commit history shows it was reached by fixing a false PASS on delete --yes proj-1 — right instinct, and the present predicate (which deliberately does not make a flag value-bearing) properly closes the reintroduction path.
• Fail-loud on a missing/unusable log, plus seeding an empty log at setup so "called nothing" and "mock never ran" stay distinguishable — that pair is subtle and correctly handled.
• Load-time rejection of ignore_flags/predicate overlap, alias collisions, blank verb, and flags beside a non-regex predicate. Good validator hygiene.
• Docs are unusually good; the "negative guards want the fewest facets" section is real, transferable guidance.

The two features (criterion + recorder) are coupled only by the default log path, so they could have been reviewed separately — but shipping them together is defensible since neither is much use alone.

[uipreliga]

Fix the issues identified before merging.

[alexandrujircan]

Rebased onto main now that #72 is in (it was squash-merged, so this branch carried its five commits separately and went CONFLICTING). All three blockers fixed in 27d81fb, the two remaining escape shapes in 8a6cf9a. I reproduced every claim before changing anything.

1. tool interpolated unescaped into generated source

RecordedCli(tool=<a-triple-quote-b>) validates
compile(render_recorder(spec)) -> SyntaxError: unterminated string literal

Constrained to pattern=r"^[A-Za-z0-9._+-]+$", and the parametrized case list now covers a triple-quote, newline, space, ; and $. It had only probed path-shaped names, which is exactly why the quote case was invisible.

2. Seeded log preserved rather than truncated

stale record after setup(target_dir=...): {"argv": ["ixp","projects","delete","proj-1"]}
CliCalledCriterion(verb='ixp projects delete', min_count=1) -> 1.0

1.0 with zero agent activity in that run. You were right that the if not log_path.exists() guard's only possible effect was preserving a previous run's log. It is now truncated unconditionally, and the whole cli_mocks/ directory is wiped before regeneration so a shim for a tool no longer in record_cli cannot linger on PATH. Two tests: the reused-target-dir case and the stale-shim case.

3. relative_to on an unresolved root

Worth stating plainly: this one could not have failed on my machine. resolved_mock_path_dirs returns resolved paths while sandbox_dir stays raw, and on Windows those compare equal — so my assertion passed and the RuntimeError path was never covered by anything. Two of your runs were red while my whole suite was green. The message no longer computes a relative path, and the path-prepend test compares resolved to resolved.

Clustered short flags and negative positionals

Fixed rather than documented, since as you say the first leaves the -y hole one keystroke away:

-rf  (undeclared)  -> {'r': [''], 'f': ['']}     predicates on r and f both see it
-yf  + aliases [y] -> absent guard now fires
-rf  (declared)    -> {'rf': ['']}               a real multi-char short flag stays whole
-ff-002            -> {'f': ['f-002']}           attached value when f is value-bearing
seek -1            -> positional ['seek', '-1']
head -1 (declared) -> {'1': ['']}

Both stay declaration-driven, same as value binding: a name the criterion mentions is taken whole, everything else is split. -1 staying positional matters for the same reason as --yes proj-1 — a token that silently leaves the positionals is how a guard stops guarding.

Also

• parse_log had no production callers while the checker duplicated the loop. It now returns (usable, unusable_count) and is the single reader; _usable_argv is gone.
• Renamed cli_recorder.py to invocation_log.py. CE004's prefix match reads coder_eval.cli_recorder as the cli layer, and the module owns both halves now, so naming it for the artifact is better anyway. I preferred that to a # noqa on a rule that is right in spirit.
• Doc contradiction fixed — log: was commented (required) one line above the paragraph documenting its default.
• run_command inheriting the agent's PATH — noted, not documented yet. Real, but I would rather add that sentence when someone hits it than guess at the wording now.

Gate

Full suite 3747 passed, 5 failures that all reproduce on origin/main (3x test_reports_stats_nonfinite, 2x test_sandbox symlink tests needing Windows privileges). make lint 171 rules pass, CE004 included. pyright at its 3 pre-existing openai_codex errors. The cli_called differential against the downstream suite still agrees 7/7 on both trajectories.

Windows/Python 3.13.9 here, so your macOS run stays the check I cannot reproduce — if the two failures you saw are gone on your side, that is the confirmation I am missing.

Your approval predates all of this, so another look before merge would be welcome.

[alexandrujircan]

Worked through the What's Missing section too — I had only acted on Blockers and the follow-up comment before, so thanks for the structure that made the gap obvious. Fixed in 1165c83.

The 🟠 that mattered most

_refresh_plugin_tools_dir really does break on the documented example. Reproduced:

which('uip') on uip_search_path  -> <sandbox>/cli_mocks/uip.CMD
which('uip') on discovery path   -> C:\Users\...\.bun\bin\uip.EXE

A shim is not inside a node_modules/@uipath tree, so resolve_uipath_plugin_dir returned None and the MST-9795 pin silently stopped being exported to every run_command criterion — for record_cli: [{tool: uip}], which is the example in every doc I wrote. Plugin discovery now filters the generated dir out, with a test asserting the dir is on uip_search_path but not on the discovery path.

Deliberately narrow: a hand-written mock under mock_path_dirs shadows that lookup the same way, but it predates this feature and changing it would alter existing tasks, so I left it and said so in the comment.

Also fixed

• agent_judge leak — cli_mocks is now in default_ignore_patterns.yaml, so the workspace copy no longer hands a Bash-enabled judge the generated shim source and calls.jsonl as authored files. Artifact capture uses a separate list (_WORKSPACE_CAPTURE_IGNORE), so the log is still preserved as evidence.
• The stdin invariant — the docstring gives "it would hang the task" as the reason for never reading stdin, and nothing asserted it. There is now a test that leaves stdin=PIPE unwritten and unclosed and asserts the shim still returns.

Deferred, with reasons

• No "default log with no producer" validator. Agreed this is the natural counterpart to check_directory_reference_compatibility, and I would rather add it as its own change than bolt a cross-model validator on here.
• No zero-config example. Correct — every surface still shows an explicit log:. Worth fixing when a task YAML actually ships using record_cli, so the example can be copied from something real rather than invented.
• No migration note for the downstream suite, including that a partial migration raises out of setup() and burns three retries as a SANDBOX_SETUP_ERROR. That belongs with the migration PR, which is written but blocked on a release.
• cli_mocks/ visible to workspace-wide criteria (git status --porcelain gates, ruff check .) — real, and the same class as the judge leak, but the fix is a per-criterion or per-repo decision rather than a default I should pick.
• Concurrency (🔵). Worth flagging that this one got louder, not quieter: since unusable records now fail the criterion rather than being skipped, an interleaved write past PIPE_BUF fails the task instead of vanishing. Better failure mode, still a real limit, still undocumented.

Gate

Full suite 3777 passed, 5 failures that all reproduce on origin/main. make lint 171 rules. pyright at its 3 pre-existing openai_codex errors.

One thing I cannot verify from here: the bare-name PATH test skips on Windows, so it will execute for the first time on CI (or on your macOS). That is the test most worth watching, since its absence is what let the shebang, the +x bit and the PATH prepend all go unexercised.

Your approval is on 2ec7154, four commits back — a look at 1165c83 before merge would be welcome.