fix(harbor): require Harbor E2E on PRs, de-flake trajectory_criteria fixture

[akshaylive]

Summary

• The Harbor E2E workflow only ran on push/schedule to main, so the trajectory_criteria scenario added in feat(harbor): unblock trajectory-dependent criteria, always allow credentials at export #186 got no PR signal before merging — it failed on the very first post-merge run (https://github.com/UiPath/coder_eval/actions/runs/35242305732). Add pull_request to its triggers and add the job as a required status check on main.
• Reproduced the failure twice (identical reward=0.5, command_executed not matched): the fixture asked the agent to run touch done.txt a second time purely "to confirm it exists," which a capable agent can reasonably skip since it's redundant. Reworded the prompt so running touch is the only way the file gets created, removing the ambiguity.
• harbor_e2e.py only printed each criterion's own score/details when the aggregate reward was already 1.0 (i.e. only for a "passed by luck" case) — reordered so any reward mismatch also surfaces the per-criterion breakdown, since that's exactly what was missing from the original failure logs.

Test plan

• make format / make check / make test all pass
• Applied the required-check change to the Protect main ruleset (Harbor E2E job now required)
• Watch this PR's own Harbor E2E run pass now that it's wired to pull_request

[uipreliga]

Review: coder_eval — pr:187 (6 files) axis:1,2,3,4,5,6,7,8

Scope: pr:187 (6 files) axis:1,2,3,4,5,6,7,8 · branch akshaya/harbor_trajectory_fix · 59d32a2 · 2026-09-17T18:48Z · workflow variant

Change class: complex — adds a new re-provisioning control-flow branch to Sandbox.adopt() (an isolation-sensitive path whose whole design is 'non-materializing steps only'), and changes CI trigger semantics by making a real-Docker/real-model E2E job a required pull_request gate; both need correctness reasoning, not a glance.

The architecture, API surface and type discipline remain excellent (9.9/9.9/9.4) and the underlying adopt() re-provisioning fix addresses a real, live-confirmed wrong-verdict bug, but the PR ships that fix by teaching the grading path to run installers from an untrusted recorded config while leaving the consent gate — and the test that asserts its exemption — untouched, with no rollback on a failed install, no fork guard on a newly-required pull_request workflow, and rationale notes that credit the fix with a Harbor failure it provably cannot reach; the bottom line is that the code change is right in spirit but must not merge until the _gate_scope_for_grade premise, the packager's missing verifier sandbox: block, and the failure-path cleanup are resolved.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9 / 10	0	0	2	0	assert_scenario_artifacts complexity pushed C(12)→C(17) by inlining a diagnostics block that duplicates three file reads/globs the function already performs ~37 lines later
2. Type Safety	9.4 / 10	0	0	1	1	New CI-script code becomes a required PR gate while .github/scripts/ sits outside the pyright pass (and the pytest pass) that Makefile:18-21 claims covers it
3. Test Health	8 / 10	0	1	2	0	adopt()'s new Node re-provisioning arm (sandbox.py:330-331) is untested across the whole suite while it rewrites package.json + a lockfile in the graded tree; the "existing venv is never rebuilt" inverse is also unpinned
4. Security	7 / 10	1	0	0	0	harbor-e2e.yml gains an unconditional pull_request trigger with no fork carve-out: fork code would run on the shared self-hosted uipath-ubuntu-latest pool with Bedrock secrets, and the required gate can never go green without those secrets
5. Architecture & Design	9.9 / 10	0	0	0	1	The Python and Node re-provisioning branches drift in shape (a 115-char triple-and line re-checking conditions the callee enforces) and "node_modules" is a bare literal beside VENV_DIRNAME
6. Error Handling & Resilience	7.5 / 10	0	2	1	0	adopt() creates .venv/node_modules in a caller-owned workspace with no cleanup on a failed install, so a half-provisioned tree latches and the retry silently takes the discover branch
7. API Surface & Maintainability	9.9 / 10	0	0	0	1	FAILURE_ARTIFACTS_DIR is coupled to the workflow's upload path by an unenforced string literal that fails silently
8. Evaluation Harness Quality	5.4 / 10	1	1	1	1	adopt() now runs uv pip install / npm install from the untrusted recorded config on the in-place evaluate <run_dir> path, which _gate_scope_for_grade exempts from the --allow-recorded-commands disclosure (and its include_setup_phase = not grade_in_place premise that "adopt runs no installer" is now false)

Overall Score: 8.3 / 10 · Weakest Axis: Evaluation Harness Quality at 5.4 / 10
Totals: 🔴 2 · 🟠 4 · 🟡 7 · 🔵 4 across 8 axes.

Blockers

1. [Axis 3] adopt()'s new Node re-provisioning arm (sandbox.py:330-331) is untested across the whole suite while it rewrites package.json + a lockfile in the graded tree; the "existing venv is never rebuilt" inverse is also unpinned (src/coder_eval/sandbox.py:331) — The PR adds one new conditional to the public Sandbox.adopt() (sandbox.py:322-331) and one new test. Two arms ship untested:

(a) Node arm — tool-confirmed dead in test: src/coder_eval/sandbox.py:330-331

        if self.config.node and self.config.node.env_packages and not (self.sandbox_dir / "node_modules").is_dir():
            self._install_node_packages()

