Uh oh!
There was an error while loading. Please reload this page.
refactor(isolation): run registered agents in an unprivileged worker - #97
refactor(isolation): run registered agents in an unprivileged worker#97dmorosanu wants to merge 1 commit into
Conversation
1dd19ec to
7a2c59aCompare1ce9fb1 to
ce92037CompareClaude finished @dmorosanu's task in 1m 37s —— View job Code Review in Progress |
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:97 (18 files) axis:1,2,3,4,5,6,7,8
Scope: pr:97 (18 files) axis:1,2,3,4,5,6,7,8 · branch codex/generic-agent-worker · ce92037 · 2026-08-11T04:14Z · workflow variant
Change class: complex — introduces a new cross-process nonce-framed RPC protocol, a stateful unprivileged worker owning the full Agent lifecycle, kernel-identity/capability handshakes, and fail-closed teardown; correctness requires reasoning about concurrency, security boundaries, and the Agent ABC contract
The codebase remains in strong shape (9.1/10) — type safety, API surface, security and architecture all score 9.4+, and the new root/UID-2000 agent-isolation boundary is a genuine improvement — but the risk is concentrated in the new isolation/agent_worker.py seam, whose weakest axis (Evaluation Harness Quality, 7.5) reflects three real ways a scored run can silently degrade for identical agent output: hard-killed isolated turns lose their token/cost/transcript telemetry, the privilege-drop capability label was not bumped when the host-side agent-kind allowlist was removed (so a stale-but-labeled image fails open to root), and the worker stays ptrace/memory-readable by its own same-UID descendants (so the graded trajectory and reported cost are forgeable); combined with a 28.6%-covered, deadline-free RPC layer that is now the production agent path for every containerized run, the bottom line is that the feature is architecturally sound but needs the telemetry, fail-closed and coverage gaps closed before it is trusted as the default.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 8.7 / 10 | 0 | 0 | 2 | 3 | New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard (bypasses tests/test_route_seam_exhaustiveness.py) |
| 2. Type Safety | 9.5 / 10 | 0 | 0 | 1 | 0 | Live _WorkerWriter callback smuggled through the JSON params dict under a magic "_writer" key, so it is never type-checked against StreamCallback |
| 3. Test Health | 9 / 10 | 0 | 1 | 0 | 0 | The new isolation worker/proxy boundary has essentially no behavioral tests: proxy RPC + crash-partial handoff + teardown, _WorkerServer.handle dispatch branches, both fail-closed security gates (identity handshake, RPC nonce), and cross-process event forwarding are all uncovered |
| 4. Security | 9.4 / 10 | 0 | 0 | 1 | 1 | Agent worker stays ptrace/memory-accessible to its own UID-2000 descendants (dumpable resets to 1 on exec), so the RPC nonce is recoverable from worker memory and the worker can be hijacked into emitting frames the root harness fully trusts — but the cited /proc/<worker_pid>/fd/0 read and /proc/<worker_pid>/fd/1 write route does NOT work (root-owned 0600 pipe inodes ⇒ EACCES) |
| 5. Architecture & Design | 9.4 / 10 | 0 | 0 | 1 | 1 | SAFE_CODER_EVAL_ENV allowlist is hand-picked, not consumer-derived: it exempts CODER_EVAL_IN_CONTAINER (no reader, contradicting the doc this PR edited) while the blanket CODER_EVAL prefix scrub strips CODER_EVAL_RAW_SDK_LOG, which the worker reads |
| 6. Error Handling & Resilience | 9 / 10 | 0 | 1 | 0 | 0 | No per-RPC deadline on the worker handshake: start()/ping run before the task_timeout watchdog arms, so a live-but-mute worker hangs the task forever |
| 7. API Surface & Maintainability | 9.9 / 10 | 0 | 0 | 0 | 1 | agent_worker_internal_command follows the hidden-CLI-command naming convention but is a main hook, and the new public names are not in any all |
| 8. Evaluation Harness Quality | 7.5 / 10 | 0 | 2 | 1 | 0 | Host-side agent-kind allowlist removed without bumping the uid-gid-v1 capability label, so a stale-but-labeled image plus new host code runs a third-party plugin agent as root with no error |
Overall Score: 9.1 / 10 · Weakest Axis: Evaluation Harness Quality at 7.5 / 10
Totals: 🔴 0 · 🟠 4 · 🟡 6 · 🔵 6 across 8 axes.
Blockers
- [Axis 3] The new isolation worker/proxy boundary has essentially no behavioral tests: proxy RPC + crash-partial handoff + teardown, _WorkerServer.handle dispatch branches, both fail-closed security gates (identity handshake, RPC nonce), and cross-process event forwarding are all uncovered (
src/coder_eval/isolation/agent_worker.py:337) —agent_isolationdefaults toTrue(src/coder_eval/models/sandbox.py:198agent_isolation: bool = Field(default=True,), and orchestrator.py:1316-1324 now returnsIsolatedAgentProxy(...)instead ofcreate_agent(...)on that path — so this class is the production Agent for every containerized run. The only test that touches it is an identity check: tests/test_orchestrator.py:379assert isinstance(agent, IsolatedAgentProxy)/ :380assert agent.agent_kind == "claude-code".grep -rn "IsolatedAgentProxy" tests/returns exactly those two assertion lines plus the import. Routed coverage confirms it: agent_worker.py = 412 stmts / 278 missing / 28.60%, with 371-408 (_spawn), 411-448 (_read_stdout), 473-490 (_request), 493-504 (_apply_snapshot), 508-530 (_raise_remote_error), 539-553 (start), 564-600 (communicate), 610-623 (stop), 626-637 (kill_sync) all uncovered. The single highest-value missing case is the crash contract at lines 587-593 —except BaseException:→self.pending_turn = partial.model_copy(update={"crashed": True, "crash_reason": "agent worker terminated"})— which is the exactcrashed=Truepartial-TurnRecordhandoff the orchestrator's_on_attempt_failuredrains; if it regresses, a crashed turn silently vanishes fromresult.turnsand the persisted task.json. Add unit tests that drive proxy↔worker in-process: extract the launch argv (CONTAINER_DROP_SHIM, sys.executable, "-I", "-m", ...at lines 375-379) into an overridable seam so a test can spawn the worker module directly (no drop shim, any platform) against a registry test agent like the existing_PluginAgent, then assert (i) a normal turn round-trips, (ii) a worker killed mid-turn yieldspending_turn.crashed is True, (iii) a worker-raisedTurnTimeoutErrorre-raises asTurnTimeoutErrorwithtimeout_seconds/iterationpreserved. - [Axis 6] No per-RPC deadline on the worker handshake:
start()/pingrun before the task_timeout watchdog arms, so a live-but-mute worker hangs the task forever (src/coder_eval/isolation/agent_worker.py:484) —_requestawaits the response future with no deadline —stop()is the only RPC with one (line 616,asyncio.wait_for(self._request("stop", {}), timeout=_STOP_TIMEOUT_SECONDS)):
472: async def _request(self, method: str, params: dict[str, Any]) -> Any:
...
484: response = await future
and the handshake plus start run unguarded:
390: hello = await self._request("ping", {})
Failure scenario: IsolatedAgentProxy.start() is invoked from Orchestrator._setup() (orchestrator.py:1105 inside execute_with_retry at 1111, which applies retries but NO timeout — verified: zero timeout/wait_for in errors/executor.py and errors/retry.py), and _setup() is awaited at orchestrator.py:468, i.e. BEFORE with ThreadedWatchdog( at orchestrator.py:495 arms task_timeout. orchestration/batch.py has no per-task timeout (zero timeout matches) and DockerRunner puts no wall clock on docker run (its heartbeat at docker_runner.py:135-142 detects HOST death, not a stalled container). So a worker that holds stdout open without answering ping/start — e.g. the third-party AgentRegistry plugin this PR exists to support blocks inside ensure_plugins_loaded() (line 224), or the setpriv shim wedges — hangs the task, the batch, and the nightly job indefinitely. This is a regression: the replaced ClaudeCodeAgent.start() is pure local assignment (agents/claude_code_agent.py:737-741) and cannot hang. discard_pending_turn (line 606), on the crash-recovery path, is likewise unbounded.
Aggravating: _WorkerWriter.write swallows a failed response write — with self._lock, contextlib.suppress(BrokenPipeError, OSError): (line 197) — so the worker cannot report that the reply never left, leaving the untimed parent waiting on a future nothing will resolve.
Fix: give _request a per-method deadline (a startup/handshake timeout for ping/start, turn_timeout-derived for communicate) and raise AgentCrashError/TurnTimeoutError after killing the worker; do not suppress OSError on a response write — abort the worker so the parent sees EOF.
3. [Axis 8] Host-side agent-kind allowlist removed without bumping the uid-gid-v1 capability label, so a stale-but-labeled image plus new host code runs a third-party plugin agent as root with no error (src/coder_eval/isolation/docker_runner.py:701) — _validate_agent_isolation_compatibility lost its allowlist (supported_agents = {CLAUDE_CODE, CODEX, ANTIGRAVITY, NONE} and the raise DockerRunError("...has no verified UID-drop launch seam for agent type...")); line 701-702 is now just if not self._docker_config.agent_isolation: return. That gate is HOST-side; the replacement enforcement (IsolatedAgentProxy, orchestrator.py:1313-1323) lives in the IMAGE. The image capability label is unchanged — docker/Dockerfile:110 still LABEL org.coder-eval.agent-isolation="uid-gid-v1" — and the only version check, _preflight_image_version (docker_runner.py:254-259), is a logger.warning ("Image %s coder_eval %s != host %s. Rebuild with make docker-image"), i.e. soft-fail by design.
So new host code + any pre-PR uid-gid-v1 image passes _preflight_agent_isolation_image (line 288), the host no longer rejects a third-party AgentRegistry kind, and the old in-container orchestrator has no launch seam for it: it calls create_agent(...) in the root process, so the plugin agent and all candidate code run as ROOT with full access to /opt/coder-eval/grader (hidden task data, references, the run output dir). Isolation fails OPEN and silently — exactly the fail-closed property #87 was built for. The reverse skew (new image + old host code) is benign (old host rejects the kind).
Fix: bump the capability label to a new value (e.g. uid-gid-v2) in docker/Dockerfile:110 and require it in _preflight_agent_isolation_image, so a stale image is rejected loudly instead of degrading; or make _preflight_image_version a hard error when agent_isolation is on. Either way, state in the PR body that make docker-image / the ghcr push is a lockstep prerequisite — the nightly runs :latest from ghcr (docker-publish.yml pushes on main), so between merge and the next image publish the nightly is in exactly this skew window.
4. [Axis 8] Hard-killed isolated turns lose token/cost/transcript telemetry: killpg(SIGKILL) on the worker session prevents the terminal AgentEndEvent, so the recovered partial has token_usage=None and agent_output="" (src/coder_eval/isolation/agent_worker.py:588) — On the task_timeout path the watchdog calls agent.kill_sync() (orchestrator.py:493) and Orchestrator._drain_killed_turn (orchestrator.py:640-671) recovers the parked partial, logging "Recovered the hard-killed turn: %d tokens, %s" — and the docstring states the recovered turn "feeds token aggregation and command stats like any other". In-process that partial is complete: ClaudeCodeAgent catches the CancelledError and calls self._finalize_external_cancel(state.finalize) (claude_code_agent.py:1024), which emits the terminal AgentEndEvent carrying usage, messages, agent_output, duration_seconds.
IsolatedAgentProxy.kill_sync (agent_worker.py:625-637) instead killpg(process.pid, SIGKILL)s the whole worker session, so the in-worker agent's cancel handler never runs and no AgentEndEvent ever crosses the pipe. The proxy's fallback at agent_worker.py:587-593 (except BaseException: / partial = collector.build_turn_record()) therefore hits EventCollector.build_turn_record's if end is None: branch (streaming/collector.py:149-160), which returns token_usage=None, messages=[], agent_output="", duration_seconds=0.0. Net effect on every isolated run that hits run_limits.task_timeout or the wait_for backstop: the final (usually longest) turn contributes 0 tokens and "unpriced" cost to total_token_usage, has an empty transcript, and reports agent_output="" to any trajectory-consuming criterion (llm_judge) — so cost/token rows in the nightly reports under-report and a judge score can differ for identical agent output. The reconciliation invariant is not violated (no token_usage to reconcile against) but the data is simply gone.
Fix: before SIGKILL, give the worker a chance to finalize — e.g. send SIGTERM to the worker only (not the group), let _serve_worker's finally: await server.close() / the agent's cancel handler emit the terminal AgentEndEvent, then escalate to killpg(SIGKILL) after a short grace; or have the worker install a SIGTERM handler that finalizes the in-flight turn and writes one last event frame. Add a test asserting a killed isolated turn still yields a partial with non-None token_usage.
Non-blocking, but please consider before merge
- [Axis 1] New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard (bypasses tests/test_route_seam_exhaustiveness.py) (
src/coder_eval/isolation/agent_worker.py:112) —_route_to_payloadkeys the wire format ontype(route).__name__(line 106:return {"type": type(route).__name__, "data": dataclasses.asdict(route)}) and_route_from_payloaddecodes it with a hand-maintained literal map:
112: route_types: dict[str, type[DirectRoute] |type[BedrockRoute] |type[LiteLLMRoute]] = {
113: "DirectRoute": DirectRoute,
114: "BedrockRoute": BedrockRoute,
115: "LiteLLMRoute": LiteLLMRoute,
116: }src/coder_eval/models/routing.py:126 already owns exactly this concern: ROUTE_NAMES: dict[type, str] = {...} — "Stable string names for environment_info recording (decoupled from class names)". A fourth ApiRoute variant added to the union at routing.py:122 must now be mirrored in both tables, and only one of them lives next to the union. Line 520 repeats the same shape a second time (exception_types: dict[str, type[Exception]] = {...} with 7 literal class names), so an error type not in that list silently degrades to AgentCrashError. Derive both maps from the SSOT — build the route codec off ROUTE_NAMES (or a {cls.__name__: cls for cls in typing.get_args(ApiRoute)} comprehension) and colocate the exception map with coder_eval.errors — and add a test asserting every get_args(ApiRoute) member round-trips through _route_to_payload/_route_from_payload.
2. [Axis 1] Privilege-drop/identity handshake collapses 10 conditions into one boolean and rejects with a single opaque error that dumps raw state without naming the failed field or expected value (src/coder_eval/isolation/agent_worker.py:209) — uv run radon cc -s -n C src/coder_eval/isolation/agent_worker.py at PR HEAD: _WorkerServer.handle - C (16)@209, IsolatedAgentProxy._read_stdout - C (15) @410, IsolatedAgentProxy._spawn - C (13)@370, _serve_worker - C (12)@277 — all 100% new code, in a module that is now on the production Docker run path. The worst readability cost is a single boolean at 394–405 that mixes process spawn with an 11-clause kernel assertion:
394: privilege_drop_ok= (
395: isinstance(hello, dict)
396: andhello.get("uid") ==AGENT_UID
...
404: andall(value==0forvalueincapabilities.values())
405: )
406: ifnotprivilege_drop_ok:
...
408: raiseRuntimeError(f"agent worker did not enter the configured unprivileged security domain: {hello!r}")An operator debugging a failed handshake gets the whole hello dict and must diff 11 conditions by eye. Extract a _privilege_drop_mismatch(hello) -> str | None that returns the first failing check by name (and name it in the error), splitting _spawn into spawn + verify. Likewise split handle (a 5-branch method-name ladder → a {method: coroutine} dispatch table) and _read_stdout (frame parse vs. event dispatch vs. response routing) into helpers.
3. [Axis 2] Live _WorkerWriter callback smuggled through the JSON params dict under a magic "_writer" key, so it is never type-checked against StreamCallback (src/coder_eval/isolation/agent_worker.py:248) — params is the model of the JSON request body (async def handle(self, method: str, params: dict[str, Any]) -> tuple[Any, bool], line 209), but a live in-process object is injected into it and pulled back out untyped:
ifrequest.get("method") =="communicate":
params["_writer"] =writer# line 310
...
writer=params.pop("_writer") # line 248 -> typed Anyrecord=awaitself.agent.communicate(
str(params["user_input"]),
stream_callback=writer, # line 251Because params.pop(...) on a dict[str, Any] yields Any, pyright performs no check that _WorkerWriter (lines 188-202) satisfies the StreamCallback Protocol (streaming/callbacks.py:12-17) — this is the only place the two are connected, and it is the one place the connection is unchecked. _WorkerWriter never declares conformance either (no StreamCallback base, no explicit annotation), so if on_event's signature changes the breakage surfaces only at runtime, inside the unprivileged worker subprocess, mid-turn. It also makes the declared type of params untrue for its actual contents (a non-JSON object inside the JSON payload dict), which is why strict pyright reports agent_worker.py:313:87 - error: Argument type is partially unknown ... "dict[Unknown, Unknown]" for the handle(..., params) call.
Fix: hand the writer over as a real parameter instead of a dict key — e.g. async def handle(self, method: str, params: dict[str, Any], *, writer: StreamCallback | None = None) — and drop the params["_writer"] injection at line 310. That restores the protocol check at the call site at zero runtime cost. Minimum viable alternative: annotate the local (writer: StreamCallback = params.pop("_writer")).
4. [Axis 4] Agent worker stays ptrace/memory-accessible to its own UID-2000 descendants (dumpable resets to 1 on exec), so the RPC nonce is recoverable from worker memory and the worker can be hijacked into emitting frames the root harness fully trusts — but the cited /proc/<worker_pid>/fd/0 read and /proc/<worker_pid>/fd/1 write route does NOT work (root-owned 0600 pipe inodes ⇒ EACCES) (src/coder_eval/isolation/agent_worker.py:416) — The whole point of this PR is a root-orchestrator / UID-2000-agent privilege boundary, and the RPC framing is the only authentication on the root-inbound side. _spawn gives the worker stdout=asyncio.subprocess.PIPE (line 381) and the root writes every request — nonce included — into the worker's stdin: request = {"nonce": self._nonce, "id": request_id, "method": method, "params": params} (line 480). After setpriv --reuid=agent, the worker execs a non-setuid python, so begin_new_exec resets dumpable to SUID_DUMP_USER and /proc/<worker_pid> is owned by 2000:2000 — the same UID as every tool subprocess the agent drives. An agent Bash command can therefore (a) read pending request bytes (containing "nonce":"<64 hex>") off /proc/<worker_pid>/fd/0 on any post-first-turn RPC (communicate in dialog mode, discard_pending_turn, stop), and (b) write forged newline-terminated frames to /proc/<worker_pid>/fd/1, which is the write end of the channel the root parses. The root then acts on those frames with full trust: it resolves the pending future (line 436-440), feeds EventCollector.on_event (line 429), runs _apply_snapshot (line 488/585) and TurnRecord.model_validate(result.get("record")) (line 586) — i.e. the agent can fabricate the scored trajectory that command_executed / skill_triggered / commands_efficiency grade against, and under-report token_usage.total_cost_usd to evade the max_usd gate. Even without the nonce, the unauthenticated fallback at line 416-418 (if not line.startswith(self._response_prefix): logger.info("[agent-worker] %s", line)) lets the agent inject arbitrary lines into the trusted task.log/log_tail, and a partial write with no trailing newline desyncs a legitimate frame so its response is silently logged instead of delivered (the pending future then hangs until the turn timeout). Concrete fix: call prctl(PR_SET_DUMPABLE, 0) in agent_worker_internal_command() before any agent is constructed — with dumpable=0 the kernel re-owns /proc/<worker_pid> as root:root and UID-2000 descendants can neither open its fds nor read its memory; additionally move the frame channel off fd 1 (a dedicated pass_fds socketpair) and compare the nonce with hmac.compare_digest instead of str.startswith. NOTE for the verifier: the base branch had an equivalent exposure (the agent could inject stream-json into the claude CLI's stdout, which the root SDK parsed), so this is a hardening gap in a NEW security control rather than a regression — hence medium, not high. CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:C/C:L/I:H/A:N (6.4)
5. [Axis 5] SAFE_CODER_EVAL_ENV allowlist is hand-picked, not consumer-derived: it exempts CODER_EVAL_IN_CONTAINER (no reader, contradicting the doc this PR edited) while the blanket CODER_EVAL prefix scrub strips CODER_EVAL_RAW_SDK_LOG, which the worker reads (src/coder_eval/isolation/agent_worker.py:52) — _SAFE_CODER_EVAL_ENV = frozenset({"CODER_EVAL_IN_CONTAINER"}) (line 52) is the sole exemption from the blanket prefix scrub at line 62 (if name in _SCRUB_ENV_VARS or (name.startswith("CODER_EVAL_") and name not in _SAFE_CODER_EVAL_ENV)). Both halves are wrong now that the whole agent lifecycle moved into the worker: (a) grep -rn IN_CONTAINER --include="*.py" src/coder_eval/ shows CODER_EVAL_IN_CONTAINER is only set (docker_runner.py:1330) and allowlisted here — no src/ reader consumes it (tests/test_codex_agent.py:108 even notes "_build_thread_options reads neither CODER_EVAL_IN_CONTAINER nor os.name"), so the allowlist entry is dead; (b) CODER_EVAL_RAW_SDK_LOG IS read agent-side, at src/coder_eval/agents/_logging.py:21 (_RAW_SDK_LOG_ENV = "CODER_EVAL_RAW_SDK_LOG"), which now executes inside the worker — so the knob CLAUDE.md documents as "Set CODER_EVAL_RAW_SDK_LOG=1 to dump every raw SDK event to the task log for inspection" silently does nothing under the new default (agent_isolation: true), exactly when a maintainer most needs it to debug a container run. Fix: derive the allowlist from real worker-side consumers — add CODER_EVAL_RAW_SDK_LOG and drop the unread CODER_EVAL_IN_CONTAINER (or keep it with a comment naming its non-Python consumer) — and add a test asserting the diagnostic knob survives build_agent_worker_environment(). This is the same dead-config class CE031 already guards for RunLimits/Dataset/SimulationConfig.
6. [Axis 8] The isolated agent worker configures no logging, so agent-side DEBUG diagnostics no longer reach task.log, and the scrubbed CODER_EVAL_RAW_SDK_LOG makes the raw-SDK dump unreachable under isolation (src/coder_eval/isolation/agent_worker.py:334) — task.log is written by an in-process logging.FileHandler(..., level=DEBUG) (logging_config.py:263-297), so before this PR every ClaudeCodeAgent/CodexAgent DEBUG/INFO record landed in it. The agent now lives in a separate process and agent_worker_internal_command (line 331-334) is just asyncio.run(_serve_worker()) — no setup_logging, no handler — so worker-side records fall to Python's lastResort handler (stderr, WARNING+). Only those WARNING+ lines survive, re-logged one level down by _read_stderr (line 461, logger.info("[agent-worker] %s", ...)). Every DEBUG/INFO diagnostic the agents emit (SDK stream progress, retry/route decisions, _log.debug calls) is now absent from task.log and therefore from the HTML report's log tail — the primary artifact for triaging a failed nightly task.
Compounding it, build_agent_worker_environment (line 62: if name in _SCRUB_ENV_VARS or (name.startswith("CODER_EVAL_") and name not in _SAFE_CODER_EVAL_ENV)) drops CODER_EVAL_RAW_SDK_LOG, which agents/_logging.py:21 reads inside the worker. The documented debug switch (CLAUDE.md: "Set CODER_EVAL_RAW_SDK_LOG=1 to dump every raw SDK event to the task log") is a silent no-op whenever isolation is on — i.e. by default on the docker driver.
Fix: call the harness logging setup at the top of agent_worker_internal_command writing to stderr at DEBUG (the proxy already forwards stderr), and add CODER_EVAL_RAW_SDK_LOG (plus CODER_EVAL_DEBUG) to _SAFE_CODER_EVAL_ENV — they are diagnostic switches, not harness paths or credentials.
Nits
- [Axis 1] _error_snapshot hand-duplicates _snapshot's payload shape (
src/coder_eval/isolation/agent_worker.py:143) —_error_snapshot's fallback re-derives the same four keys_snapshot(lines 131-140) already builds:
154: return {
155: "state": state.value,
156: "pending_turn": pending.model_dump(mode="json") ifpendingisnotNoneelseNone,
157: "sdk_options": None,
158: "environment": {},
159: }Only the last two keys differ from _snapshot, so a fifth field added to the snapshot contract silently vanishes from every error response. Collapse to one writer, e.g. have _snapshot take include_optional: bool = True (skipping the get_sdk_options() / get_environment_info() calls when False) and make _error_snapshot call it with False in its except branch.
2. [Axis 1] Agent username hardcoded as a literal twice while the exported AGENT_USERNAME constant sits unused (src/coder_eval/isolation/agent_worker.py:67) — build_agent_worker_environment imports AGENT_HOME from coder_eval.models but inlines the username next to it:
66: "HOME": AGENT_HOME,
67: "LOGNAME": "agent",
68: "USER": "agent",src/coder_eval/models/container_paths.py:30 defines AGENT_USERNAME = "agent" and models/__init__.py exports it; grep -rn AGENT_USERNAME src/ docker/ tests/ shows it has no consumers at all, so this PR added the first two sites that should have used it and used literals instead. Same pattern one line up: _SCRUB_ENV_VARS (line 53) took over the deleted utils.AGENT_ENV_SCRUB_VARS, but the companion prefix is now inlined as a bare string at line 62 (name.startswith("CODER_EVAL_")) rather than a named constant. Use AGENT_USERNAME for both env values and hoist the prefix into a module constant beside _SAFE_CODER_EVAL_ENV.
3. [Axis 1] Stream events are framed twice — the outer nonce frame wraps a complete inner wire line (src/coder_eval/isolation/agent_worker.py:202) — _WorkerWriter.on_event embeds a fully framed wire line as a JSON string inside the nonce frame — line 202: self.write({"kind": "event", "event": serialize_event(event)}) — and streaming/wire.py::serialize_event already prepends LINE_PREFIX = "\x1ecoder-eval-stream\x1e:". The host then strips it again at line 424 (deserialize_event(str(payload.get("event", "")))). Inside the outer \x1ecoder-eval-agent-rpc\x1e:<nonce>: frame the inner sentinel carries no information, and being a nested JSON string it gets escaped (�…) on every event. Split wire.py into an event_payload(event) -> dict / event_from_payload(dict) pair and embed the dict directly, keeping serialize_event/deserialize_event as the thin prefix wrappers for the stdout-line transport that actually needs them.
4. [Axis 4] Stop-flag tempdir hardcodes dir="/tmp" (bandit B108) with no nosec justification, and leaks on any path that skips communicate()'s finally (src/coder_eval/isolation/agent_worker.py:176) — Explicit disposition of the routed bandit Medium: the classic "insecure temp" reading of directory = Path(tempfile.mkdtemp(prefix="coder-eval-agent-stop-", dir="/tmp")) / directory.chmod(0o711) (lines 176-177) is a FALSE POSITIVE — mkdtemp is race-safe (mkdir with O_EXCL semantics, mode 0700, no symlink following), in-container /tmp is sticky 1777 so UID 2000 cannot unlink the root-owned dir, and 0o711 is deliberately correct for the root-writes / worker-stats handoff: the worker only needs stop_path.exists() (line 254), which requires traverse (x) but not read (r) or write (w), so the agent can neither create nor delete the stop flag. Two real residual nits: (1) dir="/tmp" is hardcoded rather than letting tempfile honour TMPDIR, and the world-traversable mode publishes the flag directory's existence to every process in the container — pass no dir= (or a root-only 0700 parent under /opt/coder-eval/) and chmod only the parent chain actually needed; (2) _remove_stop_path is only invoked from communicate's finally (line 600), so an orchestrator SIGKILL or a crash between _new_stop_path() (line 567) and entry to the try leaks the directory for the container's lifetime. Whichever way it is resolved, add # nosec B108 - root-created 0711 flag dir in a sticky container /tmp; mkdtemp is race-safe so this Medium stops being re-triaged on every bandit run (2 other findings in the tree already carry explicit nosec justifications). CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:U/C:L/I:N/A:N (2.5)
5. [Axis 5] New 663-line module carries both RPC sides plus protocol, env builder and kernel handshake, with redundant function-level imports obscuring its dependency graph (src/coder_eval/isolation/agent_worker.py:391) — agent_worker.py is one 663-line / 412-statement module holding the worker server (_WorkerServer, _serve_worker, agent_worker_internal_command), the root-side client (IsolatedAgentProxy, ~320 lines), the wire framing constants, the least-privilege env builder, and the /proc/self/status kernel handshake — radon flags four new C-grade blocks in it (_WorkerServer.handle C(16) @209, _read_stdout C(15) @410, _spawn C(13) @370, _serve_worker C(12) @277). Co-locating both protocol ends is a defensible SSOT choice, so this is a cohesion note rather than a defect; splitting the proxy from the server (with the shared frame constants in a small third module) would make each side independently readable. Concretely fixable now: line 391 from coder_eval.models import AGENT_GID, AGENT_UID duplicates the module-level from coder_eval.models import (...) already present at line 29, and line 371 from coder_eval.isolation.agent_identity import require_isolation_runtime defers an intra-package sibling import that cannot cycle — neither deferral is required by CE017, and both make the module look like it is working around an import cycle when it is not. Hoist both to module scope.
6. [Axis 7] agent_worker_internal_command follows the hidden-CLI-command naming convention but is a main hook, and the new public names are not in any all (src/coder_eval/isolation/agent_worker.py:331) — Every other *_internal_command in the tree is a hidden Typer command: src/coder_eval/cli/run_task_internal_command.py::run_task_internal_command is registered at cli/__init__.py:84 (app.command(name="_run-task-internal", hidden=True)(run_task_internal_command)). agent_worker_internal_command() is named identically but is never registered; the actual launch path is python -I -m coder_eval.isolation.agent_worker (line 379 and the Dockerfile sanity check at docker/Dockerfile:101), driven by the if __name__ == "__main__" block at line 662. Rename it to main() (or register it as a hidden command like its sibling) so the convention stays a reliable signal. Relatedly, this module adds three non-underscore names to a wheel that is going public — build_agent_worker_environment, IsolatedAgentProxy, agent_worker_internal_command — with no __all__, and src/coder_eval/isolation/__init__.py still declares __all__ = ["DockerRunner"], so orchestrator.py:1317 reaches past the package's curated surface (from coder_eval.isolation.agent_worker import IsolatedAgentProxy). Decide explicitly: either export IsolatedAgentProxy from the package __all__, or underscore-prefix build_agent_worker_environment to mark the module private.
What's Missing
Parallel paths:
- 🟠 The harness-env scrub was deleted wholesale but only re-implemented for the isolated worker.
utils.scrub_agent_env_overrides()/AGENT_ENV_SCRUB_VARSwere called UNCONDITIONALLY (not gated on isolation) by all three agents —claude_code_agent._build_sdk_env(base_env = scrub_agent_env_overrides()),codex_agent._build_codex_env(env = scrub_agent_env_overrides()), and antigravity's_harness_spawn_lockos.environ.popwindow — and their replacement,agent_worker.build_agent_worker_environment(), is reachable ONLY fromIsolatedAgentProxy._spawn(grep: 1 production call site). So host-driver runs,agent_isolation: falsedocker runs, and theagent_judgesub-agent now handSKILLS_REPO_PATH,TASK_DIR,CODER_EVAL_*andAWS_BEARER_TOKEN_BEDROCKstraight to the evaluated agent. The deleted comment justified the Bedrock entry specifically as preventing an inherited token from silently steering a DirectRoute run onto Bedrock (the CLI auto-selects onprocess.env.AWS_BEARER_TOKEN_BEDROCK) — that mis-routing risk is back for every non-containerized run, which is the common local/CI path. Neitherdocs/DOCKER_ISOLATION.md:288("Harness-only variables such asSKILLS_REPO_PATH,TASK_DIR, andCODER_EVAL_*are removed from agent SDK environments", still stated unconditionally) nor the two tests that covered it (rewritten intests/test_docker_identity_isolation.pyto assert onlybuild_agent_worker_environment()) was updated to the narrower reality. (trigger: src/coder_eval/utils.py) - 🟡
evaluation/sub_agent.py:202is the one remaining agent-construction path that does NOT go throughIsolatedAgentProxy— it instantiatesClaudeCodeAgentdirectly in the privileged process foragent_judge— and this PR removed its two protections in passing (cli_path=CONTAINER_CLAUDE_SHIM if agent_isolation_enabled()and the env scrub). The only thing keeping that safe isdocker_runner._validate_agent_isolation_compatibility's criterion denylist, whose own comment frames it as temporary ("until they have a separate grader sandbox"): whoever lifts that denylist now silently gets a judge agent running as root with the evaluator's full environment, where before it would have dropped to UID 2000 through the shim. Either giveSubAgentRunnerthe proxy/worker path too, or note in the denylist that it is now the sole guard. (trigger: src/coder_eval/agents/claude_code_agent.py) - 🔵 The new doc claim (
docs/DOCKER_ISOLATION.md:39, "applies equally to the built-in agents and to third-partyAgentRegistryplugins … there is no built-in-kind allowlist") is not matched by the remaining per-kind staging incli/run_task_internal_command.py:104-107, which stillgrant_agent_workspace(AGENT_HOME/.claude)for the claude-code state copy only. A third-party plugin agent that needs host-mounted state has no equivalent seam, so "kind-agnostic" holds for the launch boundary but not for state staging — say so, or generalize the grant. (trigger: docs/DOCKER_ISOLATION.md) - 🔵 The same Dockerfile hunk that deleted the claude wrapper also deleted
npm config set prefix /usr/localAND its verifying assertiontest "$(command -v claude)" = "/usr/local/bin/claude"(docker/Dockerfile:52-61). The claude binary therefore moves to the nodesource default prefix with nothing left pinning or checking its location. Nothing in-tree references the old path so this is currently benign, but the two removals are unrelated to the worker refactor and the image now has one less build-time invariant;docker/Dockerfile.runtimestill pins its own--prefix /opt/coder-eval/node, so the two images' claude locations are now derived by different mechanisms with no parity test (tests/test_image_from_dockerfiles.pyonly pinsCLAUDE_CODE_VERSION). (trigger: docker/Dockerfile)
Tests:
- 🟡 Two whole features now cross the new boundary with zero coverage, and neither is in the set Axis 3 enumerates. (a) Early stop: the cooperative
should_stopseam is re-implemented as a flag-file bridge (_new_stop_path()→stop_pathparam → worker'slambda: stop_path.exists()at agent_worker.py:254, published only from_publish_stop_flag_if_neededon event arrival), so everystop_early:arming — a documented, gate-affecting feature with its ownEarlyStopWatchertest suite — runs through untested machinery under the default docker config; nothing asserts a watcher decision actually reaches the worker, nor how the added event-arrival latency interacts withdecide_within's tool-call step counting. (b) Simulation/dialog mode reuses ONE worker across NcommunicateRPCs (agent state lives in_WorkerServer.agent); no test drives two turns against the same worker. (trigger: src/coder_eval/isolation/agent_worker.py) - 🟡
CONTAINER_DROP_SHIMis now the SOLE privilege-drop seam (agent_worker.py:375 spawns through it; the second constantCONTAINER_CLAUDE_SHIMwas deleted), yet no test pins that Python constant todocker/Dockerfile'sCOPY/chmod 0555destination — grep finds only two tests that read the script's contents. The repo already establishes exactly this pattern for the other container path (tests/test_image_from_dockerfiles.py::test_container_entrypoint_matches_dockerfile_copy_destination, plustest_runtime_kit_entrypoint_matches_host_path). Renaming or relocating the shim in the Dockerfile would now surface only as a runtime spawn failure inside a container. The Dockerfile's newpython -I -m coder_eval.isolation.agent_worker < /dev/nullsmoke covers the module path but not the shim path. (trigger: docker/Dockerfile) - 🟡 The
startpayload silently assumes three separate JSON round-trips work for every registry kind —config.model_dump(mode="json")→registration.config_class.model_validate,dataclasses.asdict(route)→route_type(**data), andconstructor_kwargs(LiteLLMcost_log_tags) — buttests/test_agent_worker.pyexercises them only against a bare_PluginConfigwith no fields and a hardcoded{"type": "DirectRoute", "data": {"judge_transport": None}}literal. Apytest.mark.parametrizeoverAgentRegistrykinds (claude-code / codex / antigravity / none) plustyping.get_args(ApiRoute)would cost nothing and would also close the route-codec exhaustiveness hole. (trigger: src/coder_eval/isolation/agent_worker.py)(restates: Axis 1: New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard) - 🟡 CI's only end-to-end exercise of the isolated worker is
tasks/byod_smoke_test.yaml(tagssmoke-pass,driver: docker, imageFROM coder-eval-agent:latestso it inheritsuid-gid-v1and isolation defaults on) — a single "Do nothing" turn on the happy path. Every failure path stays unexercised at every level:smoke_task_timeout(thesleep 300vstask_timeout: 30task that exists precisely to regression-test the watchdog hard kill, i.e. the path where the isolated proxy now losestoken_usage/messages/agent_output) runs on the HOST driver, andsmoke-fail'ssmoke_budget_exceeded/smoke_negative_pathlikewise. Give one timeout/crash smoke taskdriver: docker, or the newly-degraded kill path ships with neither unit nor e2e coverage. (trigger: src/coder_eval/orchestrator.py)(restates: Axis 8: Hard-killed isolated turns lose token/cost/transcript telemetry)
Downstream consumers:
- 🟡
_raise_remote_error's 7-entryexception_typesmap has three downstream consumers that the PR does not consider:errors/categorization.py(typed checks run FIRST, before the string patterns),errors/retry.py, and the report/telemetryerror_categoryrows. Its.get(error_type, AgentCrashError)default lands onErrorCategory.AGENT_CRASH— the one agent category marked retryable ("CLI crashes are often transient") — so any unmapped worker-side type is not merely relabelled but re-classified from NOT-retryable to retryable, re-running a doomed turn at full LLM cost._error_payloadserializestype(exc).__name__for any exception, and the map omits e.g.BudgetExceededError,EvaluationTimeoutError,TaskTimeoutError,JudgeInfrastructureError, plus theanthropic.RateLimitError/AuthenticationErrorclasses_categorize_by_exception_typeexplicitly handles for theagentcomponent (rate-limit's long backoff is lost; only the preserved message string still hints at the category). (trigger: src/coder_eval/isolation/agent_worker.py)(restates: Axis 1: New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard) - 🟡 The agent-authoring contract that third-party plugins (this PR's stated beneficiaries) are told to follow was not updated for the new process boundary. CLAUDE.md's "Adding a New Agent" steps 5-7 still describe an in-process agent, but an agent must now additionally: be constructible from a JSON-round-tripped config in a separate UID-2000 process, accept only JSON-serializable
constructor_kwargs, keep its ownsys.stdoutclean (fd 1 is the RPC frame channel — an unframedprint()from a plugin or its libraries is swallowed as a log line, and a mid-line interleave against_WorkerWriter's lock raisesmalformed agent-worker protocol lineand hard-kills the turn), and expose everything the root side needs through the four snapshot keys, since onlystate/pending_turn/sdk_options/environmentcross back. Note also thatIsolatedAgentProxy.communicateitself does NOT call the mandated_begin_turn()/_end_turn_ok()(it mirrors state via_apply_snapshot) — a deliberate deviation worth writing down rather than leaving as a counter-example. (trigger: src/coder_eval/isolation/agent_worker.py)
Display & mapping dicts:
- 🟡 Nothing records that a run used the isolated worker.
_record_route_environment_info(orchestrator.py:1206-1230) writesapi_routing,eval_routing,aws_region,judge_transport,litellm_*and merges the agent's ownget_environment_info(), but gains noagent_isolation/ worker-protocol key — so no report,run.jsonrow, or evalboard column distinguishes an isolated run from an in-process one. That is exactly the dimension a triager needs now that isolation changes what the artifacts contain (killed turns withtoken_usage=None, task.log missing agent DEBUG records,[agent-worker]stderr relays), and it is a one-line addition next to the route keys. (trigger: src/coder_eval/orchestrator.py)
Daily/nightly:
- 🟠 The PR states no blast radius for the production container path even though
docker.agent_isolationdefaults totrue, so from merge onward EVERY containerized nightly task drives its agent through a brand-new 663-line RPC layer measured at 28.6% coverage. Three consequences need writing into the PR body: (1) the image is a lockstep prerequisite — enforcement moved from the host (_validate_agent_isolation_compatibility) into the image (IsolatedAgentProxy) whileorg.coder-eval.agent-isolation="uid-gid-v1"is unchanged and_preflight_image_versiononly warns, so the window between merging anddocker-publish.ymlpushing:latestis a real host/image skew window; (2) nightly report/cost consumers (the external eval-runner and evalboard dashboards) can now see under-reportedtotal_token_usage/cost and empty transcripts on any task that hitstask_timeout; (3)task.log's agent-side DEBUG layer is gone for these runs, which is the artifact the nightly triage flow starts from. (trigger: src/coder_eval/isolation/docker_runner.py)(restates: Axis 8: Host-side agent-kind allowlist removed without bumping the uid-gid-v1 capability label)
Harness & Lint Improvements
This section is long enough that it pushed the comment past GitHub's 65,536-character limit — it is posted in full as the follow-up comment below (12 proposed static checks incl. CE035–CE045, plus 7 harness improvements).
Top 5 Priority Actions
- Preserve telemetry across hard kills: have
IsolatedAgentProxy.kill_syncSIGTERM the worker (not the whole session) with a short grace so the in-worker agent's cancel handler emits the terminalAgentEndEventbefore escalating tokillpg(SIGKILL)— today the fallback at src/coder_eval/isolation/agent_worker.py:588 yieldstoken_usage=None/agent_output="", so every isolated run that hitstask_timeoutunder-reports cost and can change anllm_judgescore for identical agent output; add a test asserting a killed isolated turn still carries non-Nonetoken_usage. - Make the isolation boundary fail closed on image skew: bump
LABEL org.coder-eval.agent-isolationtouid-gid-v2in docker/Dockerfile:110 and require the new value in_preflight_agent_isolation_image(src/coder_eval/isolation/docker_runner.py:288), because removing the host-side agent-kind allowlist at src/coder_eval/isolation/docker_runner.py:701 lets a stale-but-labeled image silently run a third-party plugin agent as root with full access to/opt/coder-eval/grader, and state themake docker-image/ghcr publish as a lockstep merge prerequisite. - Close the worker-hijack path that lets a graded agent fabricate its own trajectory: call
prctl(PR_SET_DUMPABLE, 0)inagent_worker_internal_command()(src/coder_eval/isolation/agent_worker.py:331) before any agent is constructed — in a default container a same-UID sibling can read the RPC nonce from/proc/<worker>/memandPTRACE_ATTACH, then emit frames the root harness trusts at src/coder_eval/isolation/agent_worker.py:416-440 to forge theTurnRecordthatcommand_executed/skill_triggered/commands_efficiencygrade and to suppresstotal_cost_usdpast themax_usdgate. - Give
_requesta per-method deadline (src/coder_eval/isolation/agent_worker.py:484), since thepinghandshake (line 390) and thestartRPC run before theThreadedWatchdogarmstask_timeoutat src/coder_eval/orchestrator.py:495 and no layer above puts a wall clock on them, so a live-but-mute worker — e.g. a third-party plugin blocking inensure_plugins_loaded()at line 224 — hangs the task, the batch and the nightly indefinitely; also stop suppressingOSErroron response writes (line 197) so the parent sees EOF instead of waiting on a future nothing resolves. - Build the missing behavioral tests for the proxy/worker boundary (src/coder_eval/isolation/agent_worker.py is 28.6% covered yet is now the production agent for every containerized run): extract the launch argv at lines 375-379 into an overridable seam so tests can drive the worker in-process against a registry test agent, then cover a normal turn round-trip, the
crashed=Truepartial handoff (lines 587-593),TurnTimeoutErrorfield preservation and both fail-closed gates, and replace the literal decode maps with derived ones — anApiRouteround-trip case in tests/test_route_seam_exhaustiveness.py plus an SSOT exception map, so the five unlisted in-tree error types stop collapsing toAgentCrashErrorat line 520.
Stats: 0 🔴 · 4 🟠 · 6 🟡 · 6 🔵 across 8 axes reviewed.
uipreliga
commented
Aug 11, 2026
Review: Harness & Lint Improvements (continued)Continuation of the review comment above — split out because the combined body exceeded GitHub's 65,536-character comment limit. Harness & Lint ImprovementsStatic checks (lint / type):
Harness improvements (not statically reachable):
|
Stack
This is PR 2, intentionally stacked on #87. Its base is
codex/uid-gid-agent-isolation, notmain.Please review this PR as the PR 2 delta, then merge it into #87's branch. Once this PR is merged, PR 3 will be created from the updated #87 stack.
Problem
#87 enforces the UID boundary through three SDK-specific launch seams and an allowlist of Claude, Codex, and Antigravity. That cannot cover an arbitrary
AgentRegistryplugin: the harness has no generic way to find and wrap whatever subprocess a plugin may create.What changed
AgentRegistry, constructs the selected agent, and owns its fullstart/communicate/stoplifecycle.NoNewPrivsis set.This covers built-ins and third-party registry plugins installed in the image without adding per-agent isolation code.
How it works now
DockerRunnerverifies that the image declares the UID/GID isolation capability and, for now, rejects dynamic grader types that are not yet safe.agent:agent(UID/GID 2000). Hidden task data, grader inputs, and result paths remain root-only.IsolatedAgentProxy.setprivlauncher. The worker checks its kernel identity before accepting work: all UID/GID slots must be 2000, supplementary groups must be empty, all capability sets must be zero, andNoNewPrivsmust be 1.AgentRegistry, constructs whichever registered agent was requested, and keeps that same instance alive for the complete evaluation lifecycle.start,communicate,discard_pending_turn, andstopcross the process boundary through a nonce-framed JSON protocol. Stream events flow back immediately; turn records, state, pending partial turns, SDK options, and environment metadata are synchronized in responses.The important change is the boundary location: isolation now wraps the generic
Agentinterface and registry construction, rather than trying to recognize and wrap a subprocess inside each built-in SDK.Out of scope
Dynamic graders (
agent_judge,run_command, anduipath_eval) remain rejected while isolation is enabled. Moving those graders out of the agent-written directory and into their own trusted execution boundary is PR 3.Validation
ruff check src tests: passedpyright: 0 errors; 1 pre-existing Antigravity warningsetprivlauncher: complete NoOp lifecycle succeeded with UID/GID 2000 in all four identity slots, no supplementary groups, all capability sets zero, andNoNewPrivs=1