Skip to content

feat(docker): add network: llm_only with a separate grading allowlist - #239

Open
bai-uipath wants to merge 24 commits into
mainfrom
bai/network-policy
Open

bai-uipath wants to merge 24 commits into
mainfrom
bai/network-policy

Conversation

@bai-uipath

@bai-uipath bai-uipath commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Why

  • Fixes #168 (opened by Sugarc0de): there is no way to block an agent's internet without also blocking its model API. network: none cuts 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.
  • Replaces #211 (network: llm_only, by @CarlesUIPath), which was held on two design questions (answered below). This PR carries #211's 18 commits unchanged, a merge of main, and four changes on top.

1. From #211, unchanged

sandbox:
  driver: docker
  docker:
    network: llm_only
    egress_allowlist: [pypi.org]   # optional; a bare host means :443
Piece What it does
Per-task network docker network create --internal with no IPv4 gateway: the task container has no route out
Proxy sidecar egress_proxy.py (stdlib only) forwards CONNECT and plain HTTP to exact host:port targets only. Runs as uid 65534, all capabilities dropped, read-only root
Derived allowlist the model hosts the task calls, from the backend, agent and judges. Bedrock example: bedrock-runtime.us-east-2.amazonaws.com:443, bedrock.us-east-2.amazonaws.com:443. Plugin agents add theirs with egress_hosts()
Task container HTTPS_PROXY/HTTP_PROXY point at the sidecar, DNS goes to a black-hole resolver, NET_RAW is dropped. A tool that ignores the proxy has no route, so it fails closed
Setup probe one CONNECT per allowed host before the agent starts; a failure is an ERROR row that names the host
Heartbeat the sidecar stops itself when the host process stops touching the heartbeat file
egress.log one line per decision, beside docker.log
Probe task tasks/docker_egress_probe/docker_egress_probe.yaml tries every way out (proxy, direct IPv4/IPv6, DNS, raw sockets, docker host, pip, git)
READY 0.0.0.0:3128 allow=bedrock-runtime.us-east-2.amazonaws.com:443,bedrock.us-east-2.amazonaws.com:443
ALLOW bedrock-runtime.us-east-2.amazonaws.com:443 CONNECT
DENY example.com:443 CONNECT

2. New: a separate grading allowlist

Graders often install their own test dependencies (SkillsBench test scripts run apt-get, the uv installer and uvx from PyPI). With one allowlist, those hosts are open to the agent too.

    network: llm_only
    grading_egress_allowlist: [pypi.org, files.pythonhosted.org]   # criteria only
    # or: grading_network: bridge                                   # criteria reach any host
Container Allowlist
coder-eval run the agent's; the grading hosts are added when grading starts
coder-eval execute the agent's only
Detached grade (evaluate, run --resume) agent + grading hosts from the start; with grading_network: bridge, plain bridge
  • How the switch works: the in-container run writes a request file, the host runs docker exec into the sidecar to reach a control port on the sidecar's own loopback, the proxy logs PHASE grading, and the criteria run after the host confirms.
  • Setup check: each grading-only host gets one direct TCP connection from the sidecar at setup, so an unreachable host fails before the agent is paid for.
  • Simulated tasks with a grading field are refused at load, because their criteria run between dialog turns.
READY 0.0.0.0:3128 allow= grading=+pypi.org:443
DENY pypi.org:443 CONNECT            # pre_run, agent phase
PHASE grading allow=+pypi.org:443
ALLOW pypi.org:443 CONNECT           # criteria
DENY example.com:443 CONNECT