The routed coverage run lists 331 verbatim in sandbox.py's missing lines (tmp/code-review-260917-1149/automated/pytest.txt:238: ... 257-265, 331, 483, ...), i.e. line 330's condition is evaluated but never taken. It is structurally unreachable from this test file: the only workspace factory, tests/test_sandbox_adopt.py:21-29, always creates (ws / "node_modules" / "pkg").mkdir(parents=True), so not (... / "node_modules").is_dir() is False for every existing adopt test. The callee body is uncovered too (same coverage line lists 843-869, which is _install_node_packages's body starting at sandbox.py:841), so nothing in the suite exercises adopt → node install end to end.

(b) Inverse of the new guard not asserted (Technique 5): .claude/notes/isolation.md states "A venv that already exists (the ordinary preserve_to / non-captured path) is still only discovered, never rebuilt." No test pins that. test_adopt_discovers_an_existing_venv_without_creating_one (tests/test_sandbox_adopt.py:75-82) plants .venv/bin but declares python={"env_packages": []}, so the interesting combination — an existing .venv plus non-empty env_packages — never runs; a future edit turning elif self.config.python.env_packages: (line 326) into an unconditional re-provision would rebuild a graded workspace's venv with no failing test.

Fix (cheap, hermetic, matches the file's siblings): add two mock-based tests to tests/test_sandbox_adopt.py — one with a workspace that has no node_modules and node={"env_packages": ["left-pad"]} asserting _install_node_packages was called (and a second with node_modules present asserting it was NOT), and one with .venv/bin planted plus python={"env_packages": ["requests"]} asserting _setup_virtualenv/_install_packages were NOT called. patch.object(sandbox, "_install_node_packages") is already the established idiom here — see tests/test_node_env_config.py:65 and :124.
2. [Axis 4] harbor-e2e.yml gains an unconditional pull_request trigger with no fork carve-out: fork code would run on the shared self-hosted uipath-ubuntu-latest pool with Bedrock secrets, and the required gate can never go green without those secrets (.github/workflows/harbor-e2e.yml:16) — UiPath/coder_eval is PUBLIC (gh repo view --json visibility → "PUBLIC"), so anyone can open a fork PR. The diff adds, at harbor-e2e.yml:16-17:

  pull_request:
    branches: [main]

while line 39 is an unguarded runs-on: uipath-ubuntu-latest. For a pull_request event GitHub runs the workflow definition and scripts from the PR head, so a fork PR's own .github/workflows/harbor-e2e.yml, .github/scripts/harbor_e2e.py, uv.lock, docker/Dockerfile (make docker-image, line 87) and templates/byod_smoke_test/ (docker build, line 90) all execute on the shared org pool — with Docker daemon access and, on a non-ephemeral runner, persistence into the next push:/schedule: run, which DOES carry AWS_BEARER_TOKEN_BEDROCK (lines 32-34).

This is not a theoretical bar: the repo already states and enforces the rule next door. .github/workflows/pr-checks.yml:40-43:

    # Fork PRs go to stock GitHub-hosted runners: this job runs the PR's own
    # `uv.lock` build hooks and test files, and the repo is public, so untrusted code
    # must not land on the shared pool image. Everything else uses the pool.
    runs-on: ${{ (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) && 'ubuntu-latest' || 'uipath-ubuntu-latest' }}

harbor-e2e.yml now takes the same untrusted input with none of that protection, and its steps are strictly more privileged (two docker builds vs. pytest).

Fix, in order of preference:

1. Gate the job on same-repo PRs only — if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository — since the job cannot pass on a fork anyway (fork PRs receive no secrets, so AWS_BEARER_TOKEN_BEDROCK is empty and the real model call fails). A required check that is guaranteed to fail for every external contributor is its own problem; skipping is the honest outcome.
2. Or copy pr-checks.yml:43's ternary verbatim so fork PRs land on ubuntu-latest.
3. Independently: move AWS_BEARER_TOKEN_BEDROCK/AWS_REGION/BEDROCK_MODEL off the workflow-level env: (lines 26-34) and onto only the Run Harbor E2E scenarios step (line 92). Today they are in the environment of npm install -g @anthropic-ai/claude-code (line 57), pip install uv (line 72), uv sync (line 81) and both docker builds — every unpinned third-party install step. That pre-dates this PR, but the new trigger multiplies how often it runs and who can influence what runs there. CVSS:3.1/AV:N/AC:L/PR:N/UI:R/S:C/C:H/I:H/A:H
4. [Axis 6] adopt() creates .venv/node_modules in a caller-owned workspace with no cleanup on a failed install, so a half-provisioned tree latches and the retry silently takes the discover branch (src/coder_eval/sandbox.py:326) — adopt() now writes provisioning state into a directory it does not own, with no cleanup on failure and no staleness check on reuse:

            elif self.config.python.env_packages:
                self._setup_virtualenv()      # line 327 -- creates <workspace>/.venv
                self._install_packages()      # line 328 -- may raise RuntimeError

        if self.config.node and self.config.node.env_packages and not (self.sandbox_dir / "node_modules").is_dir():
            self._install_node_packages()     # line 331

Two concrete failures:

1. Partial state latches. _setup_virtualenv (line 795: self.venv_dir = self.sandbox_dir / VENV_DIRNAME) creates .venv first; _install_packages then raises RuntimeError(f"Failed to install packages: {e.stderr}") (line 839) on a network/bad-package failure. Nothing removes the half-built .venv: _cleanup_on_exit is False by contract (line 307) and --resume deliberately does not clear a to-grade row's artifacts (run_command.py: "to_grade is deliberately NOT cleared: its artifacts are what is being graded"). On the operator's retry, line 324 if candidate.is_dir(): self.venv_dir = candidate takes the discover branch — grading proceeds against a venv with none of env_packages installed, which is precisely the silent-wrong-verdict this PR exists to fix, now permanent instead of transient. Same for node_modules after a partial npm install.

2. Stale reuse across grades. No failure is even needed: grade #1 plants .venv with env_packages: [requests]; the task YAML/variant later declares [requests, pyyaml]; grade #2 hits line 324 and discovers the old venv, so the second verdict is computed against the first grade's package set. Before this PR the grading path never wrote a .venv, so this stale-artifact class could not arise from coder_eval's own writes. (Cross-axis: this is the axis-8 "stale artifacts surviving a re-run task" blocker shape.)

Note the parallel path already gets this right: setup()'s identical _setup_virtualenv()/_install_packages() pair sits inside try: ... except Exception: at sandbox.py:395-405, which tears the tree down and re-raises.

Fix: wrap the new block so a failed provision leaves nothing behind — try: self._setup_virtualenv(); self._install_packages() except Exception: shutil.rmtree(self.venv_dir, ignore_errors=True); self.venv_dir = None; raise (and the equivalent for node_modules) — and, for the stale case, either record the resolved env_packages alongside the venv and rebuild on mismatch, or provision into a grader-owned directory outside the adopted workspace. Also update the now-false docstring at lines 268 and 275-276 ("materializing nothing into it", "it writes no FILES"), and add an error-path test next to test_adopt_reprovisions_env_packages_missing_from_a_captured_workspace (tests/test_sandbox_adopt.py:91) asserting no .venv survives a failed install.
4. [Axis 6] adopt()'s new re-provisioning can raise subprocess.SubprocessError (TimeoutExpired, and CalledProcessError from the stdlib-venv fallback), which neither grading caller catches — one row's failure aborts the whole --resume batch (src/coder_eval/sandbox.py:328) — The new self._install_packages() (line 328), self._setup_virtualenv() (line 327) and self._install_node_packages() (line 331) run network subprocesses on the GRADING path for the first time. Their own handlers only wrap CalledProcessError:

• sandbox.py:837-839: subprocess.run(cmd, ..., timeout=300, env=env) inside except subprocess.CalledProcessError as e: raise RuntimeError(...) — a hung index leaves the 300s timeout to raise subprocess.TimeoutExpired, which is not wrapped.
• sandbox.py:801-805: uv venv with timeout=60 under except (subprocess.CalledProcessError, FileNotFoundError) — same gap; the fallback venv.create(..., with_pip=True) (line 811) raises a bare subprocess.CalledProcessError from ensurepip.
• sandbox.py:855-866: _install_node_packages repeats the shape with timeout=300.

subprocess.TimeoutExpired.__mro__ is (TimeoutExpired, SubprocessError, Exception, ...) — verified locally: it is neither OSError nor RuntimeError nor the builtin TimeoutError. So it slips past both callers of adopt():

• cli/run_command.py:817: except (RegradeError, OSError, RuntimeError, ValueError) as e: — the handler whose own comment (line 793) says "outside it, one bad task.json aborts the whole resume before run_batch, so none of the to_run tasks execute either". A 300s-timeout install on ONE row now aborts the entire resume, including every task still queued to execute.
• cli/evaluate_command.py:490-491: result = asyncio.run(_setup_and_run()) / except RegradeError as e: — here even the ordinary wrapped RuntimeError("Failed to install packages: ...") escapes, so any install failure prints a raw traceback instead of the ✗ <message> rendering that handler exists for ("unwrapped, these operator-facing messages arrived as the tail of a stack trace").

Fix: wrap the new block in adopt() so every provisioning failure leaves as one declared class — catch subprocess.SubprocessError (or Exception) around lines 327-331 and re-raise as RuntimeError(f"Could not re-provision env_packages for {self.sandbox_dir}: ...") — and add RuntimeError (or RegradeError) coverage to evaluate_command.py:491 so a grade-time provisioning failure degrades to a named operator error rather than a traceback. Fixing the wrapping inside _install_packages/_install_node_packages/_setup_virtualenv themselves (except (subprocess.CalledProcessError, subprocess.TimeoutExpired)) fixes both call sites at once.
5. [Axis 8] adopt() now runs uv pip install / npm install from the untrusted recorded config on the in-place evaluate <run_dir> path, which _gate_scope_for_grade exempts from the --allow-recorded-commands disclosure (and its include_setup_phase = not grade_in_place premise that "adopt runs no installer" is now false) (src/coder_eval/sandbox.py:326) — The in-place grading gate was built on the premise that adopt installs nothing, and this PR falsifies it without touching the gate. src/coder_eval/orchestration/regrade.py:132-134 states the premise verbatim: "include_setup_phase is not grade_in_place. pre_run and the sandbox's own provisioning exist only on the --copy path; adopt runs no installer and the orchestrator skips pre_run, so in place they are not a capability the run dir has." embedded_commands puts exactly these commands behind that flag (regrade.py:232-238: if include_setup_phase: ... commands.append(f"uv pip install {' '.join(sandbox.python.env_packages)}")). And tests/test_detached_grading_boundaries.py:804-811 now LOCKS THE HOLE IN: """adopt() installs nothing, so in place these are not capabilities the run dir has...""" followed by task = self._task_with(python=PythonEnvConfig(env_packages=["attacker-pkg"])) / check_embedded_commands(task, tmp_path, allow_recorded_commands=False, include_setup_phase=False) — asserting NO refusal. Full reachable chain, no operator opt-in anywhere: coder-eval evaluate <shared_run_dir> -> _resolve_run_dir_or_work_dir (cli/evaluate_command.py:162) -> task_from_prior(..., allow_recorded_commands=False, grade_in_place=True) -> _gate_scope_for_grade returns include_setup_phase=False -> gate passes -> regrade_in_place -> grading_sandbox_config returns the recorded sandbox block verbatim (regrade.py:603 return task.sandbox.model_copy(deep=True)) -> sandbox.adopt(workspace) (regrade.py:990) -> the new elif self.config.python.env_packages: self._setup_virtualenv(); self._install_packages() -> _install_packages builds cmd = ["uv", "pip", "install", *self.config.python.env_packages] (sandbox.py:827) with entries the attacker chose in task.json's task_config.resolved. An attacker who shares a run directory omits .venv from it (they control its contents, and capture_to strips .venv anyway) and gets arbitrary code execution on the grader's host at install time (PEP 517 build hooks, npm postinstall), or simply injects --index-url/a URL/-e git+... since the list is splatted into argv unquoted. Fix: either exclude env_packages re-provisioning when the task came from an untrusted recorded config (thread the trust decision into Sandbox/grading_sandbox_config), or flip _gate_scope_for_grade to include_setup_phase=True unconditionally and update regrade.py:132-134 and test_the_in_place_path_is_exempt to match. Do not ship the code change while that test still asserts the exemption.
6. [Axis 8] The note credits the adopt() re-provisioning fix with a Harbor E2E failure it cannot reach: the exported tests/task.yaml carries no sandbox: block, so the verifier grades with env_packages == [] (.claude/notes/isolation.md:862) — The note claims (line 862-865): "Confirmed live: a Harbor E2E scenario's run_command criterion failed No module named pytest against a workspace whose agent phase had run pytest successfully moments earlier." That scenario is template_sources (the only fixture with env_packages: [pytest] plus command: python -m pytest tests/), and the fix cannot reach it. _write_verifier_task_yaml (src/coder_eval/harbor/packager.py:342-361) builds payload from exactly task_id, description, agent, initial_prompt, success_criteria (+ optional reference, run_limits, checker_context) — there is no "sandbox" key — so the verifier's /tests/task.yaml gets the default SandboxConfig, i.e. python.env_packages == []. I verified this by running coder-eval export tests/harbor_e2e/fixtures/template_sources.yaml: the emitted tests/task.yaml has no sandbox: key, while environment/task.yaml (agent phase) does carry python: env_packages: [pytest]. test.sh grades with that file (coder-eval evaluate /tests/task.yaml /logs/agent --in-place, packager.py:79), and an explicit task file wins over the recorded config (evaluate_command.py:154-156), so self.config.python.env_packages is empty at sandbox.py:326 and the new elif is dead on this path. The only in-tree coverage of the agent/verifier asymmetry is tests/test_harbor_packager.py:583 test_agent_phase_sandbox_preserves_python_and_limits — the agent phase only. Either carry sandbox.python/sandbox.node into _write_verifier_task_yaml (and add the packager test), or correct the note: as written it tells the next reader this class of failure is handled when the CI scenario that produced it is not.

Non-blocking, but please consider before merge

1. [Axis 1] assert_scenario_artifacts complexity pushed C(12)→C(17) by inlining a diagnostics block that duplicates three file reads/globs the function already performs ~37 lines later (.github/scripts/harbor_e2e.py:124) — Verified with uv run radon cc -s: F 124:0 assert_scenario_artifacts - C (17) at the PR head vs F 118:0 assert_scenario_artifacts - C (12) on main — the PR added 5 to the single most complex function in the file, and it is now the same function that both asserts and gathers diagnostics. The added block duplicates work the function already did below it, three times over: verifier_task_json = trial_dir / "verifier" / "task.json" appears at line 134 and again identically at line 174; verifier_result = json.loads(verifier_task_json.read_text(encoding="utf-8")) appears at line 137 and again at line 182 (the same file parsed twice per scenario); and agent_task_jsons = sorted((trial_dir / "agent").glob("**/task.json")) at line 150 duplicates agent_task_jsons = list((trial_dir / "agent").glob("**/task.json")) at line 171, differing only in sorted vs list. The block at 130-141 also runs unconditionally on every green scenario to build a string used only inside the if reward.get("reward") != 1.0: branch at 143 — and its justifying comment ("Read the per-criterion breakdown BEFORE the reward gate below, so a failing reward's own root cause ... is always in the failure message") is not a reason: the failure branch can read it itself. Fix: hoist verifier_task_json / verifier_result / agent_task_jsons to single reads at the top of the function, and extract the two diagnostic formatters into module-level helpers (e.g. _criteria_detail(verifier_result) and _agent_recorded_commands(agent_task_json)) called only from the failure branch — that removes the duplication and drops the function back toward its pre-PR grade. While there, replace the sentinel union agent_commands: list[dict[str, object]] | str = "no agent/task.json found" (line 151) with a plain list plus a separate absence note, so the variable has one type.
2. [Axis 1] .claude/notes/isolation.md:867 justifies adopt()'s env_packages gate with an "asking for packages earns a venv" rule that setup() does not actually have (.claude/notes/isolation.md:867) — Lines 866-868 read: gated on \env_packages`, not bare `config.python`, so the common empty-`env_packages` default still adopts as a pure no-op (matching `setup`'s own "asking for packages is what earns a venv" rule). setup()has no such rule. src/coder_eval/sandbox.py:377-383 isif self.config.python:→self._setup_virtualenv()unconditionally, and only thenif self.config.python.env_packages: self._install_packages()— i.e. bareconfig.pythonDOES earn a venv insetup; only the *install* is gated on env_packages. The note therefore claims parity where the PR actually introduces a divergence: for python: {env_packages: []}with a captured workspace,setupyields a venv (and a populatedvenv_dir/VIRTUAL_ENVon the criterion PATH) whileadoptnow yields none. CLAUDE.md makes.claude/notes/the thing a maintainer must read before changing this code, so a false claim here is the worst place for one. Fix: state the divergence instead of denying it — e.g. "gated onenv_packages, unlike setup, which creates a venv for bare config.python; the empty-env_packagesadopt path deliberately stays a no-op because there is nothing to install and an empty venv would only shadow the interpreter" — and note the consequentVIRTUAL_ENV` asymmetry so the next reader does not rediscover it as a bug.
3. [Axis 2] New CI-script code becomes a required PR gate while .github/scripts/ sits outside the pyright pass (and the pytest pass) that Makefile:18-21 claims covers it (.github/scripts/harbor_e2e.py:n/a) — This PR does two things at once: .github/workflows/harbor-e2e.yml adds pull_request:\n branches: [main] (making this script block every PR, per its own new comment "Required on every PR"), and it adds +61 lines to harbor_e2e.py — the most type-loose code in the file (three untyped .get() walks over Any from json.loads, one hand-written list[dict[str, object]] | str annotation, one -> Path | None).

No type checker covers that file. pyproject.toml [tool.pyright] declares include = ["src/coder_eval"], and Makefile:71 (inside verify:) runs a bare uv run pyright, so .github/scripts/ is never analyzed. Makefile:18-22 reads the other way and is what a contributor will believe:

# `.github/scripts/` is in scope on purpose: release tooling that lives in a real
# module (rather than inline in a workflow `run:` heredoc) is exactly what ruff,
# pyright and pytest can see — leaving it unlinted would forfeit the reason it was
# extracted.
LINT_PATHS := src/ tests/ .github/scripts/

LINT_PATHS feeds only ruff format (Makefile:25/69) and ruff check (Makefile:28/70). The typecheck: target (Makefile:45-55) never references it.

The fix is free today: uv run pyright --outputjson .github/scripts/ returns {'filesAnalyzed': 2, 'errorCount': 0, 'warningCount': 0} — both harbor_e2e.py and release_notes.py already pass. Add ".github/scripts" to [tool.pyright] include (or give the directory its own derived config the way typecheck: already does for the CE036 engine via .pyright-tests.json, which exists precisely because pyright's exclude beats a CLI file arg). If that is deliberately not wanted, correct the Makefile comment so it stops claiming pyright coverage the gate does not have.

Severity calibration: the Axis-2 Low anchor is "missing types on private helpers in cold paths"; this is filed one step up because as of this PR the file is no longer a cold path — it gates merges — and the repo's own documentation asserts a static gate that does not exist. No live defect exists today (the file type-checks clean), which is why it is not higher.
4. [Axis 3] The Harbor E2E fixtures are never parsed offline, yet this PR makes the live Docker+model run a required PR gate (tests/harbor_e2e/fixtures/trajectory_criteria.yaml:40) — This PR edits the fixture's criterion (tests/harbor_e2e/fixtures/trajectory_criteria.yaml:40):

    command_pattern: "touch\\s+\\S*done\\.txt"

and .github/workflows/harbor-e2e.yml adds pull_request: branches: [main], so a malformed fixture now blocks every PR. Nothing under tests/ validates these fixtures offline: grep -rn "harbor_e2e" tests/ matches only the fixture files themselves (no test module references them), and every YAML-validating test globs tasks/ exclusively — tests/test_yaml_migration.py:25 ((ROOT / "tasks").rglob("*.yaml")), tests/test_tags.py:137, tests/test_custom_lint.py:2960 and :4642. A bad key, a dropped required field, or an uncompilable regex therefore surfaces only after a full Docker-build + real-model CI round trip; note command_executed swallows a bad regex into a 0.0 score rather than raising (src/coder_eval/criteria/command_executed.py:270-276, details=f"Invalid command_pattern regex: {e}"), so the CI symptom is an opaque reward != 1.0, not a parse error.

Add a ~10-line offline test (e.g. tests/test_harbor_e2e_fixtures.py) parametrized over tests/harbor_e2e/fixtures/*.yaml that does TaskDefinition.model_validate(yaml.safe_load(...)) and re.compile()s every command_pattern/exclude_pattern. I ran exactly that against the PR HEAD and all four fixtures validate today (docker_baseline.yaml OK ['file_exists','file_contains','run_command'], trajectory_criteria.yaml OK ['file_exists','command_executed'], …), so the test lands green and is pure guard.

Separately, while editing: \S*done\.txt also matches touch notdone.txt / touch xdone.txt. touch\s+(\S*/)?done\.txt keeps the absolute-path tolerance the comment argues for (touch /work/done.txt) without loosening the filename itself.
5. [Axis 3] harbor_e2e.py's new failure-only agent-command dump does an unguarded json.loads (line 153) that can replace the reward/criteria root-cause message it exists to produce, and none of the ~61 new lines has any test (.github/scripts/harbor_e2e.py:153) — The PR adds ~60 lines that run only when a scenario has already failed, with no test anywhere (grep -rn "harbor_e2e" tests/ matches no test module) and no coverage instrumentation (the file is outside --cov=coder_eval; it does not appear in tmp/code-review-260917-1149/automated/pytest.txt at all). That is the class of code that breaks precisely when it is needed. One concrete defect is visible on a read:

        agent_task_jsons = sorted((trial_dir / "agent").glob("**/task.json"))
        agent_commands: list[dict[str, object]] | str = "no agent/task.json found"
        if agent_task_jsons:
            agent_result = json.loads(agent_task_jsons[0].read_text(encoding="utf-8"))   # line 153

Failure scenario: reward is 0.0 and the agent phase died mid-write leaving a truncated agent/**/task.json (exactly the crash shape this script exists to diagnose). json.loads raises JSONDecodeError, which nothing here catches; main()'s except Exception at line 231 then prints [name] FAILED: Expecting value: line 1 column 1 (char 0) and the reward/criteria root cause the new code was added to surface is lost. The same applies to the pre-gate read at line 137 (json.loads(verifier_task_json.read_text(...))), which now runs on the success path too and can turn a passing-but-odd verifier record into a decode error. The sibling helper already states the right rule and honours it — _zip_scenario_dir (lines 202-216): "Best-effort: a zip failure must never mask the real scenario failure it was trying to preserve evidence for", with except OSError.

Fix: wrap each diagnostic read in try/except Exception and fall back to a string (f"unreadable: {exc}"), so the diagnostic can never outrank the failure it describes. Then extract the two summarizers into pure helpers (_criteria_detail(path) / _agent_commands(path)) taking a Path and returning list | str, and unit-test them under tests/ against a truncated file, a missing file, and a well-formed one — they are plain JSON reshaping with no Docker or Harbor dependency, so they are testable offline even though the driver as a whole is not.
6. [Axis 6] _zip_scenario_dir can raise past its own try, killing main()'s scenario loop from inside the except handler (.github/scripts/harbor_e2e.py:211) — The docstring at lines 205-206 promises "Best-effort: a zip failure must never mask the real scenario failure it was trying to preserve evidence for", but two escape routes break it:

211:    FAILURE_ARTIFACTS_DIR.mkdir(parents=True, exist_ok=True)   # OUTSIDE the try
212:    try:
213:        archive = shutil.make_archive(str(FAILURE_ARTIFACTS_DIR / scenario.name), "zip", root_dir=str(scenario_dir))
214:    except OSError as exc:

1. The mkdir is outside the try (low likelihood — WORK_DIR.mkdir at line 221 already proved tmp/ writable — but it is on the wrong side of the guard for free).
2. except OSError is too narrow for shutil.make_archive. Verified locally against this machine's CPython: a single file in the tree with an mtime before 1980 makes it raise ValueError('ZIP does not support timestamps before 1980') — MRO (ValueError, Exception, ...), not an OSError. The tree being zipped is jobs/ plus container-captured artifacts/ workspaces, where epoch-0 mtimes from image layers are entirely ordinary. UnicodeEncodeError on an undecodable filename from the same trees escapes identically.

Because the call site is inside the handler — except Exception as exc: at line 231, archive = _zip_scenario_dir(scenario) at line 234 — an escape there propagates out of the for scenario in SCENARIOS loop: the remaining scenarios never run and the === Summary === block (lines 238-240) never prints. On a check that is now required on every PR, a first-scenario failure would hide the state of the other four for a full ~20-minute CI round trip. (The original error itself survives, because line 232 prints it before the zip is attempted.)

Fix: move the mkdir inside the try and widen to except Exception as exc: — a genuinely best-effort evidence path is the one place a blanket catch is correct, and the docstring already states that intent. This file is outside --cov=coder_eval and has no unit test, so a small tests/ case (or a strict_timestamps=False zipfile walk) is the only thing that would catch a regression here.
7. [Axis 8] adopt()'s docstring still promises "materializing nothing" / "writes no FILES" (sandbox.py:268, 276-278) while the function now provisions .venv/ and node_modules/; regrade.py and .claude/notes/isolation.md repeat the stale guarantee (src/coder_eval/sandbox.py:268) — Line 268 still reads """Use ``workspace`` **as** the sandbox, materializing nothing into it. and line 276-278 still reads "Materializing nothing" means it writes no FILES; it does still chmod ``+x`` over the task's declared mock-PATH directories. After this PR adopt writes a whole .venv/ (and node_modules/ + package.json/lockfile) into the graded tree. CLAUDE.md is explicit that "A docstring states the contract", and the in-body comment at lines 310-314 was updated while the docstring above it was not. This is not cosmetic: the directory adopt now writes into is the caller's — cleanup() never deletes it (_cleanup_on_exit stays False, line 307), PreservationMode.NONE is passed precisely because "the workspace belongs to the run being graded; never move or delete it" (regrade.py:987-988), and on the container route it is a read-WRITE bind mount of the original run's output (argv += ["-v", f"{self.grade_workspace.resolve()}:{CONTAINER_GRADE_WORKSPACE}"], docker_runner.py:1318) whose own docstring says "the latter is adopted and never written over" (regrade.py:797). So a grade now permanently mutates the artifact directory of the run it is grading. Update the docstring to state exactly which files adopt may create and under what condition, and fix regrade.py:797.

Nits

Omitted here for size — 4 🔵 findings are in the full report (tmp/code-review-260917-1149/00-summary.md).

What's Missing

Parallel paths:

• 🔴 The repo's fork-PR runner convention was applied in nine other places but not here: .github/workflows/pr-checks.yml carries the head.repo.full_name != github.repository ternary at lines 43/202/294 and the skip-if: at 347/469/677/849/927/1009, and claude-pr-review.yml:36-39 guards the same way — after this PR harbor-e2e.yml is the only pull_request-triggered workflow with neither, while running two docker builds and the PR's own .github/scripts/harbor_e2e.py on the shared uipath-ubuntu-latest pool. (trigger: .github/workflows/harbor-e2e.yml) (restates: Axis 4: harbor-e2e.yml gains an unconditional pull_request trigger with no fork carve-out)
• 🟠 src/coder_eval/harbor/packager.py:338-364 (_write_verifier_task_yaml) is the parallel path the fix needs and was not touched — the verifier's /tests/task.yaml carries no sandbox: key, so env_packages is [] at grading time and the new elif is dead on exactly the template_sources scenario .claude/notes/isolation.md:862 cites as the live proof. (trigger: src/coder_eval/sandbox.py) (restates: Axis 8: The note credits the adopt() re-provisioning fix with a Harbor E2E failure it cannot reach)
• 🟡 Only the in-body comment at sandbox.py:310-314 was updated; the three other statements of the old contract were not — the docstring at sandbox.py:268 and 276-278, orchestration/regrade.py:799 ("the latter is adopted and never written over"), and .claude/notes/isolation.md:276 ("adopt() guarantees it materializes nothing into the workspace"), the last of which is in an in-scope file the PR edited further down. (trigger: src/coder_eval/sandbox.py) (restates: Axis 8: adopt()'s docstring still promises "materializing nothing" / "writes no FILES")
• 🟡 setup() (sandbox.py:378-383) was left as-is, so the two provisioning paths now diverge — bare config.python earns a venv in setup but not in adopt — and nothing reconciles the resulting VIRTUAL_ENV / PATH asymmetry (_command_env, sandbox.py:1103-1105) for a python: {env_packages: []} task graded from a captured workspace. (trigger: .claude/notes/isolation.md) (restates: Axis 1: isolation.md:867 justifies adopt()'s env_packages gate with an "asking for packages earns a venv" rule that setup() does not actually have)
• 🟡 User-facing docs were not updated for a behaviour change operators will feel: docs/USER_GUIDE.md:266-275 still describes in-place grading purely as "grade where the files are" with "an adopted directory is never moved or deleted", and says nothing about a grade now running uv pip install / npm install — i.e. needing network, taking install-time, and being able to fail the grade outright. (trigger: src/coder_eval/sandbox.py)

Tests:

• 🟠 The one new test does not actually pin the new behaviour: _setup_virtualenv builds with --system-site-packages (sandbox.py:803/811) and requests is importable in the project env (verified: 2.33.0), so python -c "import requests" succeeds even if _install_packages() were deleted from adopt — the test survives the mutation it exists to catch. Assert against a package absent from the host env, or assert the venv's own site-packages gained it. (trigger: tests/test_sandbox_adopt.py)
• 🟠 The node half of the new rule (sandbox.py:330-331) ships with no test at all — the full 6433-item run lists line 331 and _install_node_packages's body (843-869) as never executed, and _workspace() (tests/test_sandbox_adopt.py:21-29) always creates node_modules/, so no adopt test can ever take the branch. (trigger: src/coder_eval/sandbox.py) (restates: Axis 3: adopt()'s new Node re-provisioning arm (sandbox.py:330-331) is untested across the whole suite)
• 🟡 No hermeticity marker was added for the suite's new network dependency: test_adopt_reprovisions_env_packages_missing_from_a_captured_workspace does a real uv venv + PyPI uv pip install requests and is unmarked, so make test (-m "not live and not lint") now fails offline — every other env_packages test in the repo patches the installer instead (tests/test_node_env_config.py:53/66/79/126). (trigger: tests/test_sandbox_adopt.py)
• 🟡 The no-op half of the new gate is unasserted: nothing exercises python: {env_packages: []} against a workspace with no .venv, so the note's central claim ("the common empty-env_packages default still adopts as a pure no-op") has no test — test_adopt_writes_nothing_into_the_workspace (tests/test_sandbox_adopt.py:56) uses the default config and passes trivially. (trigger: tests/test_sandbox_adopt.py)
• 🟡 No error-path test accompanies the new writes: nothing asserts what the graded tree looks like after _install_packages raises RuntimeError (sandbox.py:839) — the case where a half-built .venv latches and the next adopt silently takes the discover branch. (trigger: src/coder_eval/sandbox.py) (restates: Axis 6: adopt() creates .venv/node_modules in a caller-owned workspace with no cleanup on a failed install)
• 🟡 The fixture edit lands with no offline guard: nothing under tests/ validates tests/harbor_e2e/fixtures/*.yaml (every YAML-parsing test globs tasks/ only), and command_executed degrades a bad regex to score=0.0 (criteria/command_executed.py:269-279), so a malformed fixture now surfaces as an opaque reward != 1.0 after a ~20-minute build on a required check. (trigger: tests/harbor_e2e/fixtures/trajectory_criteria.yaml) (restates: Axis 3: The Harbor E2E fixtures are never parsed offline, yet this PR makes the live Docker+model run a required PR gate)
• 🟡 None of the +60 new lines has a test or any coverage instrumentation (the file is outside --cov=coder_eval and absent from the coverage report), although the sibling .github/scripts/release_notes.py already has tests/test_release_notes.py importing it by path — the precedent for unit-testing this directory exists and was not followed. (trigger: .github/scripts/harbor_e2e.py) (restates: Axis 3: harbor_e2e.py's new failure-only agent-command dump does an unguarded json.loads that can replace the root-cause message it exists to produce)

Downstream consumers:

• 🔴 Every consumer of the invariant this PR falsified was left asserting it: orchestration/regrade.py:133-136 still documents "adopt runs no installer" as the reason _gate_scope_for_grade returns include_setup_phase=False, regrade.py:232-238 still hides uv pip install {env_packages} behind that flag, and tests/test_detached_grading_boundaries.py:804-811 now actively locks the exemption in with env_packages=["attacker-pkg"]. (trigger: src/coder_eval/sandbox.py) (restates: Axis 8: adopt() now runs uv pip install / npm install from the untrusted recorded config on the in-place evaluate <run_dir> path)
• 🟡 Repeat in-place grading of the same run directory is no longer idempotent and nothing accounts for it: the first evaluate <run_dir> writes .venv (and node_modules + package.json/lockfile) into run_dir/artifacts/<task_id>/, so the second grade of the same directory takes the discover branch instead of the install branch, and any preserved run dir that is archived, copied or shipped downstream now carries a venv it never used to. (trigger: src/coder_eval/sandbox.py)

Display & mapping dicts:

• 🔵 The new criteria_detail projection (harbor_e2e.py:138-141) covers only criterion_type / score / details, while a serialized criterion result also carries error, evaluation_status and result_kind — so a criterion that raised or came back NOT_GRADED prints details: None, which is precisely the case the dump was added to disambiguate. (trigger: .github/scripts/harbor_e2e.py)

Daily/nightly:

• 🟠 The PR promotes a live Docker + real-Bedrock workflow (5 scenarios, two image builds, timeout-minutes: 20) to a per-PR required gate on the shared self-hosted pool without stating the blast radius: pool contention and Bedrock spend per PR are unquantified, and the nightly's own stated purpose — catching drift in Harbor's upstream releases — now means an upstream break red-lights every open PR, with no documented bypass. (trigger: .github/workflows/harbor-e2e.yml)
• 🟡 The adopt change lands on the production grading path (cli/evaluate_command.py:462, orchestration/regrade.py:990, and the in-container adopt at cli/run_task_internal_command.py:276) yet the PR states nothing about the nightly / coder-eval-uipath consumer: nightly in-place grading now performs network installs mid-grade, adding latency and a failure class that did not previously exist on that side of the run. (trigger: src/coder_eval/sandbox.py) (restates: Axis 6: adopt()'s new re-provisioning can raise subprocess.SubprocessError, which neither grading caller catches — one row's failure aborts the whole --resume batch)

Harness & Lint Improvements

Condensed for GitHub's comment-size limit — full rationale, CE ids, file targets and wiring in 08-.../00-summary.md.

Static checks (lint / type):

• Widen the CE AST runner's scan set to .github/scripts/
• Add ".github/scripts" to [tool.pyright] include (pyproject.toml:329), and add CE067 — toolchain-scope parity for LINT_PATHS`
• Turn on C901 for .github/scripts/ via a directory-scoped .github/scripts/ruff.toml
• CE068 — fork-untrusted-execution guard on pull_request workflows
• CE069 — an artifact upload may not fail silently
• CE070 — no installer is reachable from Sandbox.adopt
• Give the task-YAML lint parametrizations one SSOT and extend it past tasks/.
• Add actions to the CodeQL matrix

Harness improvements:

• Make adopt()'s write contract executable.
• Add a per-diff coverage floor to make verify
• Bring .github/scripts/ under pytest and coverage.
• Add an agent↔verifier parity test to the Harbor packager.
• Make the setup vs adopt venv-policy difference a named test pair, and have .claude/notes/isolation.md cite the test names instead of paraphrasing the rule
• Audit required checks against fork reachability.

Top 5 Priority Actions

1. Close the untrusted-installer hole this PR opens: src/coder_eval/sandbox.py:326-331 now runs uv pip install/npm install from the attacker-controlled recorded config on the evaluate <run_dir> in-place path, which _gate_scope_for_grade still exempts on the now-false premise at src/coder_eval/orchestration/regrade.py:133-136 ("adopt runs no installer") — either thread the trust decision into grading_sandbox_config or set include_setup_phase=True unconditionally, and do not ship while tests/test_detached_grading_boundaries.py:804-811 still asserts the exemption for an env_packages=["attacker-pkg"] payload.
2. Make the fix reach the failure it claims, or stop claiming it: _write_verifier_task_yaml (src/coder_eval/harbor/packager.py:338-364) emits no sandbox: key, so the Harbor verifier grades template_sources with env_packages == [] and the new branch is dead on exactly the CI path cited at .claude/notes/isolation.md:862 — carry sandbox.python/sandbox.node into the verifier task.yaml with a packager test, or correct the note.
3. Stop a failed provision from latching a silently-wrong verdict: wrap src/coder_eval/sandbox.py:326-331 so a raising _install_packages/_install_node_packages removes the .venv it just created and re-raises (today the half-built venv survives in the caller-owned workspace and the retry takes the candidate.is_dir() discover branch at line 324, grading against a venv with none of env_packages), and widen the helpers' own except subprocess.CalledProcessError at sandbox.py:805/839/866 to subprocess.SubprocessError so a TimeoutExpired/ensurepip failure no longer escapes cli/run_command.py:817 and aborts an entire --resume batch.
4. Add the fork carve-out before the new pull_request trigger at .github/workflows/harbor-e2e.yml:16-17 lands on the public repo: the job is unguarded runs-on: uipath-ubuntu-latest (line 39) with workflow-level Bedrock secrets (lines 26-34) and two docker builds, while the same-repo guard already appears nine times in pr-checks.yml/claude-pr-review.yml — and since fork PRs get no secrets, skipping is also the only way this newly-required check is not permanently red for external contributors.
5. Fix the contract-and-coverage debt around the change: correct the stale adopt() docstring (src/coder_eval/sandbox.py:268, 276-278 "materializing nothing"/"writes no FILES"), regrade.py:799 and the false "setup's own rule" claim at .claude/notes/isolation.md:867 and :276; add the missing tests for the node arm at sandbox.py:330-331 (dead across the full 6433-item suite) and for existing-venv-plus-non-empty-env_packages; and harden the new CI diagnostics with a guarded read at .github/scripts/harbor_e2e.py:153, except Exception at :214, plus a ~10-line offline TaskDefinition.model_validate test over tests/harbor_e2e/fixtures/*.yaml.

---

Stats: 2 🔴 · 4 🟠 · 7 🟡 · 4 🔵 across 8 axes reviewed.

---

Verification: 42 findings proposed, 31 verified, 3 refuted as false-positive, 24 corrected in place, 11 low-severity passed through unverified. Full report: tmp/code-review-260917-1149/.

[uipreliga]

Fix what you agree with and 🚢