Repository navigation
feat(docker): add network: llm_only with a separate grading allowlist - #239
bai-uipath wants to merge 24 commits into
Conversation
…kerDriverConfig The new mode and its append-merged allowlist validate and merge on every config layer; normalize_egress_target is the single entry parser. Harbor export refuses llm_only instead of widening it to public. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…_only A CONNECT + absolute-form HTTP proxy with an exact host:port allowlist, a probe subcommand, and a heartbeat watchdog that stops it when the host dies. It imports only five stdlib modules, so the host can bind-mount it into the framework image and run it with python3 -I. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
resolve_egress_targets unions the backend hosts, forwarded *_URL hosts, the agent config's new egress_hosts() hook, system_one_judge base URLs and the user allowlist. DockerRunError moves to isolation/errors.py and the loopback rewrite to isolation/egress.py so egress.py has no cycle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sidecar DockerRunner.run wraps the task container in egress_scope: an --internal network with inhibit_ipv4, a locked-down proxy sidecar from the framework image running the host's bind-mounted egress_proxy.py, a CONNECT probe of every target, and teardown on every path. The bridge and none argv is pinned byte for byte. The grading consent text names the mode. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ess task New Network modes section in DOCKER_ISOLATION, a per-harness network table in HARNESS_PARITY, the design rationale in .claude/notes, and tasks/docker_egress_llm_only.yaml, which fails if general internet is reachable from the container. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Give the llm_only task container a black-hole upstream DNS server so an unpatched daemon (CVE-2024-29018) cannot tunnel DNS out, create the internal network with --ipv6=false, never forward ALL_PROXY, extend the live test to assert DNS, direct-route and host-alias bypasses fail, and correct the docs (judge api_route hosts, CDN fronting, Docker versions). Defer four harness candidates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The backend's hosts are now added only for a component that reaches its model through API_BACKEND (a claude-code agent, an enabled simulator, an llm_judge or agent_judge criterion), and a forwarded URL variable counts only where its owner reads it (LITELLM_BASE_URL for the litellm backend, CODEX_BASE_URL for codex). Real runs had shown a Claude task reaching the Azure Codex host and Codex/Antigravity/Pi tasks reaching api.anthropic.com. Also stops a bad LITELLM_BASE_URL loopback port leaking its value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Derive judge and simulator hosts from resolve_route / resolve_evaluation_route, so a LiteLLM run allows the pinned Claude backend and a route: litellm judge allows its own api_base. - Write the sidecar log to its own egress.log with write_text_atomic: a symlink the agent plants in the run dir is replaced, never followed, and container stdout cannot forge ALLOW/DENY lines. Grading folds it back as grade.egress.log. - Refuse llm_only at host dispatch when the driver is not docker. - Redact refused request lines, cap the proxy at 256 connections, drop NET_RAW from the task container, wrap egress staging errors as EgressSetupError, and stop reporting a never-created network as leaked. - Make the loopback helpers public and keep one LOOPBACK_HOSTS; document the plugin-agent hooks in EXTENDING.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The agent tries to reach the internet in its own ways, and the grading criteria probe each path out from inside the container (proxy, direct IPv4/IPv6, DNS, raw sockets, the docker host, pip, git, npm) while the model host must stay reachable. Under network: bridge the same task fails, which is the control. Also documents the WebFetch domain-check DENY on Bedrock and that provider-side tools such as WebSearch are outside the container boundary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The model-host probe skipped the litellm backend, so it passed without checking anything there. It now reads LITELLM_BASE_URL and requires that exact host and port through the proxy, and another port of it to be denied. Also lists the api.anthropic.com DENY lines that Claude Code makes on the litellm route. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PYSEC-2026-4175/4177 (urllib3 2.7.0) and PYSEC-2026-4011/4013 (virtualenv 21.2.0) fail the Quality Gate audit on every branch. Pins the oldest fixed release of each, to stay clear of the runner's minimum package age; python-discovery follows virtualenv to 1.6.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CodeQL cannot model pytest.raises or contextlib.suppress, so it reported the awaited task inside them as an ineffectual statement. Use an explicit try/except (the pattern in test_reference_permissions.py) and asyncio.gather(..., return_exceptions=True) for cleanup, give the live fixture one explicit return, and compare the full Bedrock target list instead of a substring-like membership check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…al credentials Since the allowlist uses the real route resolvers, a Bedrock backend without a token resolves no route, so the bad AWS_REGION was never reached in CI (no token there) while it passed on a machine whose .env has one. The test now sets its own token. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Table-driven allowlist cases, no repeated argv assertions, merged proxy parser cases, and drop the docker-daemon live test that tasks/docker_egress_probe covers end to end. Same behaviours checked. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
OpenCode honours the proxy when its CLI is in the image, and its catalog and update checks are harmless DENY lines. Pi and OpenCode were verified on Bedrock and Azure gpt-5.6-luna, with the provider hosts in egress_allowlist. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ests CodeQL py/side-effect-in-assert: a dict pop and an awaited cancel ran inside assert statements, which python -O would skip. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Drop tasks/docker_egress_llm_only.yaml: tasks/docker_egress_probe makes the same checks and many more. - Drop the ALLOW_IMAGE_SKEW fallback of the sidecar to :latest; it lost its only test and the error already says to run make docker-image. - Drop two overrides-engine tests that main already covers (a -D docker key keeps its siblings; allowlist append is in the merge tests). - HARNESS_PARITY links to the derived-allowlist and expected-DENY tables instead of repeating them; the Codex ChatGPT-login note moves there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…m_only A host in egress_allowlist is open to the agent as well, so a grader that installs its own test dependencies forced those hosts open for the agent too. Add sandbox.docker.grading_egress_allowlist (hosts for the criteria only) and grading_network: bridge (the criteria reach any host). The proxy sidecar switches once, when grading starts, in the same container: the in-container run asks the host through a request file, the host flips the sidecar through a control port on the sidecar's own loopback, and the criteria run after the proxy confirms. A detached grade starts with the grading policy, execute keeps the agent's, and a task with an enabled simulation is refused. Grading-only hosts are checked at setup with a direct TCP probe from the sidecar, and tasks/docker_egress_probe/docker_egress_grading.yaml checks the switch end to end with no agent or model API. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
filelock 3.32.6 has three new advisories (fixed in 4.2.1), which fail the pip-audit step of the quality gate on main as well. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
osv-scanner flags GHSA-cx6f-vjhg-g3cc in virtualenv 21.14.12, which fails the quality gate. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…during grading A host SIGTERM or SIGKILL skips the egress teardown, so each killed llm_only task leaked a stopped sidecar and an internal network, and each network holds one Docker address pool. Every egress setup now first prunes egress-labelled stopped containers and unused networks older than 10 minutes. Prune never touches a running container or a network in use, so concurrent runs keep theirs. A grader refused a host it needs (an apt mirror, PyPI) scored the row as an ordinary FAILURE, which read as an agent miss. Every graded llm_only run now marks the grading phase in the proxy log, also with no grading allowlist (that switch adds no host and is best-effort). After the run, the host reads the sidecar log from Docker, records the hosts refused during grading in environment_info.egress_denied_in_grading, and turns a FAILURE into an ERROR that names them. Hosts the judge CLIs call and do not need are ignored. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…arness The model_host checks read only API_BACKEND, so on Codex, Antigravity or Pi with a non-Bedrock provider they asked the proxy for the Bedrock host, got 403 and failed although the agent's own model host was allowed. The probe now tries every model host the container's variables name (backend, CODEX_BASE_URL or api.openai.com, Gemini, OpenRouter) and checks the first one the proxy allows. The parity table records a probe run per harness. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Claude finished @bai-uipath's task in 1m 36s —— View job Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: feat(docker): add network: llm_only with a separate grading allowlist
PR #239 by @bai-uipath · bai/network-policy → main · OPEN · reviewed against 156dcba · workflow variant
Change class: complex — adds a new egress-proxy subsystem (network: llm_only), new container networking/teardown control flow, a separate grading allowlist, and new public schema fields
The new network: llm_only grading-egress feature is well built (Security 9.4, API Surface 9.9, clean agent SPI, tested proxy and allowlist parsing), but the host-side verdict rewrite and the grading handshake can change final_status for identical agent output: a criterion's own DENY lines, a stale handshake file on --resume, or a failed watch task can turn a FAILURE into an ERROR that evaluate cannot re-grade, and the key wiring has no test (Test Health 7.2), so fix the classifier and the handshake lifecycle before merge.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 8.6 / 10 | 0 | 1 | 0 | 4 | grading_network: llm_only behaves the same as None, duplicates grading_egress_allowlist, and its description contradicts the code |
| 2. Type Safety | 9.2 / 10 | 0 | 0 | 1 | 3 | Plugin SPI egress_hosts() output reaches the sidecar --allow flag without normalization; a bare-host or URL-form entry from a third-party agent kills the proxy, and the setup error does not name the entry |
| 3. Test Health | 7.2 / 10 | 0 | 2 | 1 | 3 | The in-container before_grading branch (run_task_internal_command.py:281-284) has no test. Only the default network: bridge path runs. |
| 4. Security | 9.4 / 10 | 0 | 0 | 1 | 1 | Agent can trigger the grading egress switch early by creating the fixed-name request file in the writable run dir (documented accepted risk; Low) |
| 5. Architecture & Design | 7.7 / 10 | 0 | 1 | 2 | 3 | _record_grading_denials rewrites final_status/task.json ad hoc outside the terminal-status chain: the ERROR cannot be re-graded, task.html goes stale, transcript exclude is skipped |
| 6. Error Handling & Resilience | 7.8 / 10 | 0 | 1 | 2 | 2 | A failed grading watch re-raises from the inner finally of run() (docker_runner.py:755-763). This masks the original error, skips the log close and _kill_container, and on a normal exit discards task.json. |
| 7. API Surface & Maintainability | 9.9 / 10 | 0 | 0 | 0 | 1 | Pi egress_hosts silently allowlists openrouter.ai for an unrecognised provider prefix (e.g. Bedrock), and does not warn the way the LiteLLM path does |
| 8. Evaluation Harness Quality | 7.4 / 10 | 0 | 2 | 1 | 1 | Grading-denial classifier turns FAILURE into ERROR on any DENY after PHASE grading, including DENYs that criteria cause on purpose, so final_status changes for identical agent output |
Overall Score: 8.4 / 10 · Weakest Axis: Test Health at 7.2 / 10
Totals: 🔴 0 · 🟠 7 · 🟡 8 · 🔵 18 across 8 axes.
Blockers
- [Axis 1]
grading_network: llm_onlybehaves the same as None, duplicatesgrading_egress_allowlist, and its description contradicts the code (src/coder_eval/models/sandbox.py:270) —grading_network: Literal["bridge", "llm_only"] | None(L270) has three states, but no code tells"llm_only"apart fromNone. Every consumer tests only for== "bridge":grading_widens_egress(L396self.grading_network == "bridge" or bool(self.grading_egress_allowlist)),DockerRunner._container_network(docker_runner.py:789),_resolve_egress_plan(docker_runner.py:811grading_any = cfg.grading_network == "bridge") and regrade.py:289 (docker.grading_network != "bridge"). The only effect of"llm_only"is in the validator. The field description (L274, "'llm_only': the agent's allowlist plusgrading_egress_allowlist") describes the same behaviour as the default. The docs table in DOCKER_ISOLATION.md § Grading network does not list it at all. The validator also has a special case that accepts a combination that does nothing (L386if self.network == "bridge" and self.grading_network == "bridge": return self). The second field also slips past the validator:grading_egress_allowlistis ignored without an error undernetwork: bridge/noneand undergrading_network: bridge. DOCKER_ISOLATION.md:162 says "Both fields neednetwork: llm_only... any other combination is refused when the task loads", so the docs and the code disagree. Fix: remove"llm_only"from the Literal, or better, merge the two knobs into one field, for examplegrading_egress_allowlist: list[str] | Literal["any"]. Then "grading reaches any host" and "grading adds these hosts" are one setting with one validator, and reject that field unlessnetwork == "llm_only". The project is greenfield, so the removal needs no back-compat. Cross-axis: 7 (knob surface and docs mismatch). - [Axis 3] The in-container before_grading branch (run_task_internal_command.py:281-284) has no test. Only the default network: bridge path runs. (
src/coder_eval/cli/run_task_internal_command.py:280) — Lines 280-284 are the only link between the container and the host grading switch, and no test reaches them. No test calls run_task_internal_command (tests/test_run_task_internal.py tests only _scrub_staged_inputs), and the routed coverage lists 202-286 as missed. The code is:
docker = ctx.authored_sandbox.docker
if ctx.grade and docker.network == "llm_only":
orchestrator.before_grading = functools.partial(
request_grading_egress, output_dir, widens=docker.grading_widens_egress
)
If this regresses, the container never writes the request file. It can read ctx.task.sandbox (forced to tempdir in the container) instead of authored_sandbox, invert ctx.grade, or drop widens. Then _serve_grading_request waits forever, the sidecar never switches, and the proxy log has no PHASE grading line. grading_denials(..., whole_log=False) then returns [] (egress.py:476-478), so a grader that could not reach PyPI is recorded as a plain agent FAILURE. That is the misattribution this feature exists to stop, and no test catches it. Fix: move this into a small pure helper, for example _grading_hook(ctx, output_dir) -> Callable | None, and add a parametrized test. Graded llm_only gives a partial with widens == authored_sandbox.docker.grading_widens_egress. Execute (grade=False) gives None. bridge/none give None. A ctx whose forced task.sandbox differs from authored_sandbox still uses the authored one.
3. [Axis 3] The documented failed-switch-gives-ERROR contract and the post-failure grading site have no tests (src/coder_eval/orchestrator.py:2221) — docs/DOCKER_ISOLATION.md:170 says "If it fails, the row is an ERROR that names the failure". The orchestrator awaits the hook at two sites: line 2221 await self._start_grading() on the single-shot path, and line 863 await self._start_grading() in _evaluate_post_failure_criteria. The only test, tests/test_egress_grading_phase.py:72-95 test_the_hook_runs_once_before_the_criteria, covers the success path at site 2221 only. No test makes before_grading raise and checks that final_status == ERROR, that error_message names the switch failure and that no criterion ran. No test reaches site 863 at all. On that path a raising hook should go to _record_post_failure_not_evaluated and keep the original AgentCrashError/TurnTimeoutError (orchestrator.py:785-791). A refactor that moves _start_grading() out of the try that run() catches (line 615 except Exception), or after check_all_async, would ship green. Add three tests: (1) a hook that raises RuntimeError gives an ERROR row with the message and no criterion results; (2) an agent crash plus an evaluable_after_agent_failure criterion calls the hook once, before that criterion; (3) a hook that raises on the post-failure path records not_evaluated and keeps the original terminal error.
4. [Axis 5] _record_grading_denials rewrites final_status/task.json ad hoc outside the terminal-status chain: the ERROR cannot be re-graded, task.html goes stale, transcript exclude is skipped (src/coder_eval/isolation/docker_runner.py:830) — _record_grading_denials (L817-839) changes the verdict after the in-container Orchestrator has already finalized the row: if result.final_status is FinalStatus.FAILURE: result.final_status = FinalStatus.ERROR (L829-830). It then writes task.json again with its own serializer, write_text_atomic, task_json, result.model_dump_json(indent=2) (L837). The ERROR it writes is an execution fact (_EXECUTION_FACT_STATUSES[ERROR] = True, models/enums.py). That breaks three things. (1) The fix in error_message does not work. The message says to "Add each host to sandbox.docker.grading_egress_allowlist and grade the run again (coder-eval evaluate <run_dir>)". But evaluate loads the prior from task.json (load_prior_result, regrade.py L60), and Orchestrator._terminal_status (orchestrator.py L464-470) returns an inherited execution fact unchanged. evaluate_command.py L591 then exits 1 with "grading cannot overturn that". partition_for_resume (batch.py L431) treats ERROR as complete, so run --resume skips the row too. The only way out is to run the agent again. (2) The in-container write_task_html (orchestrator.py L1129) has already written task.html with FAILURE, so task.html and task.json disagree. (3) This is a third task.json writer, and it leaves out the canonical exclude=TASK_JSON_TRANSCRIPT_EXCLUDE that orchestrator.py L1121 and evaluate_command.py L629 apply. Fix: keep the verdict inside the Orchestrator. One option is to pass the denied hosts back in as a fact (for example on the ack or through a post-grading hook) and let _terminal_status or _finalize_result choose ERROR. Another is to give this ERROR a distinct marker that is_execution_fact and _owes_a_grade treat as re-gradeable. In both cases, write task.json and task.html through one shared serializer. Also add a test that runs evaluate on a denial-ERROR row and checks that adding the host gives SUCCESS or FAILURE. Cross-axis: flagged by axes 5, 6 and 8 (status classifier).
5. [Axis 6] A failed grading watch re-raises from the inner finally of run() (docker_runner.py:755-763). This masks the original error, skips the log close and _kill_container, and on a normal exit discards task.json. (src/coder_eval/isolation/docker_runner.py:758) — In the inner finally (lines 755-763), the code awaits the grading-watch task before it does the real cleanup:
if grading_watch is not None:
grading_watch.cancel()
with contextlib.suppress(asyncio.CancelledError):
await grading_watch
await asyncio.to_thread(_remove_grading_handshake, output_dir)
await asyncio.to_thread(log_fh.close)
if proc.returncode is None:
await self._kill_container(proc, container_name)
cancel() does nothing to a task that has already finished, so await grading_watch re-raises whatever exception that task ended with. _serve_grading_request catches only EgressSetupError. Its final await asyncio.to_thread(write_text_atomic, output_dir / EGRESS_GRADING_ACK_FILENAME, reply) (line 201) can raise OSError: disk full, or a directory at the ack path. The container can write to the run dir, so it can create that directory. _remove_grading_handshake (line 759) can also raise, for example an unlink of a directory. When either raises: (a) log_fh.close() and _kill_container never run, so a run cancelled by Ctrl-C or the task timeout leaves the container running and using budget. This is the exact hazard the # HAZARD: docker run --rm does NOT propagate a kill comment guards against. (b) The OSError replaces the in-flight CancelledError or other exception. (c) On a normal exit, _parse_result_or_raise is skipped, so the container's real task.json is discarded and the row becomes a batch-layer ERROR. I reproduced the re-raise in a scratch script: a done task that raised OSError, then cancel(), then await inside suppress(CancelledError), raises OSError out of the finally. Fix: make _serve_grading_request total. Catch OSError around the ack write and log a warning. In the finally, kill the container and close the log first. Then collect the watch result through try/except Exception: logger.warning(...), so a diagnostic helper can never mask the original error or skip a cleanup step.
6. [Axis 8] Grading-denial classifier turns FAILURE into ERROR on any DENY after PHASE grading, including DENYs that criteria cause on purpose, so final_status changes for identical agent output (src/coder_eval/isolation/docker_runner.py:829) — _record_grading_denials (docker_runner.py:817-838) does if result.final_status is FinalStatus.FAILURE: result.final_status = FinalStatus.ERROR whenever grading_denials() (egress.py:468-481) returns a host. That function takes every DENY line after PHASE grading, less a hard-coded GRADING_DENIAL_NOISE = frozenset({"http-intake.logs.us5.datadoghq.com:443", "api.anthropic.com:443"}) (egress.py:214). Every graded llm_only run writes the PHASE line, so the rule also applies to tasks that have no grading allowlist. Three failure scenarios: (1) The in-tree probe task tasks/docker_egress_probe/docker_egress_probe.yaml runs egress_probe.py blocked_proxy_https ... as criteria, and these open CONNECT example.com:443 on purpose (egress_probe.py:104). The same happens with model_host_other_port_blocked, whose port is not :443 and so is not in the noise set. Any FAILURE of that task, which includes a real leak where a blocked_* probe fails, becomes ERROR with the message "Add example.com:443 to grading_egress_allowlist". The boundary's own regression check thus reports a leak as a grader infrastructure error and gives the wrong fix. The same applies to every no-internet task (the issue #168 use case) whose criteria check that a host is blocked. (2) An agent_judge is an LLM agent. It can use a network tool on some runs and not on others, so the same trajectory can give FAILURE on one run and ERROR on the next. (3) The noise set depends on the Claude Code CLI version. If a CLI release changes its telemetry host, every agent_judge FAILURE under llm_only becomes ERROR. ERROR also changes tasks_error, the worst-status fold across replicates and the category that the coder-eval-uipath dashboards consume. Fix: report the denials in environment_info and in a warning, and keep FAILURE. If a reclassification is necessary, make it opt-in and limit it to hosts that a criterion declares it needs. Do not apply it to every DENY in the phase. At least exclude the hosts that the criteria probe on purpose, and add a test that runs the probe task's log shape through grading_denials and checks that a FAILURE stays a FAILURE. n/a
7. [Axis 8] Grading handshake files are removed only in a finally after the run, so --resume in the same run_dir switches to the grading allowlist before the agent runs (src/coder_eval/isolation/docker_runner.py:191) — _serve_grading_request starts polling as soon as the container starts (while not await asyncio.to_thread(request.exists):, docker_runner.py:191, created at :744-748) and calls enter_grading the first time it finds .coder_eval_egress_grading_request in output_dir = self.rt.run_dir.resolve(). The only cleanup is _remove_grading_handshake in the inner finally (docker_runner.py:759). Nothing deletes the request or ack file before the container starts. The in-container request_grading_egress (run_task_internal_command.py:72-74) also accepts an ack file that already exists. The isolation notes added by this PR say that a host SIGKILL or SIGTERM (a CI cancel, timeout, systemd) skips every finally. If that happens during grading, both files stay in the task's run_dir. clear_rerun_artifacts (orchestration/batch.py:459-483) clears only the artifacts dir, so run --resume runs the task again in the same run_dir. The host then switches the sidecar to grading (the grading allowlist, or any host with grading_network: bridge) as soon as the agent starts. The agent gets the hosts that the feature exists to keep from it, for example github.com, where benchmark solutions are published. The PHASE line also goes to the start of the log, so every agent-phase DENY counts as a grading denial and a FAILURE becomes ERROR. This is the stale-artifact-on-resume class. Fix: call _remove_grading_handshake(output_dir) before docker run and before the watch task starts, not only in the finally block. Also make the container delete a stale ack before it writes its request. Add a test that puts both files in run_dir before run() and checks that enter_grading is not called until the container makes a new request. n/a
Non-blocking, but please consider before merge
- [Axis 2] Plugin SPI egress_hosts() output reaches the sidecar --allow flag without normalization; a bare-host or URL-form entry from a third-party agent kills the proxy, and the setup error does not name the entry (
src/coder_eval/isolation/egress.py:168) —resolve_egress_targetstrusts the hook's output as-is:targets.update(task.agent.egress_hosts(forwarded))(egress.py:168). The hook is a public SPI typeddef egress_hosts(self, env: Mapping[str, str]) -> tuple[str, ...](agent_config.py:225). It says "Normalizedhost:porttargets" only in its docstring, and docs/EXTENDING.md:86 just says "return thehost:porttargets".egress_allowlistaccepts a bare host such as 'pypi.org' and normalizes it to port 443 (sandbox.py_normalize_egress_allowlist), so a plugin author can easily return 'api.example.com' or 'https://api.example.com'. In the sidecar,_parse_allowcallssplit_host_port(entry, None), which returns None for a port-less entry, so the proxy logsBAD --allowand exits 2 (egress_proxy.py:437-440, 466-469). The host then sees only thedocker execprobe fail against a stopped container, and that error does not name the bad entry. Fix: pass each hook result throughnormalize_egress_targetat the seam and wrap a ValueError so it names the agent kind, e.g.targets.update(normalize_egress_target(h) for h in task.agent.egress_hosts(forwarded)). Optionally add aNewType('EgressTarget', str)return type so the contract shows in the type and not only the docstring. Add a test with a plugin config that returns a bare host. - [Axis 3] The llm_only runtime path of DockerRunner.run() and the network boundary have no executing (daemon-backed) test (
src/coder_eval/isolation/docker_runner.py:744) — The only run() test, tests/test_docker_egress_runner.py:395with pytest.raises(FileNotFoundError): await _rt_runner(tmp_path, "llm_only").run(), raises atasyncio.create_subprocess_exec. None of lines 744-767 run: thegrading_watch = (asyncio.create_task(_serve_grading_request(output_dir, egress)) if egress is not None and egress.plan.grading_phase else None)creation, its cancel and_remove_grading_handshakein thefinally(755-759), and the scoring call siteif egress is not None and self.grade: await self._record_grading_denials(...)(766-767)._record_grading_denialsand_serve_grading_requesthave only direct unit tests. If the gate is dropped, inverted, or moved before_parse_result_or_raise, the FAILURE-to-ERROR reclassification stops or fires in execute mode, and no test fails. Add a run() test with a fake process that writes a valid task.json (contract echoed) and a fakeegress_scopewhosedocker logsreturns a log withPHASE gradingand a DENY. Check that (a) a FAILURE becomes ERROR whengrade=True, (b) execute mode leaves it unchanged, (c) the request and ack files are gone after the run, and (d) the watch task is cancelled on a cancel during streaming. - [Axis 4] Agent can trigger the grading egress switch early by creating the fixed-name request file in the writable run dir (documented accepted risk; Low) (
src/coder_eval/isolation/docker_runner.py:191) — The host polls the writable run directory and widens egress as soon as the file exists. It does not check who wrote the file or when: docker_runner.py:190-194request = output_dir / EGRESS_GRADING_REQUEST_FILENAME/while not await asyncio.to_thread(request.exists):/await enter_grading(egress). The watcher starts at line 745, before the agent runs. output_dir is bind-mounted read-write into the task container (line 708grant_container_access(output_dir, writable=True), line 1569-v {output_dir}:{CONTAINER_OUTPUT_DIR}). The file name is a fixed constant (path_utils.py:48EGRESS_GRADING_REQUEST_FILENAME = ".coder_eval_egress_grading_request"), and the coder_eval package installed in the task image contains it. The legitimate writer is run_task_internal_command.py:72await asyncio.to_thread(write_text_atomic, output_dir / EGRESS_GRADING_REQUEST_FILENAME, "grading"). Onetouchfrom the agent before its turn ends therefore gives it the grading_egress_allowlist for the rest of the run, or any host when grading_network: bridge sets--grading-allow-any. With any host open, the agent can fetch benchmark solutions (for example from github.com, which .claude/notes/isolation.md names as the reason grading hosts are split off) and can send the container's forwarded API keys and the ~/.claude OAuth copy to any host. docs/DOCKER_ISOLATION.md ('The grading switch trusts the container') and the notes document this as accepted. It stays Medium, not Low, because a cheap defense-in-depth measure exists. Fix: have the host generate a per-run random nonce and put it only in the staged context.json, which_scrub_staged_inputsalready deletes after load. The in-container orchestrator writes the nonce into the request file, and_serve_grading_requestrefuses any request whose content does not match. This turns atouchinto a memory-scrape of the orchestrator process. Also record in environment_info whether the switch came before the agent-phase end event in the container's stream, so a premature switch can be seen after the run. CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:C/C:L/I:L/A:N - [Axis 5] Egress hosts come from isinstance checks on core criterion classes, not a per-criterion hook, and a disabled system_one_judge still opens its host (
src/coder_eval/isolation/egress.py:132) — Agents get a clean SPI in this PR:uses_api_backend: ClassVar[bool]andegress_hosts(env)onBaseAgentConfig(models/agent_config.py), documented in docs/EXTENDING.md. Criteria do not. The isolation layer hardcodes which criteria call a model:if any(isinstance(c, LLMJudgeCriterion | AgentJudgeCriterion) and c.enabled for c in task.success_criteria):(L132) andif isinstance(criterion, SystemOneJudgeCriterion):(L170). The two branches also disagree: L170 does not checkc.enabled, so a disabled system_one_judge still opens its host. A plugin-registered criterion that callsresolve_evaluation_routeor any external host is not visible to the allowlist. EXTENDING.md documents no hook for criteria, so isolation/egress.py must change for each new model-calling criterion type. This is pattern drift against the criterion registry. Fix: give the criterion model base the same two members as the agent config (uses_api_backend: ClassVar[bool] = Falseandegress_hosts(env) -> tuple[str, ...]). Override them on LLMJudge, AgentJudge and SystemOneJudge, and makeresolve_egress_targetsiterate polymorphically. Then document the hook beside the agent one in EXTENDING.md. - [Axis 5] Egress-phase logic raises DockerRunner complexity (run() A->C, _build_argv D(27)) (
src/coder_eval/isolation/docker_runner.py:641) — radon on origin/main givesDockerRunner.run - A (5)(L612) and_build_argv - D (24). At PR HEAD it givesDockerRunner.run - C (15)(L641) andDockerRunner._build_argv - D (27)(L1470). The file grows from 1516 to 1655 lines. The PR moved the pure egress pieces into isolation/egress.py, but it left the egress policy and lifecycle in the runner. That covers_resolve_egress_plan(L793),_container_network(L786),_record_grading_denials(L817), the grading handshake server_serve_grading_request,_remove_grading_handshake,_prepare_egress_dirand_network_name, plus a nestedasync with scope/ grading_watch / heartbeattry/finallyinside run(). Fix: move plan resolution and the handshake into egress.py, behind one context manager such asegress_session(rt, grade, grade_workspace, output_dir)that yields the handle and owns the grading watcher and its cleanup. run() then has a singleasync with. Fix the status rewrite with the previous finding. - [Axis 6] _run_to_completion loses the caller's cancel when the work it shields fails after the cancel (
src/coder_eval/isolation/egress.py:537) — The docstring says the helper re-raises the cancel at the end ("then re-raise the cancel"). The loop does not do that when the shielded task raises:
while not task.done():
try:
await asyncio.shield(task)
except asyncio.CancelledError:
cancelled = True
if cancelled:
if not task.cancelled() and task.exception() is not None:
logger.warning(...)
raise asyncio.CancelledError
After a cancel, the loop awaits asyncio.shield(task) again. If the worker thread then raises, for example subprocess.TimeoutExpired from the 30 s _docker_sync timeout, that exception leaves the loop and the if cancelled: branch never runs. Its warning path is in practice dead code. I reproduced this in a scratch script: a caller cancelled 50 ms into a 300 ms thread that raises OSError receives OSError, not CancelledError. In _checked, the exception then becomes EgressSetupError, and run()'s except EgressSetupError writes a synthetic ERROR task.json for a task the user or the task-timeout watchdog cancelled. A cancelled task is recorded as an egress setup failure. Fix: wrap the shield await in try/except asyncio.CancelledError: cancelled = True; continue and except BaseException: if cancelled: break; raise, then run the existing if cancelled: block, so the cancel always wins.
7. [Axis 6] The rule that turns a FAILURE graded without its hosts into an ERROR fails open with no warning when docker logs fails (src/coder_eval/isolation/egress.py:465) — read_proxy_log returns None when docker logs exits non-zero: return result.stdout if result is not None and result.returncode == 0 else None. _best_effort warns only when an exception is raised, not on a non-zero exit. _record_grading_denials (docker_runner.py:822-825) then does denied = [] if proxy_log is None else grading_denials(...), followed by if not denied: return. When the log cannot be read, the documented rule ("make a FAILURE graded without them an ERROR") is skipped and nothing is logged. A judge that was refused its host then stays a scored agent FAILURE. Both readings are now silent: "no hosts were refused" and "we could not check". Fix: in read_proxy_log or _record_grading_denials, log a warning that names the sidecar, the return code and the stderr when the log is unreadable. Also record that in environment_info (for example egress_denials_unknown: true), so the row says that the check did not run.
8. [Axis 8] grading_egress_allowlist / egress_allowlist are silently accepted and ignored outside network: llm_only, though docs say such combinations are refused at load (src/coder_eval/models/sandbox.py:384) — _check_grading_network checks only grading_network: if self.grading_network is None or self.network == "llm_only": return self (sandbox.py:384). A task with network: bridge or network: none and grading_egress_allowlist: [pypi.org] loads without an error, and the field description says it is "Ignored with grading_network: bridge or another network mode" (sandbox.py:286). docs/DOCKER_ISOLATION.md:162-163 says "Both fields need network: llm_only. ... any other combination is refused when the task loads". With network: none the author expects grading to reach pypi.org, but it cannot, and with no sidecar there is no denial report. The grader fails and the row is scored as an agent FAILURE. Technique 5 (inverse guard): refuse a non-empty grading_egress_allowlist when network != "llm_only", as is done for grading_network, or change the guide and the field description so that they agree. Add a test for the refused combination. n/a
What's Missing
Parallel paths:
- 🔵 The dialog grading site
_run_dialog_criteria_check(orchestrator.py:2304) does not call_start_grading(), but the single-shot (L2221) and post-failure (L863) sites do. run_task_internal_command.py installsbefore_gradingfor every graded llm_only run, so a simulated llm_only row never writesPHASE grading, and its grading denials are not reported. (trigger: src/coder_eval/orchestrator.py) (restates: Axis 5: before_grading is called at 2 of the 4 grading sites) - 🟡 The host rewrites task.json with ERROR after a grading denial, but the in-container task.html (
write_task_html, orchestrator.py:1129) is not rewritten. The re-grade paths (evaluate,run --resume,_owes_a_grade,is_execution_fact) are also not updated to treat this new ERROR as re-gradeable. That makes the error message's remedy (coder-eval evaluate <run_dir>) a dead end. (trigger: src/coder_eval/isolation/docker_runner.py) _(restates: Axis 5: record_grading_denials rewrites final_status/task.json ad hoc outside the terminal-status chain) - 🔵 Agents get an egress SPI (
uses_api_backend,egress_hosts) that is documented in docs/EXTENDING.md. The new-criterion checklist in EXTENDING.md §2 does not say that a model-calling criterion must also be added by hand to the isinstance checks in isolation/egress.py (_model_routesL132,resolve_egress_targetsL170). Without that, the criterion's host is refused under llm_only. (trigger: src/coder_eval/isolation/egress.py) (restates: Axis 5: Egress hosts come from isinstance checks on core criterion classes) - 🔵 The run-dir layout reference
.claude/shared/run-layout.mddoes not list the new per-task filesegress.logorgrade.egress.log, or the.coder_eval_egress_grading_request/_ackhandshake dotfiles that path_utils.py adds. It listsgrade.logandtask.log, so a reader of the layout cannot tell which files a resume or a copy must keep or clear. (trigger: src/coder_eval/path_utils.py)
Tests:
- 🟠 No test checks that a FAILURE stays a FAILURE when the denials come from the criteria themselves. For example, run the probe-task log shape (
DENY example.com:443and model-host:8/9 afterPHASE grading) from docker_egress_probe.yaml or docker_egress_grading.yaml throughgrading_denials+_record_grading_denials. The current tests cover only 'judge noise' and 'no phase line'. (trigger: src/coder_eval/isolation/docker_runner.py) (restates: Axis 8: Grading-denial classifier turns FAILURE into ERROR on any DENY after PHASE grading) - 🟠 No test runs
evaluate <run_dir>on a denial-ERROR row with the host now ingrading_egress_allowlistand checks that the row is graded again. That is the exact remedy the new error_message tells users to follow. (trigger: src/coder_eval/isolation/docker_runner.py) _(restates: Axis 5: record_grading_denials rewrites final_status/task.json ad hoc outside the terminal-status chain) - 🟠 No test puts stale
.coder_eval_egress_grading_request/_ackfiles in run_dir beforeDockerRunner.run()and checks thatenter_gradingis not called before the container makes a new request. This is the resume-after-SIGKILL case. (trigger: src/coder_eval/isolation/docker_runner.py) (restates: Axis 8: Grading handshake files are removed only in a finally after the run) - 🟡 No test covers the overlap case where every
grading_egress_allowlisthost is already inegress_allowlist. Heregrading_widens_egressis True butEgressPlan.switches_for_gradingadds nothing, andcheck_grading_network_without_simulationrefuses a simulated task. (trigger: src/coder_eval/models/sandbox.py) (restates: Axis 1: Two predicates for "grading widens egress" are derived separately and can disagree) - 🟡 No test checks the documented 'refused at load' rule for
grading_egress_allowlistundernetwork: noneornetwork: bridge. tests/test_docker_egress_config.py:97 encodes the lenient behaviour that the docs say is refused. (trigger: src/coder_eval/models/sandbox.py) (restates: Axis 8: grading_egress_allowlist / egress_allowlist are silently accepted and ignored outside network: llm_only) - 🟡 The built-in
egress_hosts()implementations have no per-agent table test for a model prefix outside the known set (for example a Pi model on Bedrock). test_egress_targets.py:192 only checks that each registered config's hook returns without error on an empty config. (trigger: src/coder_eval/models/agent_config.py) (restates: Axis 7: Piegress_hostssilently allowlistsopenrouter.aifor an unrecognised provider prefix)
Downstream consumers:
- 🟠 The new FAILURE-to-ERROR reclassification feeds every status consumer, and none of these consumers is reviewed or tested for it:
tasks_errorand the pass-rate inputs, the worst-status fold across replicates, suite thresholds, the JUnit writer (a<failure>becomes an<error>for CI-gate Action users), theevaluateexit code, andpartition_for_resume, which treats ERROR as complete. A criterion-caused DENY therefore changes all of them for identical agent output. (trigger: src/coder_eval/isolation/docker_runner.py) (restates: Axis 8: Grading-denial classifier turns FAILURE into ERROR on any DENY after PHASE grading) - 🟡 docs/REPORT_SCHEMA.md (the task.json contract that coder-eval-uipath reads) is not updated. It does not name
environment_info.egress_denied_in_grading, the new cause of an ERROR whoseerror_messageis a grading denial, or the newegress.log/grade.egress.logartifacts. (trigger: src/coder_eval/isolation/docker_runner.py)
Display & mapping dicts:
- 🔵 The new
egress_denied_in_gradingenvironment_info key is a list. It has no entry in the env-table key handling (reports/helpers.pyis_env_table_key), so the HTML, markdown and experiment Environment tables render it as a Python repr. The evalboardruns.tsenvironment_info value type also does not allow a list. No new enum values were added, so the status and icon dicts need no change. (trigger: src/coder_eval/isolation/docker_runner.py) (restates: Axis 8: environment_info.egress_denied_in_grading is a list, but the reports and the evalboard expect flat scalar values)
Nightly pipeline:
- 🟡 The PR description does not state the impact on the nightly or coder-eval-uipath pipeline. The default
network: bridgeis unchanged, so existing nightly tasks should not be affected. A nightly task that opts into llm_only has new needs and new effects that the PR does not state: the framework image must be present on the runner for the sidecar (make docker-image), the daemon must allow labelled network create and prune, and its FAILURE rows can become ERROR, which moves the error counts that the coder-eval-uipath dashboards read. (trigger: src/coder_eval/isolation/egress.py)
Top 5 Priority Actions
- Stop the FAILURE-to-ERROR rewrite in docker_runner.py:829-835 from firing on every DENY after PHASE grading: grading_denials (egress.py:468-481) also counts the hosts that criteria probe on purpose (docker_egress_probe.yaml, and
! curl https://example.comin docker_egress_grading.yaml), so a real leak or regression is recorded as ERROR with advice to allowlist example.com. Record the denials in environment_info and a warning and keep FAILURE, or limit the rewrite to hosts that a criterion declares, and add a test that runs the probe task's log shape through it. - Move the denial verdict out of _record_grading_denials (docker_runner.py:817-839) and into the Orchestrator's terminal-status chain or a re-gradeable marker: today the ERROR it writes is an execution fact that
evaluate <run_dir>(orchestrator.py:464-470, evaluate_command.py:591) andrun --resume(batch.py:431) will not overturn, so the fix that the error message gives cannot work; task.html also still shows FAILURE because this third task.json writer does not rewrite it. - Call _remove_grading_handshake(output_dir) before
docker runand before the watch task starts (docker_runner.py:191, :744-748), and make the container delete a stale ack before it writes its request (run_task_internal_command.py:71-74): after a host SIGKILL during grading, --resume reuses the run_dir, switches the sidecar to the grading allowlist (or any host) when the agent starts, and counts every agent-phase DENY as a grading denial. - Make the run() cleanup and cancel paths total: in the inner finally (docker_runner.py:755-763), kill the container and close the log before you await the watch task, catch OSError on the ack write in _serve_grading_request (:201), and in _run_to_completion (egress.py:537) let the caller's cancel always win. Today an OSError discards the container's real task.json, and a cancelled task is recorded as an egress-setup ERROR.
- Add the missing contract tests: a pure _grading_hook helper for run_task_internal_command.py:280-284 with parametrized llm_only/bridge/none/execute cases that read authored_sandbox; a raising before_grading that gives an ERROR row at orchestrator.py:2221 and keeps the original error on the post-failure path at :863; and a daemon-free run() test that covers docker_runner.py:744-767. Also refuse or document grading_egress_allowlist outside network: llm_only (sandbox.py:384 against DOCKER_ISOLATION.md:162) and remove the grading_network: "llm_only" value, which has no runtime effect (sandbox.py:270).
The full review is longer than GitHub's 65,536-character comment limit. Nits and Harness & Lint Improvements are in a follow-up comment.
Stats: 0 🔴 · 7 🟠 · 8 🟡 · 18 🔵 across 8 axes reviewed.
Review (part 2 of 2): feat(docker): add network: llm_only with a separate grading allowlistFollow-up to the review against Nits
Harness & Lint ImprovementsStatic checks (lint / type):
Harness improvements (not statically reachable):
|

Why
network: nonecuts both, because the agent runs inside the container, so no-internet tasks fall back to success criteria that catch a lookup only after the fact.network: llm_only, by @CarlesUIPath), which was held on two design questions (answered below). This PR carries #211's 18 commits unchanged, a merge ofmain, and four changes on top.1. From #211, unchanged
docker network create --internalwith no IPv4 gateway: the task container has no route outegress_proxy.py(stdlib only) forwardsCONNECTand plain HTTP to exacthost:porttargets only. Runs as uid 65534, all capabilities dropped, read-only rootbedrock-runtime.us-east-2.amazonaws.com:443,bedrock.us-east-2.amazonaws.com:443. Plugin agents add theirs withegress_hosts()HTTPS_PROXY/HTTP_PROXYpoint at the sidecar, DNS goes to a black-hole resolver,NET_RAWis dropped. A tool that ignores the proxy has no route, so it fails closedCONNECTper allowed host before the agent starts; a failure is anERRORrow that names the hostegress.logdocker.logtasks/docker_egress_probe/docker_egress_probe.yamltries every way out (proxy, direct IPv4/IPv6, DNS, raw sockets, docker host, pip, git)2. New: a separate grading allowlist
Graders often install their own test dependencies (SkillsBench test scripts run
apt-get, the uv installer anduvxfrom PyPI). With one allowlist, those hosts are open to the agent too.coder-eval runcoder-eval executeevaluate,run --resume)grading_network: bridge, plainbridgedocker execinto the sidecar to reach a control port on the sidecar's own loopback, the proxy logsPHASE grading, and the criteria run after the host confirms.3. New: leaked networks are reclaimed (the #211 review blocker)
network createon that daemon failed.llm_onlysetup first runs:4. New: hosts refused during grading are reported
FAILURE, which looked like an agent miss. Onlyegress.logshowed the cause.llm_onlyrun marksPHASE grading, also with no grading field. Then the switch adds no host and is best-effort. After the run, the host reads the sidecar log from Docker, not from the run directory, which the container can write.FAILUREERROR, and the message names the hostshttp-intake.logs.us5.datadoghq.com:443,api.anthropic.com:443) do not count, or everyagent_judgeFAILURE on the direct or litellm route would become ERROR.Also in this PR
filelock4.2.4 andvirtualenv21.14.13: new advisories fail pip-audit and osv-scanner in the quality gate.mainhas the same versions.API_BACKEND, so on Codex (Azure) it asked for the Bedrock host and failed although Codex's own host was allowed. It now tries each model host the container's variables name.Not taken from Harbor
gostwithNET_ADMIN), wildcard and CIDR entries, task-level and per-step policy. The schema stayssandbox.docker.*.On the two questions from #211
network_mode: public | no-network | allowlistper phase, enforced by agosttransparent-proxy sidecar withNET_ADMIN. Its own agent calls the model from the host, and in-container agents need--allow-agent-hostby hand. This PR keeps #211's explicit proxy (no capabilities, no third-party image) and adds Harbor's agent/verifier split, the one Harbor feature that changes results for us.egress_hosts(), and grading is one switch point in the orchestrator. Other sandbox drivers are not covered: the fields are Docker-only, asnetworkalready is.Risks
HTTPS_PROXYnever reaches the proxy. It fails closed, but it leaves noDENYline, so § 4 cannot report it.llm_only(#168 suggests mapping it to Harbor'sallowlistmode; export refusesllm_onlytoday), and #211's non-blocking review item that_model_routescopies the orchestrator's route resolution.Validation
make verifypartstest_stats_nonfinitecases that also fail onmainlocally on Python 3.13.7task.jsonrewrite replaces a planted symlink, a phase-only switch that fails does not stop gradinguntil=10mremoved nothing (all just created);until=1sremoved a stopped sidecar and an unused network, and kept a running container and the network it usedEvery harness,
docker_egress_probe.yaml(colima, Docker 29.2.1, framework image built from this branch). The agent tries 8 ways out, then 20 grading probes run in the same container, so each run also goes through the grading switch (PHASE gradingin every log):bedrock-runtimegpt-5.6-terra, AzureCODEX_BASE_URLhostgemini-3.5-flashgenerativelanguage.googleapis.comopencode-ai1.19.4)bedrock-runtimebedrock-runtime--noproxy, Python urllib, pip, git clone, a DNS lookup, the harness's web fetch tool, its web search tool. Claude Code's WebFetch and OpenCode'swebfetchwere tried and refused. The other harnesses offered no web fetch tool, and no harness offered web search on these routes.--noproxy, external DNS and a public resolver, the docker host, the gateway, other sidecar ports, raw sockets, pip, git and npm, and the model host on another port.Grading, end to end in Docker (same setup): the separate grading allowlist, the switch, detached grades and the refused-host report.
egress.logdocker_egress_grading.yaml(no agent)DENY pypi.orginpre_run,PHASE grading,ALLOW pypi.org,DENY example.com(recorded, status kept)-D sandbox.docker.network=bridge(control)-D sandbox.docker.grading_network=bridgePHASE grading allow=*, thenexample.comallowedexecutethenevaluatepypi.orgallowed from the start,example.comdeniedexecutethenevaluate,grading_network: bridgebridge, no sidecarpypi.org, no grading fieldPHASE grading allow=unchanged,DENY pypi.org:443executethenevaluategrade.egress.log:DENY pypi.org:443grading_egress_allowlist: [pypi.org], on48887ebcpypi.orgdenied,PHASE grading, grading'spypi.orgallowedllm_onlytask 10 minutes laterCloses #168
🤖 Generated with Claude Code