3. New: leaked networks are reclaimed (the #211 review blocker)

  • Before: a SIGTERM or SIGKILL of coder-eval skips the teardown. Each killed task left a stopped sidecar and a network, and each network holds one Docker address pool (about 29 on Docker Desktop). After a few cancelled parallel runs, every network create on that daemon failed.
  • After: every llm_only setup first runs:
docker container prune --force --filter label=org.coder-eval.egress --filter until=10m
docker network prune   --force --filter label=org.coder-eval.egress --filter until=10m
  • Prune never removes a running container or a network that is in use, and the 10-minute age spares a concurrent setup. A prune that the daemon refuses (another prune is running) is skipped.
  • Not added: a SIGTERM-to-cancel handler. The prune also covers SIGKILL, and a process-wide handler would change shutdown for every driver.

4. New: hosts refused during grading are reported

  • Before: a grader that could not reach a host it needed scored a plain FAILURE, which looked like an agent miss. Only egress.log showed the cause.
  • After: every graded llm_only run marks PHASE 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.
Row status Grading was refused a host
FAILURE becomes ERROR, and the message names the hosts
any other kept
"final_status": "ERROR",
"error_message": "Grading was refused access to pypi.org:443 under network: llm_only, so the grader may have failed, not the agent. Add each host to sandbox.docker.grading_egress_allowlist and grade the run again (`coder-eval evaluate <run_dir>`).",
"environment_info": {"egress_denied_in_grading": ["pypi.org:443"]}
  • The judge CLI hosts that are refused harmlessly (http-intake.logs.us5.datadoghq.com:443, api.anthropic.com:443) do not count, or every agent_judge FAILURE on the direct or litellm route would become ERROR.
  • A detached grade container counts its whole log, because no agent runs in it.

Also in this PR

  • filelock 4.2.4 and virtualenv 21.14.13: new advisories fail pip-audit and osv-scanner in the quality gate. main has the same versions.
  • The probe task's model-host check works on every harness. It read only 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

  • A transparent proxy (gost with NET_ADMIN), wildcard and CIDR entries, task-level and per-step policy. The schema stays sandbox.docker.*.

On the two questions from #211

  • How other frameworks do it: Harbor 0.23 has network_mode: public | no-network | allowlist per phase, enforced by a gost transparent-proxy sidecar with NET_ADMIN. Its own agent calls the model from the host, and in-container agents need --allow-agent-host by 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.
  • Is it extensible: plugin agents declare their model hosts with egress_hosts(), and grading is one switch point in the orchestrator. Other sandbox drivers are not covered: the fields are Docker-only, as network already is.

Risks

  • The switch trusts the container. The agent is root in the same container. It can write the request file early, and a process it leaves running keeps running into grading. The same paths let it turn its own FAILURE into ERROR, which still counts as a miss in the pass rate. This is documented, and it is the same class of gap as the anti-cheat window.
  • A tool that ignores HTTPS_PROXY never reaches the proxy. It fails closed, but it leaves no DENY line, so § 4 cannot report it.
  • Exact hosts only. Redirect targets (for example GitHub release-asset hosts for the uv installer) must be listed one by one.
  • Not done here: a Harbor export of llm_only (#168 suggests mapping it to Harbor's allowlist mode; export refuses llm_only today), and #211's non-blocking review item that _model_routes copies the orchestrator's route resolution.
  • Overlap: #211 should be closed in favor of this PR.

Validation

Check Result
make verify parts format, ruff, pyright (src and tests), custom lint (741) and prose budget pass
pytest (not live) 7103 passed, 3 failed: the test_stats_nonfinite cases that also fail on main locally on Python 3.13.7
New unit tests prune arguments and a refused prune, denial parsing (agent phase, judge noise, no phase line), FAILURE to ERROR and SUCCESS kept, task.json rewrite replaces a planted symlink, a phase-only switch that fails does not stop grading
Prune behavior, Docker 29.2.1 until=10m removed nothing (all just created); until=1s removed a stopped sidecar and an unused network, and kept a running container and the network it used

Every 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 grading in every log):

Harness Model Agent's 8 attempts blocked 19 grading probes blocked Model host allowed, on its own port only Row
Claude Code Sonnet 5, Bedrock ✅ 8 of 8 ✅ 19 of 19 ✅ bedrock-runtime ✅ SUCCESS 1.000, 9 of 9 criteria
Codex gpt-5.6-terra, Azure ✅ 8 of 8 ✅ 19 of 19 ✅ the CODEX_BASE_URL host ✅ SUCCESS 1.000, 9 of 9
Antigravity gemini-3.5-flash ✅ 8 of 8 ✅ 19 of 19 ✅ generativelanguage.googleapis.com ✅ SUCCESS 1.000, 9 of 9
OpenCode Sonnet 5, Bedrock (framework image plus opencode-ai 1.19.4) ✅ 8 of 8 ✅ 19 of 19 ✅ bedrock-runtime ✅ SUCCESS 1.000, 9 of 9
Pi Sonnet 5, Bedrock ✅ 8 of 8 ✅ 19 of 19 ✅ bedrock-runtime ✅ SUCCESS 1.000, 9 of 9
Delegate not run
  • The 8 attempts: curl, curl --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's webfetch were tried and refused. The other harnesses offered no web fetch tool, and no harness offered web search on these routes.
  • The 19 blocked probes: the proxy (CONNECT, plain HTTP, absolute-form HTTPS, urllib, curl), direct IPv4 and IPv6, the default route, curl --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.
  • Delegate: not run. It needs a UiPath cloud login, and the identity endpoint that login uses does not answer now. The same cause fails the "Delegate Live E2E (ROPC)" check. Its CLI is also not in the framework image.

Grading, end to end in Docker (same setup): the separate grading allowlist, the switch, detached grades and the refused-host report.

Run Row egress.log
docker_egress_grading.yaml (no agent) SUCCESS 1.000 DENY pypi.org in pre_run, PHASE grading, ALLOW pypi.org, DENY example.com (recorded, status kept)
same, -D sandbox.docker.network=bridge (control) FAILURE 0.333 no sidecar
same, -D sandbox.docker.grading_network=bridge FAILURE 0.667 PHASE grading allow=*, then example.com allowed
same, execute then evaluate SUCCESS 1.000 grading container: pypi.org allowed from the start, example.com denied
same, execute then evaluate, grading_network: bridge FAILURE 0.667 plain bridge, no sidecar
a grader that needs pypi.org, no grading field ERROR (was FAILURE) PHASE grading allow=unchanged, DENY pypi.org:443
same, execute then evaluate ERROR grade.egress.log: DENY pypi.org:443
Claude Code (Sonnet 5, Bedrock) with grading_egress_allowlist: [pypi.org], on 48887ebc SUCCESS 1.000 the agent's pypi.org denied, PHASE grading, grading's pypi.org allowed
SIGKILL of coder-eval mid-task, then a new llm_only task 10 minutes later the kill left a stopped sidecar and its network; a task run 3 minutes after the kill kept them (too new); the task run 10 minutes after it removed both

Closes #168

🤖 Generated with Claude Code

CarlesUIPath and others added 24 commits October 1, 2026 16:11
…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>
@bai-uipath
bai-uipath marked this pull request as ready for review October 26, 2026 21:47
@github-actions

github-actions Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Claude finished @bai-uipath's task in 1m 36s —— View job


Review in Progress

Todo List

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Analyze the full diff (git diff origin/main...HEAD)
  • Review implementation files with full context
  • Perform cross-file consistency checks
  • Analyze what's missing
  • Provide design-level scrutiny
  • Format and post final review

Starting review of PR #239: Docker network isolation with llm_only mode...

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: 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

  1. [Axis 1] grading_network: llm_only behaves the same as None, duplicates grading_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 from None. Every consumer tests only for == "bridge": grading_widens_egress (L396 self.grading_network == "bridge" or bool(self.grading_egress_allowlist)), DockerRunner._container_network (docker_runner.py:789), _resolve_egress_plan (docker_runner.py:811 grading_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 plus grading_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 (L386 if self.network == "bridge" and self.grading_network == "bridge": return self). The second field also slips past the validator: grading_egress_allowlist is ignored without an error under network: bridge/none and under grading_network: bridge. DOCKER_ISOLATION.md:162 says "Both fields need network: 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 example grading_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 unless network == "llm_only". The project is greenfield, so the removal needs no back-compat. Cross-axis: 7 (knob surface and docs mismatch).
  2. [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

  1. [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_targets trusts the hook's output as-is: targets.update(task.agent.egress_hosts(forwarded)) (egress.py:168). The hook is a public SPI typed def egress_hosts(self, env: Mapping[str, str]) -> tuple[str, ...] (agent_config.py:225). It says "Normalized host:port targets" only in its docstring, and docs/EXTENDING.md:86 just says "return the host:port targets". egress_allowlist accepts 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_allow calls split_host_port(entry, None), which returns None for a port-less entry, so the proxy logs BAD --allow and exits 2 (egress_proxy.py:437-440, 466-469). The host then sees only the docker exec probe fail against a stopped container, and that error does not name the bad entry. Fix: pass each hook result through normalize_egress_target at 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 a NewType('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.
  2. [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:395 with pytest.raises(FileNotFoundError): await _rt_runner(tmp_path, "llm_only").run(), raises at asyncio.create_subprocess_exec. None of lines 744-767 run: the grading_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_handshake in the finally (755-759), and the scoring call site if egress is not None and self.grade: await self._record_grading_denials(...) (766-767). _record_grading_denials and _serve_grading_request have 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 fake egress_scope whose docker logs returns a log with PHASE grading and a DENY. Check that (a) a FAILURE becomes ERROR when grade=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.
  3. [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-194 request = 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 708 grant_container_access(output_dir, writable=True), line 1569 -v {output_dir}:{CONTAINER_OUTPUT_DIR}). The file name is a fixed constant (path_utils.py:48 EGRESS_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:72 await asyncio.to_thread(write_text_atomic, output_dir / EGRESS_GRADING_REQUEST_FILENAME, "grading"). One touch from 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_inputs already deletes after load. The in-container orchestrator writes the nonce into the request file, and _serve_grading_request refuses any request whose content does not match. This turns a touch into 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
  4. [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] and egress_hosts(env) on BaseAgentConfig (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) and if isinstance(criterion, SystemOneJudgeCriterion): (L170). The two branches also disagree: L170 does not check c.enabled, so a disabled system_one_judge still opens its host. A plugin-registered criterion that calls resolve_evaluation_route or 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] = False and egress_hosts(env) -> tuple[str, ...]). Override them on LLMJudge, AgentJudge and SystemOneJudge, and make resolve_egress_targets iterate polymorphically. Then document the hook beside the agent one in EXTENDING.md.
  5. [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 gives DockerRunner.run - A (5) (L612) and _build_argv - D (24). At PR HEAD it gives DockerRunner.run - C (15) (L641) and DockerRunner._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_dir and _network_name, plus a nested async with scope / grading_watch / heartbeat try/finally inside run(). Fix: move plan resolution and the handshake into egress.py, behind one context manager such as egress_session(rt, grade, grade_workspace, output_dir) that yields the handle and owns the grading watcher and its cleanup. run() then has a single async with. Fix the status rewrite with the previous finding.
  6. [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 installs before_grading for every graded llm_only run, so a simulated llm_only row never writes PHASE 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_routes L132, resolve_egress_targets L170). 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.md does not list the new per-task files egress.log or grade.egress.log, or the .coder_eval_egress_grading_request / _ack handshake dotfiles that path_utils.py adds. It lists grade.log and task.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:443 and model-host:8/9 after PHASE grading) from docker_egress_probe.yaml or docker_egress_grading.yaml through grading_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 in grading_egress_allowlist and 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 / _ack files in run_dir before DockerRunner.run() and checks that enter_grading is 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_allowlist host is already in egress_allowlist. Here grading_widens_egress is True but EgressPlan.switches_for_grading adds nothing, and check_grading_network_without_simulation refuses 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_allowlist under network: none or network: 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: Pi egress_hosts silently allowlists openrouter.ai for 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_error and 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), the evaluate exit code, and partition_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 whose error_message is a grading denial, or the new egress.log / grade.egress.log artifacts. (trigger: src/coder_eval/isolation/docker_runner.py)

Display & mapping dicts:

  • 🔵 The new egress_denied_in_grading environment_info key is a list. It has no entry in the env-table key handling (reports/helpers.py is_env_table_key), so the HTML, markdown and experiment Environment tables render it as a Python repr. The evalboard runs.ts environment_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: bridge is 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

  1. 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.com in 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.
  2. 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) and run --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.
  3. Call _remove_grading_handshake(output_dir) before docker run and 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.
  4. 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.
  5. 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.

@uipreliga

Copy link
Copy Markdown
Collaborator

Review (part 2 of 2): feat(docker): add network: llm_only with a separate grading allowlist

Follow-up to the review against 156dcba: the nits and the harness / lint proposals.

Nits

  1. [Axis 1] Two predicates for "grading widens egress" are derived separately and can disagree (src/coder_eval/models/sandbox.py:394) — DockerDriverConfig.grading_widens_egress (L396: self.grading_network == "bridge" or bool(self.grading_egress_allowlist)) and EgressPlan.switches_for_grading (egress.py:242: self.grading_any or bool(self.grading_targets)) both answer the same question. The second runs after docker_runner.py:812 filters out the hosts the agent already has (tuple(t for t in cfg.grading_egress_allowlist if t not in targets)). When every grading_egress_allowlist host is also in egress_allowlist, the container gets widens=True (run_task_internal_command.py:283), so a failed switch is fatal although the switch adds nothing. check_grading_network_without_simulation (tasks.py:693) also refuses the task. Choose one source of truth: drop the property in favour of the plan, or normalize the overlap away in the model.
  2. [Axis 1] Orchestrator grading hook is an Optional callable plus a separate one-shot boolean (src/coder_eval/orchestrator.py:357) — self.before_grading: Callable[[], Awaitable[None]] | None = None (L357), self._grading_started = False (L358) and _start_grading (L884-887: if self.before_grading is not None and not self._grading_started: self._grading_started = True; await self.before_grading()) keep two fields for one piece of state in a hot module. Consume the hook once instead: hook, self.before_grading = self.before_grading, None; if hook is not None: await hook(). This removes the flag. Alternatively, default to a no-op coroutine (Null Object) and reset it after the first call.
  3. [Axis 1] Small literal and helper duplication in the egress modules and probe script (src/coder_eval/isolation/egress.py:264) — (a) task_container_egress_argv lists the proxy variable names again (L264 for name in ("HTTPS_PROXY", "HTTP_PROXY", "https_proxy", "http_proxy"), L266 for name in ("NO_PROXY", "no_proxy")) next to PROXY_ENV_NAMES (L189-201). (b) build_probe_argv (L356-362) and build_grading_argv (L372-378) repeat the "exec", sidecar, "python3", "-I", f"{SIDECAR_EGRESS_DIR}/{EGRESS_PROXY_MODULE}" prefix. (c) enter_grading (L453-459) repeats the try/except and returncode-to-EgressSetupError shape of _checked (L405-421). (d) The error text "{source} has a host or port that network: llm_only cannot allowlist." is written twice (egress.py:111 and models/sandbox.py:159). Derive the argv names from one tuple, add a _proxy_exec_argv(sidecar, *args) helper, and route enter_grading through _checked (with a stdout-first detail option).
  4. [Axis 1] Module layout: constants and logger declared between functions (src/coder_eval/models/sandbox.py:136) — logger = logging.getLogger(__name__) (sandbox.py:136) is declared between normalize_egress_target and url_egress_target instead of with the module header. In isolation/egress.py, _LITELLM_BASE_KWARGS = ("api_base", "base_url") (L72) and the full constants block (EGRESS_PROXY_MODULE ... _NETWORK_RM_RETRY_SECONDS, L178-220) sit between function definitions. Move the module-level constants and the logger to the top of each module, so a reader finds the configuration surface in one place.
  5. [Axis 2] _container_network() -> str and _network_name(network: str, ...) widen the Literal["bridge", "none", "llm_only"] network type to str (src/coder_eval/isolation/docker_runner.py:786) — def _container_network(self) -> str: (docker_runner.py:786) and def _network_name(network: str, egress: EgressHandle | None) -> str: (line 174) drop the closed Literal from DockerDriverConfig.network. The network != "none" comparisons in _build_argv (lines 1533, 1547) and the == "llm_only" branch in _network_name then become free-string compares that pyright cannot tie to the model's value set. Add a type DockerNetwork = Literal["bridge", "none", "llm_only"] alias in models/sandbox.py, use it for the field and for both helpers' parameter and return types, so a future network mode or a typo shows in type checking.
  6. [Axis 2] normalize_egress_target accepts invalid IPv4 literals and over-long labels, though its docstring says the host must be a DNS name or an IPv4 address (src/coder_eval/models/sandbox.py:107) — The host regex _EGRESS_HOST = re.compile(r"[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*") (sandbox.py:107, applied at line 125) accepts '999.999.999.999', '1.2.3', and labels over 63 characters or names over 253. These entries pass the user-facing egress_allowlist / grading_egress_allowlist validator and fail only later at the sidecar probe, as a DNS failure. Reject an all-numeric dotted host that ipaddress.IPv4Address does not accept, and enforce the RFC 1035 length limits, so the error appears when the task loads.
  7. [Axis 2] _model_routes catches AssertionError from resolve_route's type-narrowing asserts, so the allowlist depends on asserts that python -O strips (src/coder_eval/isolation/egress.py:120) — except (AssertionError, ValueError): (egress.py:120) depends on resolve_route's Bedrock branch, which uses assert settings.aws_region is not None only for pyright narrowing (routing.py:176-177; its docstring says asserts are for narrowing, not validation). Under python -O the assert is gone, so BedrockRoute(region=None) is built even though the field is typed str. _bedrock_targets(None) then gives 'bedrock-runtime.none.amazonaws.com:443', which passes normalize_egress_target, and the setup fails with a misleading probe error. The LiteLLM branch already raises instead of asserting, so that it survives -O (routing.py:191-192). Make the Bedrock branch raise ValueError the same way, and catch only ValueError here.
  8. [Axis 3] The fold-back of egress.log to grade.egress.log after a detached grade has no test (src/coder_eval/orchestration/regrade.py:698) — The PR adds (EGRESS_LOG_FILENAME, GRADE_EGRESS_LOG_FILENAME), to the rescued tuple in _fold_back_container_logs. The existing fold-back tests in tests/test_regrade.py (lines 529, 668-675, 961, 996) check only grade.docker.log, including the symlink-victim case at 996. Extend the 668 test to put an egress.log in the scratch dir and check that grade.egress.log lands in the row dir, so a dropped tuple entry does not lose the grading proxy log without notice.
  9. [Axis 3] Error branches in egress setup have no test, including the documented 'unreachable grading host fails before the agent runs' message (src/coder_eval/isolation/egress.py:433) — The routed coverage shows these new branches as missed: egress.py:433-437 (if direct: raise EgressSetupError(f"network: llm_only could not reach a grading host from the egress sidecar: {detail}. ...grading_egress_allowlist."), the behaviour documented at DOCKER_ISOLATION.md:171-173; test_scope_dials_the_grading_hosts_directly covers only success), 145-146 (judge api_route ValueError warning), 603-604 (empty-allowlist warning), and docker_runner.py:702 (raise EgressSetupError(f"network: llm_only could not stage the egress proxy: {exc}")). Add a probe-direct-failure case to the test_a_failed_setup_tears_down_what_it_created parametrization (answer exec c-egress twice, failing the second) that matches grading_egress_allowlist, plus caplog cases for the two warnings.
  10. [Axis 3] The new grading-network validators have tests for the refusal only, not for the accepted inverse (tests/test_egress_grading_phase.py:58) — test_a_simulated_task_cannot_widen_grading checks that check_grading_network_without_simulation (models/tasks.py:691) raises for simulation.enabled=True with grading_network="bridge". No test checks the other half of the rule: a simulated task with grading_network: llm_only or no grading fields (does not widen), or with simulation.enabled=False plus grading_network: bridge, must still load. A validator that is too broad (for example one that checks grading_network is not None instead of grading_widens_egress) would reject valid dialog tasks and no test would fail. Add a parametrized acceptance case next to the refusal.
  11. [Axis 4] The egress proxy logs the absolute-form https target without redaction, which can put URL credentials or query tokens into egress.log (src/coder_eval/egress_proxy.py:222) — egress_proxy.py:222-224: https = split_host_port(target[len("https://") :].split("/", 1)[0].rpartition("@")[2], 443) / shown = f"{https[0]}:{https[1]}" if https else repr(target) / _log(f"BAD {shown} absolute-form https (use CONNECT)"). The authority is split only on '/'. So GET https://api.example.com?key=SECRET HTTP/1.1 logs BAD api.example.com?key=secret:443. If parsing fails (for example https://user:tok@host:99999/), the line logs repr(target) with the userinfo and the query in full. The same happens in the CONNECT path: split_host_port keeps userinfo, so CONNECT user:pw@host:443 gives a DENY user:pw@host:443 CONNECT line (line 236). These lines go to egress.log, which is a shareable run artifact. This contradicts the stated invariant in _redacted at line 71 ('the target may carry credentials') and in .claude/notes/isolation.md ('BAD lines carry only the method and length'). Busybox wget, which the docs list, sends this absolute-form https request. Fix: cut the authority at the first of '/?#' and remove userinfo before parsing, as _parse_absolute_http already does. In the parse-failure case, log _redacted(request_line) instead of repr(target). Also refuse a CONNECT authority that contains '@'. Add a test that sends a query-bearing https target and a target with userinfo and an invalid port. CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:U/C:L/I:N/A:N
  12. [Axis 5] The network decision for a detached-grade container is coded twice: the consent-gate disclosure and DockerRunner (src/coder_eval/orchestration/regrade.py:289) — The trust-gate disclosure works out the grading container's network on its own: if docker.network == "llm_only" and docker.grading_network != "bridge": and allowed = list(dict.fromkeys([*docker.egress_allowlist, *docker.grading_egress_allowlist])) (L289-291). This copies DockerRunner._container_network (docker_runner.py L786-791, if self.grade_workspace is not None and cfg.network == "llm_only" and cfg.grading_network == "bridge": return "bridge") and the grade_workspace arm of _resolve_egress_plan. The two agree today. If they drift, the operator consents to a network that differs from the one the container joins. Fix: add one pure helper (for example grading_container_network(docker_cfg) in isolation/egress.py, or a property on DockerDriverConfig next to grading_widens_egress) and call it from both sites.
  13. [Axis 5] before_grading is called at 2 of the 4 grading sites, not at one seam; the dialog criteria site never marks the phase (src/coder_eval/orchestrator.py:2304) — await self._start_grading() is added by hand before check_all_async at L863 (post-failure) and L2221 (single-shot loop). It is not called at L2145 (evaluate-only) or at L2304 (_run_dialog_criteria_check). Skipping L2145 is deliberate, because a detached grade uses whole_log=True. L2304 is a gap. run_task_internal_command.py installs before_grading for every graded llm_only run, and docs/DOCKER_ISOLATION.md says "Every graded llm_only run marks the phase". A simulated llm_only task that ends successfully never writes PHASE grading, so grading_denials returns [] and the FAILURE-to-ERROR report is off for dialog rows, with no warning. Fix: make the phase hook part of one grading seam (for example a wrapper around success_checker.check_all_async that the grading sites share). Alternatively, state the dialog exclusion in the docs and leave before_grading unset for simulated tasks.
  14. [Axis 5] A second, ad-hoc stdlib-only sidecar guard sits beside CE057 / SIDECAR_MODULES, and the module path is found with path arithmetic (src/coder_eval/isolation/docker_runner.py:211) — egress_proxy.py is the same kind of file as argv_match.py: a stdlib-only module copied or bind-mounted into an interpreter that does not have coder_eval. That class already has an SSOT (SIDECAR_MODULES) and a lint rule (CE057). This PR adds a separate pytest check instead (ALLOWED_IMPORTS = {"argparse", "asyncio", "os", "sys", "time"}, tests/test_egress_proxy.py L22/L279), which make lint does not run. It also finds the file by package depth: shutil.copy2(Path(__file__).resolve().parents[1] / EGRESS_PROXY_MODULE, ...) (L211). Fix: generalize CE057's target set to a declared list of stdlib-only sidecars, each with its own allowed imports, and add egress_proxy.py to that list. Find the file with importlib.resources.files("coder_eval") / EGRESS_PROXY_MODULE or coder_eval.egress_proxy.__file__.
  15. [Axis 6] The proxy's upstream dial does not catch UnicodeError from IDNA, so it writes no FAIL line and sends no 502 (src/coder_eval/egress_proxy.py:243) — The dial is guarded only by except (OSError, TimeoutError) as exc: around asyncio.open_connection(host, port) (lines 239-246). getaddrinfo encodes a hostname with a label longer than 63 characters through the IDNA codec and raises UnicodeEncodeError (a ValueError, not an OSError). The exception leaves handle_client as an unhandled callback traceback: the proxy writes no FAIL line and the client gets EOF, not 502. I reproduced this: a CONNECT to a host with a 70-character label under grading_any returned b'' and a traceback. This is reachable under grading_network: bridge, or with an allowlist entry that has a long label, because the _EGRESS_HOST regex in normalize_egress_target does not cap a label at 63 characters. The setup probe then fails with only no response. Fix: add UnicodeError (or ValueError) to the dial's except tuple so the proxy logs FAIL <dest> and sends _BAD_GATEWAY. Optionally, cap each label at 63 characters in _EGRESS_HOST.
  16. [Axis 6] A certain config error (network: llm_only without driver: docker) is found per task at dispatch, not before the batch starts (src/coder_eval/orchestration/batch.py:196) — if sandbox_cfg is not None and driver != "docker" and sandbox_cfg.docker.network == "llm_only": raise ValueError(...) runs inside run_single, under the semaphore and the per-task except Exception. Each affected task therefore becomes a separate in-memory ERROR row (with no task.json on disk) after the batch has started. The other resolved-config checks in run_batch (the duplicate-template-dir ValueError, check_pricing_coverage) fail the whole batch before any dispatch. This check needs only the resolved driver, which resolved_tasks already holds, so move it into that pre-dispatch block. The error then fails loud once, before any work starts.
  17. [Axis 7] 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 (src/coder_eval/models/agent_config.py:511) — provider, _, model_id = (self.model or "").partition("/") / return (_PI_PROVIDER_HOSTS.get(provider if model_id else "", "openrouter.ai:443"),) sends any explicit prefix that is not in _PI_PROVIDER_HOSTS to OpenRouter. HARNESS_PARITY.md verifies Pi on "Sonnet 5 on Bedrock", the common route. On that route the derived allowlist opens openrouter.ai:443, which the agent never uses, and leaves out the Bedrock hosts it does need. Nothing at run time tells the user to add those hosts. The parallel LiteLLM path in isolation/egress.py does log a warning when it cannot derive a host (logger.warning("The LiteLLM endpoint of %s is not forwarded: add its host to egress_allowlist.", source)). Fix: fall back to OpenRouter only when there is no prefix (Pi's own default). For an explicit unknown prefix, return () and log a warning that names the provider and points to sandbox.docker.egress_allowlist. This also stops an unused LLM-proxy host from being opened.
  18. [Axis 8] environment_info.egress_denied_in_grading is a list, but the reports and the evalboard expect flat scalar values (src/coder_eval/isolation/docker_runner.py:828) — result.environment_info["egress_denied_in_grading"] = denied (docker_runner.py:828) stores a list[str]. reports/html.py::_render_environment writes every non-excluded key as _esc(v), so the Environment table shows a Python repr (['deb.debian.org:80', 'pypi.org:443']). evalboard/lib/runs.ts:580-583 types environment_info values as string | number | null | Record<string, string>. .claude/notes/isolation.md already records the same mistake with a nested value ('flat, not a nested sub-dict: environment_info is rendered as a flat map by the HTML report and typed as one by the evalboard'). Store a comma-joined string, or move the field to a typed EvaluationResult field, so that the task.json contract that coder-eval-uipath reads stays flat. n/a

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] Widen CE031 (tests/lint/dead_config_fields.py, whole-tree class TestCE031DeadConfigFields) to dead Literal MEMBERS. For each Literal[...] field on a CE030/CE031-tracked config model (DockerDriverConfig included), every member other than the default must appear in at least one comparison (==, !=, in, match case) against an attribute of that field name somewhere in src/, outside the model's own validators. A member that no code compares is an alias of another state. Use the existing CE031 test and do not take a new CE number. Prevents: grading_network: "llm_only" (models/sandbox.py:270). Every consumer tests only == "bridge" (sandbox.py:396, docker_runner.py:789/811, regrade.py:289), so the value acts like None and its description contradicts the code.
  • [ce-lint] New CE069 final_status_single_writer (tests/lint/rules/ce069_final_status_single_writer.py, wired in tests/lint/runner.py): an ast.Assign/AnnAssign whose target is an attribute named final_status is allowed only in src/coder_eval/orchestrator.py. Today this flags exactly one site. Pair it with a check that a task.json write (write_text_atomic(<...TASK_JSON...>, <x>.model_dump_json(...))) goes through the one shared serializer that applies TASK_JSON_TRANSCRIPT_EXCLUDE and also re-renders task.html. Prevents: _record_grading_denials rewrites the verdict after finalization (docker_runner.py:830) and writes a third ad-hoc task.json (docker_runner.py:837). The result is an ERROR that evaluate cannot re-grade, a stale task.html and serializer drift.
  • [ce-lint] New CE070 grading_via_one_seam, in the same shape as CE061: in orchestrator.py, self.success_checker.check_all_async( may be called only inside one _grade(..., mark_phase: bool) method, and that method runs _start_grading() when mark_phase is true. The evaluate-only site (L2145) then passes mark_phase=False, so the exemption is written in code and is not just a missing call. Delete the hand-placed await self._start_grading() calls in the same change. Prevents: before_grading runs at only 2 of the 4 grading sites. The dialog path (orchestrator.py:2304) never writes PHASE grading, so the denial report is off for simulated llm_only rows and nothing warns about it.
  • [ce-lint] New CE071 cancel_and_reap_helper: forbid the inline X.cancel() followed by with contextlib.suppress(asyncio.CancelledError): await X inside a finally: body. Route these through one shared cancel_and_reap(task) helper. The helper cancels, awaits, logs any non-cancel exception and never raises it, so cleanup that comes after it always runs. It flags 3 sites today: docker_runner.py:758 and :777, and delegate_agent.py:727. Prevents: A grading watch that already failed re-raises an OSError from the inner finally (docker_runner.py:755-763). This masks the original error, skips log close and _kill_container, and discards the container's task.json.
  • [ce-lint] New CE072 no_except_assertion_error: forbid except AssertionError in src/coder_eval/, alone or inside a tuple. Asserts are for pyright narrowing only and python -O strips them, so validation must raise ValueError. This flags exactly one site today (egress.py:120), and the fix makes routing.py's Bedrock branch raise the way the LiteLLM branch already does. Prevents: _model_routes depends on resolve_route's narrowing asserts (egress.py:120 and routing.py:176-177). Under -O it gives bedrock-runtime.none.amazonaws.com:443 and a misleading probe failure.
  • [ce-lint] New CE073 no_criterion_isinstance_in_isolation: in src/coder_eval/isolation/, forbid isinstance(x, <Name ending in Criterion> | ...). A criterion's network needs must come from a ClassVar hook on BaseSuccessCriterion (uses_api_backend: ClassVar[bool] and egress_hosts(env)), as agents already declare theirs. requires_agent is the existing precedent. The rule is scoped to isolation/ because orchestrator.py and regrade.py still dispatch on criterion type for other reasons. Prevents: The hardcoded criterion isinstance branches at egress.py:132/170, which disagree with each other: a disabled system_one_judge still opens its host. Without the rule, each new model-calling criterion means a hand edit to egress.py.
  • [ce-lint] Widen CE057 (ce057_sidecar_shim_stdlib_only.py): derive the target set from SIDECAR_MODULES plus a declared STDLIB_ONLY_MODULES: Mapping[str, frozenset[str]] (module mapped to its allowed import roots), and add egress_proxy.py with {argparse, asyncio, os, sys, time}. Delete the ad-hoc ALLOWED_IMPORTS check in tests/test_egress_proxy.py:22/279, which make lint does not run. Keep the existing assertion that every listed module resolves to a real file. Prevents: A second, parallel stdlib-only guard for egress_proxy.py that lives outside make lint, plus the path-arithmetic locator at docker_runner.py:211.
  • [ce-lint] Complexity ratchet as a whole-tree lint test (radon is already a runtime dependency): record the cyclomatic complexity of each function in hot modules (isolation/docker_runner.py, orchestrator.py, isolation/egress.py) in a checked-in baseline. Fail when a function exceeds its baseline, and fail when a new function enters at grade C or worse. Per CLAUDE.md, set each cap below the current value, not at it. Prevents: DockerRunner.run growing from A(5) to C(15) and _build_argv from D(24) to D(27) in this PR without any gate, which left the egress lifecycle split between egress.py and the runner.
  • [pyright] Change EvaluationResult.environment_info (models/results.py:630, plus the twin at L1186) from dict[str, Any] to dict[str, EnvInfoValue], where EnvInfoValue = str | int | float | bool | None | dict[str, str] mirrors evalboard/lib/runs.ts:580-583. Pyright then rejects a list value at the write site. This complements CE054, which checks key round-trips but not value shapes. Prevents: environment_info["egress_denied_in_grading"] = denied stores a list (docker_runner.py:828), which renders as a Python repr in task.html and breaks the flat-map contract that the evalboard and coder-eval-uipath read.
  • [pyright] Add type DockerNetwork = Literal["bridge", "none", "llm_only"] in models/sandbox.py and use it for the field, _container_network() and _network_name(). Then set reportUnnecessaryComparison = "error" in [tool.pyright], so a comparison against a literal outside the set (a typo, or a removed mode) fails typecheck. Prevents: _container_network() -> str and _network_name(network: str, ...) widening the closed network set (docker_runner.py:786/174). That makes the != "none" / == "llm_only" compares at L1533/1547 free-string compares.
  • [pyright] Add EgressTarget = NewType("EgressTarget", str), constructed only by normalize_egress_target. Type the egress_hosts() SPI return as tuple[EgressTarget, ...] and the build_sidecar_create_argv targets the same way. Pyright then rejects an in-tree agent config that returns a raw str. Third-party plugins are outside pyright's scope, so keep a runtime normalize_egress_target at the seam (egress.py:168) that names the agent kind. Prevents: Plugin egress_hosts() output reaching --allow unnormalized (egress.py:168), where a bare host or URL-form entry kills the proxy with an error that does not name the entry.
  • [ci-gate] Add a diff-coverage gate to make verify / CI (for example diff-cover coverage.xml --compare-branch=origin/main --fail-under=90), scoped to src/coder_eval/isolation/, orchestrator.py and cli/run_task_internal_command.py. New error branches and new wiring in the sandbox and grading paths must be executed by at least one test. Prevents: The uncovered lines this review found: run_task_internal_command.py:282 (before_grading wiring), docker_runner.py:744-767 (grading watch and denial gate) and :702, egress.py:433-437 (documented unreachable-grading-host error), 145-146 and 603-604.

Harness improvements (not statically reachable):

  • Add an llm_only fake-daemon fixture for DockerRunner.run(). A fake create_subprocess_exec writes a contract-echoing task.json and the request file, and a fake egress_scope serves docker logs with PHASE grading plus DENY lines. Assert the graded and execute-mode verdicts, that the handshake files are gone after the run, that the watch task is cancelled when streaming is cancelled, and that an OSError on the ack write does not mask the result. Also parametrize tests/_container_contract.py::contract_payload over network modes, so the _run-task-internal before_grading wiring runs under llm_only. Why not static: These are runtime sequencing facts: subprocess lifecycle, file-poll ordering and exception propagation out of finally blocks. No AST shape expresses 'the watch result is collected after the kill'. Prevents: run() L744-767 never executed in tests, run_task_internal L282 never executed, the finally re-raise at docker_runner.py:755-763, and a dropped or inverted egress is not None and self.grade gate.
  • Add orchestrator grading-hook contract tests: (1) a raising before_grading gives an ERROR row whose message names the switch failure and that has no criterion results; (2) an agent crash with 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) a simulated llm_only task marks the phase. Why not static: How an exception maps to FinalStatus, and how often a hook runs, can only be seen by running Orchestrator.run(). Prevents: The untested failed-switch-gives-ERROR contract in DOCKER_ISOLATION.md:170, the untested post-failure site at orchestrator.py:863, and the missing dialog-path phase mark at orchestrator.py:2304.
  • Add a stale-artifact-on-resume guard. Declare a TRANSIENT_RUN_FILES tuple in path_utils.py (starting with the egress grading request and ack). Add a parametrized test that pre-seeds each file in run_dir before run() and asserts that it is removed before docker run, and that enter_grading is not called until the container makes a fresh request. A new handshake file added to the tuple is then covered automatically. Why not static: Whether a polled file is cleared before the poll starts depends on runtime ordering around a SIGKILL that skips the finally blocks, and a static check cannot see that. Prevents: --resume in the same run_dir switching to the grading allowlist (or any host under grading_network: bridge) before the agent runs (docker_runner.py:191/759), which also misattributes agent-phase DENYs as grading denials.
  • Add a re-grade round-trip test: produce a grading-denial row, add the host to grading_egress_allowlist, run coder-eval evaluate <run_dir>, and assert the row becomes SUCCESS or FAILURE. Also add a cross-file invariant test that task.json and task.html always agree on final_status after every writer, host-side ones included. Why not static: This needs the evaluate pipeline, prior-result inheritance (_terminal_status, is_execution_fact) and the rendered HTML. It is a behavioural property across several modules. Prevents: The grading-denial ERROR that cannot be re-graded, whose own error message tells the user to re-grade (docker_runner.py:829-837), and task.html/task.json status drift.
  • Add a golden test from the probe tasks: feed the proxy log shapes that the criteria in tasks/docker_egress_probe/docker_egress_probe.yaml and docker_egress_grading.yaml produce on purpose (the example.com:443 DENYs and the model host on other ports) through grading_denials and the status rule. Assert that a FAILURE from a real leak or a real grading-allowlist regression stays a FAILURE and is not advised to allowlist example.com. Why not static: Whether a DENY was intended is semantic and depends on what each criterion probes. Only a golden comparison over real log shapes captures it. Prevents: The grading-denial classifier turning every FAILURE with any post-PHASE DENY into ERROR (docker_runner.py:829, egress.py:468-481), which reports the boundary's own leak check as grader infrastructure trouble.
  • Add a DockerDriverConfig validator matrix test. Parametrize every llm_only-only field (egress_allowlist, grading_egress_allowlist, grading_network) across network in {bridge, none, llm_only}, and assert both the refusals and the accepted inverses (including a simulated task with no widening fields). Add a doc-claim check that the combinations DOCKER_ISOLATION.md:162-163 calls 'refused when the task loads' are the ones the matrix refuses. Why not static: Which fields a given mode accepts is validator logic, and whether the prose in the guide matches it is semantic. A lint rule cannot read either relation. Prevents: grading_egress_allowlist being silently ignored under network bridge or none while the docs say it is refused (sandbox.py:384), and an over-broad check_grading_network_without_simulation (tasks.py:691) that no acceptance case pins.
  • Add a predicate-parity property test, or better, delete the duplicates. Over generated DockerDriverConfig combinations, assert that grading_widens_egress equals _resolve_egress_plan(...).switches_for_grading, and that the regrade consent disclosure (regrade.py:289-291) names the same network and allowlist that DockerRunner._container_network and the grade_workspace plan produce. Why not static: Two predicates that are written independently but must agree have to be checked by evaluating both on the same inputs. A static check cannot see that. Prevents: The disagreement between grading_widens_egress and switches_for_grading when every grading host is already in egress_allowlist (sandbox.py:394, egress.py:242), and consent-gate drift between regrade.py:289 and docker_runner.py:786.
  • Add an egress-proxy input fuzz test. Send request lines that carry secrets: absolute-form https with ?key=, userinfo with an invalid port, CONNECT user:pw@host:443, and a host with a 70-character label. Assert that no secret substring reaches egress.log, that the IDNA failure gives a FAIL line and a 502 and not EOF, and that normalize_egress_target agrees with ipaddress.IPv4Address and the RFC 1035 length limits. Why not static: Redaction and error handling are properties of the parsing paths on real inputs. Only running the proxy's handler shows whether a value reaches _log unredacted. Prevents: Unredacted absolute-form and CONNECT targets in egress.log (egress_proxy.py:222-236), the uncaught UnicodeError in the dial (egress_proxy.py:243), and invalid IPv4 or over-long hosts passing the validator (sandbox.py:107).
  • Add a cancellation-semantics test for _run_to_completion: cancel the caller, then make the shielded worker raise (OSError or TimeoutExpired), and assert that the caller gets CancelledError, the warning is logged, and no synthetic egress-setup ERROR task.json is written. Why not static: This is a timing interaction between a cancel and a worker exception that arrives later. It can be seen only under a running event loop. Prevents: A cancelled task recorded as an egress setup failure (egress.py:531-544, which escalates through _checked and docker_runner.py:769-771).
  • Add fail-loud tests for the best-effort egress diagnostics: when docker logs exits non-zero, _record_grading_denials logs a warning and sets an egress_denials_unknown marker in environment_info, and _teardown warns when it skips the egress.log copy. Add a fold-back test parametrized over the rescued tuple in regrade.py so that every entry, grade.egress.log included, is checked automatically. Why not static: A returncode == 0 else None fallback is a value flow and not an except clause, so CE005 cannot see it, and a missing fold-back entry is a filesystem effect. Prevents: The silent fail-open of the denial rule (egress.py:465 to docker_runner.py:822-825), the silent egress.log copy skip at egress.py:513-514, and the untested egress.log fold-back at regrade.py:698.
  • Add an agent-SPI conformance suite run against a fake third-party agent config: egress_hosts() returning a bare host or a URL is either normalized or produces a setup error that names the agent kind, and Pi with an unrecognized provider prefix (for example Bedrock) returns no OpenRouter host and warns, pointing to sandbox.docker.egress_allowlist. Why not static: Third-party plugin code is outside pyright's include path, and a provider-to-host mapping is a domain fact and not a code shape. Prevents: Unnormalized plugin egress targets killing the proxy (egress.py:168), and the silent openrouter.ai:443 fallback for unknown Pi providers (agent_config.py:511).
  • Add a red-team test for the grading switch: the agent touches the request file during its turn, and the test asserts that the host refuses a request without the per-run nonce from the scrubbed context.json, and that environment_info records whether the switch came before the agent-phase end event. Why not static: This is an adversarial runtime behaviour across the container boundary. Which process wrote the file and when is not visible in the source. Prevents: An agent triggering the grading egress switch early by creating the fixed-name request file (docker_runner.py:191, path_utils.py:48). This is a documented, accepted risk, and the test pins the defense-in-depth fix.
  • Add a batch pre-dispatch test: a batch that contains network: llm_only with a non-docker driver must fail once, before any task dispatches. This matches the duplicate-template-dir and check_pricing_coverage checks. Why not static: Whether a check runs before or after dispatch depends on where it sits in run_batch's control flow, and a lint rule has no reliable way to tell a per-task check that should be batch-level. Prevents: An invalid driver/network combination found per task inside run_single (batch.py:196), which gives N in-memory ERROR rows after the batch has started.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no way to seal an agent's network without also sealing its model access, so no-internet tasks fall back to success criteria

3 participants