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

.claude/notes/isolation.md

@@ -274,7 +274,9 @@ run's stale `aws_region`.
 
 ### Why pre_run and post_run each run exactly once
 
-`adopt()` guarantees it materializes nothing into the workspace, but that guarantee is
+`adopt()` never re-runs `pre_run`/`post_run` itself and never stages a template into the
+workspace (its own writes are confined to re-provisioning a missing `.venv`/`node_modules`
+from `env_packages`, see § Why the venv gets system site packages), but that restraint is
 only as strong as its weakest caller: `run()` invokes the hooks unconditionally, with
 `cwd = sandbox_dir`. Several in-tree tasks stage fixtures there (`cp -a /app/[!.]* "$PWD/"`),
 so re-running `pre_run` during a detached grade would overwrite the agent's deliverables
@@ -370,11 +372,23 @@ and derived through the same function `run_evaluation` uses (`_gate_scope_for_gr
 second copy of the rule keeps answering the old question the moment the default moves: the
 lever shipped beside an `include_setup_phase` every caller passed as its exact complement,
 and a caller setting one and forgetting the other would silently drop half of a SECURITY
-gate. In place, the grade may dispatch a
-CONTAINER built from the recorded sandbox block, a wider capability than any recorded shell
-string; on `--copy` instead, `pre_run` and the sandbox's own installers, neither of which an
-adopted workspace reaches. `post_run` is in NEITHER set — it belongs to the grading phase
-and runs on both paths, so it is scanned unconditionally.
+gate. In place, the grade may dispatch a CONTAINER built from the recorded sandbox block, a
+wider capability than any recorded shell string; on `--copy` instead, `pre_run` and a
+`template_sources` repo's `git clone`, neither of which an adopted workspace reaches.
+`env_packages` installs are scanned on BOTH paths — `Sandbox.adopt` can now run them too
+(see the bullet below) — and `post_run` is likewise scanned unconditionally, because it
+belongs to the grading phase and runs on both paths. See `orchestration.md` § What the gate
+covers, and why each part is in scope for the full derivation.
+
+- **`adopt()`'s `env_packages` re-provisioning is the one place it writes FILES.** Every
+  other step `adopt` takes is discovery: an existing venv is picked up, never rebuilt, and a
+  bare `config.python` with empty `env_packages` is a pure no-op. The exception is a
+  captured or WORKDIR-aligned workspace missing `.venv`/`node_modules` while `env_packages`
+  is non-empty (see § Why the venv gets system site packages) — `adopt` then runs the same
+  `_setup_virtualenv`/`_install_packages`/`_install_node_packages` `setup()` uses, and on a
+  failed install removes exactly what it just created (never the caller's own tree) and
+  re-raises, so a half-built venv never latches into the workspace being graded and the next
+  adopt silently discovers it as complete.
 
 ## Grading a docker row inside a container
 
@@ -855,6 +869,31 @@ shape this host got is logged rather than left to be inferred.
 `setup` is: discovering a venv a task never asked for grades it under a PATH it never ran
 under, and would let an agent shadow binaries by writing `.venv/bin/` into its own workspace.
 
+Discovery alone was not the whole story: `Sandbox.capture_to` (the docker-WORKDIR-alignment
+path — Harbor's `CoderEvalAgent`, any `--workspace-dir` execute) excludes `.venv` /
+`node_modules` / `.npm-prefix` from the copy-out as noise (`_WORKSPACE_CAPTURE_IGNORE`), so a
+workspace adopted from a captured WORKDIR never has one to discover — even though the execute
+phase installed `env_packages` into it. 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. `adopt` now falls back to `_setup_virtualenv` +
+`_install_packages` (or `_install_node_packages`) whenever the expected directory is missing
+AND `env_packages` is non-empty. Reaching this on the Harbor verifier path needed a second,
+parallel fix: `_write_verifier_task_yaml` (`harbor/packager.py`) built `tests/task.yaml` with
+no `sandbox` key at all, so the verifier graded with `env_packages == []` and this branch was
+dead on exactly the scenario above — it now carries `sandbox.python`/`sandbox.node`'s
+`env_packages` (nothing else in `sandbox`, which is an agent-phase-only concern there).
+
+The `env_packages` gate is NOT parity with `setup`, which is worth stating precisely because
+it looks like it should be: `setup` (`sandbox.py`) creates a venv for bare `config.python`
+unconditionally and gates only the INSTALL on `env_packages`, so `python: {env_packages: []}`
+still gets an empty venv (and a populated `VIRTUAL_ENV`/PATH) on the `--copy` path. `adopt`
+gates venv creation itself on `env_packages`, so the same config stays a true no-op in place —
+deliberately: there is nothing to install, and re-provisioning an empty venv into a workspace
+that already ran without one would only shadow the interpreter for no benefit. The two paths
+therefore diverge for that one config shape; this is accepted, not accidental. A venv that
+already exists (the ordinary `preserve_to` / non-captured path) is still only discovered,
+never rebuilt.
+
 The unit test in `tests/test_sandbox.py` reads `pyvenv.cfg`. That proves the flag is set,
 not that the result is correct. Every task image installs packages globally: the
 framework image uses `uv pip install --system`, and skillsbench task images use

.claude/notes/orchestration.md

@@ -670,8 +670,20 @@ and it prints as the command is already being prepared.
 ### What the gate covers, and why each part is in scope
 
 `include_setup_phase` covers the two capability families that exist only on the `--copy`
-path: `pre_run`, and the sandbox's own provisioning. Both are SKIPPED when grading in
-place, so on that path they are not a capability the run dir has.
+path: `pre_run` and a `template_sources` repo's `git clone`. Both are SKIPPED when
+grading in place — the orchestrator never runs `pre_run` there, and `Sandbox.adopt` never
+stages a template — so on that path neither is a capability the run dir has.
+
+**`env_packages` installs are NOT part of `include_setup_phase`, and used to be.** The
+premise was "`adopt` runs no installer", true when `adopt` only ever DISCOVERED an
+existing `.venv`/`node_modules`. It no longer holds: `adopt` now re-provisions
+`env_packages` whenever a captured workspace is missing them (see
+`.claude/notes/isolation.md` § Why the venv gets system site packages), so `uv pip
+install`/`npm install` are a capability of the IN-PLACE path too — exactly like `post_run`
+below. Gating them on `include_setup_phase` would let a shared run directory install
+attacker-chosen packages on the grader's host with no consent prompt, simply by omitting
+`.venv` from what it shares (which `capture_to` already strips as noise). They are
+disclosed unconditionally instead, regardless of `grade_in_place`.
 
 **`post_run` is deliberately NOT behind that flag**, and this is the one place the
 distinction bites. It used to be, back when the hooks were skipped as a pair — but

.github/scripts/harbor_e2e.py

@@ -28,6 +28,12 @@
 REPO_ROOT = Path(__file__).resolve().parents[2]
 WORK_DIR = REPO_ROOT / "tmp" / "harbor_e2e"
 AGENT_IMPORT_PATH = "coder_eval.harbor.agent:CoderEvalAgent"
+# Where a failing scenario's full export/ + jobs/ tree gets zipped for upload --
+# a print-statement diagnostic only shows what this script thought to ask for
+# (and dies with the runner). A zip preserves everything: docker/agent logs,
+# every task.json/trajectory.json, artifacts/ workspaces -- so a failure can be
+# inspected after the fact instead of guessed at from stdout.
+FAILURE_ARTIFACTS_DIR = REPO_ROOT / "tmp" / "harbor_e2e_failures"
 
 
 @dataclass(frozen=True)
@@ -115,13 +121,82 @@ def run_harbor(scenario: Scenario, export_dir: Path, jobs_dir: Path) -> Path:
     return trial_dirs[0]
 
 
+def _read_json_best_effort(path: Path) -> object | str | None:
+    """Read and parse ``path`` as JSON, or a string describing why not.
+
+    Only used to build FAILURE diagnostics: a truncated/missing file must
+    degrade to a note rather than raise and replace the real reward/criteria
+    failure this exists to explain -- the same rule ``_zip_scenario_dir``
+    already states for itself.
+    """
+    if not path.is_file():
+        return None
+    try:
+        return json.loads(path.read_text(encoding="utf-8"))
+    except (OSError, json.JSONDecodeError) as exc:
+        return f"unreadable ({exc})"
+
+
+def _criteria_detail(verifier_result: object) -> list[dict[str, object]] | None:
+    """Per-criterion score/detail from a parsed verifier ``task.json``."""
+    if not isinstance(verifier_result, dict):
+        return None
+    return [
+        {
+            "type": r.get("criterion_type"),
+            "score": r.get("score"),
+            "details": r.get("details"),
+            "error": r.get("error"),
+            "evaluation_status": r.get("evaluation_status"),
+            "result_kind": r.get("result_kind"),
+        }
+        for r in verifier_result.get("success_criteria_results", [])
+    ]
+
+
+def _agent_commands(agent_task_jsons: list[Path]) -> list[dict[str, object]] | str:
+    """The AGENT phase's own recorded tool calls, for a failure message.
+
+    Read directly rather than trusting the verifier's hydration of it: its
+    `iterations[].commands` is the raw telemetry criteria like
+    `command_executed` are supposed to hydrate from, so dumping it distinguishes
+    "no commands were ever recorded" (a hydration/telemetry bug) from "the
+    recorded commands just didn't match the pattern" (an agent/fixture issue).
+    """
+    if not agent_task_jsons:
+        return "no agent/task.json found"
+    agent_result = _read_json_best_effort(agent_task_jsons[0])
+    if not isinstance(agent_result, dict):
+        return f"unreadable: {agent_result!r}"
+    return [
+        {"tool_name": c.get("tool_name"), "parameters": c.get("parameters")}
+        for it in agent_result.get("iterations", [])
+        for c in it.get("commands", [])
+    ]
+
+
 def assert_scenario_artifacts(scenario: Scenario, trial_dir: Path) -> None:
     reward_path = trial_dir / "verifier" / "reward.json"
     if not reward_path.is_file():
         raise RuntimeError(f"[{scenario.name}] missing {reward_path}")
     reward = json.loads(reward_path.read_text(encoding="utf-8"))
+
+    # Read the per-criterion breakdown ONCE, before the reward gate below, so a
+    # failing reward's own root cause (which criterion, and why) is always in
+    # the failure message -- not only when an unrelated criterion happens to
+    # carry the aggregate to 1.0 "by luck" while this one silently failed.
+    verifier_task_json = trial_dir / "verifier" / "task.json"
+    if not verifier_task_json.is_file():
+        raise RuntimeError(f"[{scenario.name}] missing {verifier_task_json}")
+    verifier_result = _read_json_best_effort(verifier_task_json)
+    criteria_detail = _criteria_detail(verifier_result)
+    agent_task_jsons = sorted((trial_dir / "agent").glob("**/task.json"))
+
     if reward.get("reward") != 1.0:
-        raise RuntimeError(f"[{scenario.name}] expected reward 1.0, got {reward!r} ({reward_path})")
+        raise RuntimeError(
+            f"[{scenario.name}] expected reward 1.0, got {reward!r} ({reward_path}); "
+            + f"criteria: {criteria_detail!r}; agent-phase recorded commands: {_agent_commands(agent_task_jsons)!r}"
+        )
 
     trajectory_path = trial_dir / "agent" / "trajectory.json"
     if not trajectory_path.is_file():
@@ -130,18 +205,15 @@ def assert_scenario_artifacts(scenario: Scenario, trial_dir: Path) -> None:
     if "schema_version" not in trajectory:
         raise RuntimeError(f"[{scenario.name}] {trajectory_path} is missing 'schema_version'")
 
-    agent_task_jsons = list((trial_dir / "agent").glob("**/task.json"))
     if not agent_task_jsons:
         raise RuntimeError(f"[{scenario.name}] no task.json found under {trial_dir / 'agent'}")
-    verifier_task_json = trial_dir / "verifier" / "task.json"
-    if not verifier_task_json.is_file():
-        raise RuntimeError(f"[{scenario.name}] missing {verifier_task_json}")
+    if not isinstance(verifier_result, dict):
+        raise RuntimeError(f"[{scenario.name}] {verifier_task_json} did not parse as JSON: {verifier_result!r}")
 
     # An overall reward of 1.0 does not prove a trajectory criterion was
     # actually graded -- it could pass "by luck" from unrelated criteria while
     # this one silently scored 0.0 against an ungraded/empty trajectory. Check
     # each trajectory-dependent criterion's OWN score directly.
-    verifier_result = json.loads(verifier_task_json.read_text(encoding="utf-8"))
     trajectory_results = [
         r
         for r in verifier_result.get("success_criteria_results", [])
@@ -161,6 +233,28 @@ def assert_scenario_artifacts(scenario: Scenario, trial_dir: Path) -> None:
     )
 
 
+def _zip_scenario_dir(scenario: Scenario) -> Path | None:
+    """Zip a failed scenario's whole ``export/`` + ``jobs/`` tree for upload.
+
+    Best-effort: a zip failure must never mask the real scenario failure it was
+    trying to preserve evidence for.
+    """
+    scenario_dir = WORK_DIR / scenario.name
+    if not scenario_dir.is_dir():
+        return None
+    try:
+        FAILURE_ARTIFACTS_DIR.mkdir(parents=True, exist_ok=True)
+        archive = shutil.make_archive(str(FAILURE_ARTIFACTS_DIR / scenario.name), "zip", root_dir=str(scenario_dir))
+    # Broad by intent: e.g. a pre-1980 mtime from a container image layer raises
+    # ValueError, not OSError, and an undecodable filename raises UnicodeEncodeError.
+    # Either would otherwise escape from inside main()'s own `except Exception`,
+    # killing the scenario loop and hiding every scenario after this one.
+    except Exception as exc:
+        print(f"[{scenario.name}] could not zip {scenario_dir} for upload: {exc}", file=sys.stderr)
+        return None
+    return Path(archive)
+
+
 def main() -> int:
     WORK_DIR.mkdir(parents=True, exist_ok=True)
     failures: list[str] = []
@@ -175,6 +269,9 @@ def main() -> int:
         except Exception as exc:
             print(f"[{scenario.name}] FAILED: {exc}", file=sys.stderr)
             failures.append(scenario.name)
+            archive = _zip_scenario_dir(scenario)
+            if archive is not None:
+                print(f"[{scenario.name}] full export/+jobs/ tree zipped to {archive} for upload", file=sys.stderr)
 
     print("\n=== Summary ===")
     for scenario in SCENARIOS:

.github/workflows/harbor-e2e.yml

@@ -1,18 +1,20 @@
 name: Harbor E2E
 
-# Deliberately NOT triggered on pull_request: this exercises real Docker
-# builds, a real `harbor` install, and (for the llm_judge scenario) a real
-# model call, so it is informational rather than a required PR check for now
-# (see .github/scripts/harbor_e2e.py's module docstring). workflow_dispatch
-# lets a maintainer run it on demand; the nightly schedule catches drift
-# between coder-eval's own release and Harbor's own upstream releases without
-# blocking anyone's PR.
+# Required on every PR: exercises real Docker builds, a real `harbor` install,
+# and (for the llm_judge scenario) a real model call, so it is the one gate
+# that actually round-trips a task through Harbor's own export/agent/verifier
+# contract rather than coder-eval's in-process tests. workflow_dispatch lets a
+# maintainer run it on demand; the nightly schedule catches drift between
+# coder-eval's own release and Harbor's own upstream releases even when no PR
+# touched anything.
 on:
   workflow_dispatch:
   schedule:
     - cron: "17 5 * * *" # nightly, off the hour to avoid GitHub's peak-load pile-up
   push:
     branches: [main]
+  pull_request:
+    branches: [main]
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}
@@ -27,13 +29,16 @@ env:
   # Anthropic-credit spend off this path; DirectRoute is exercised elsewhere.
   API_BACKEND: "bedrock"
   CLAUDE_CODE_USE_BEDROCK: "1"
-  AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }}
-  AWS_REGION: ${{ secrets.AWS_REGION }}
-  BEDROCK_MODEL: ${{ secrets.BEDROCK_MODEL }}
 
 jobs:
   harbor-e2e:
     name: Harbor export + CoderEvalAgent round trip
+    # A fork PR's own workflow, scripts, uv.lock and Dockerfiles all run for a
+    # `pull_request` event -- and this job does two `docker build`s plus a real
+    # Bedrock model call. Fork PRs get no secrets (below) so the job cannot pass
+    # for one anyway; skip rather than run untrusted code on the shared pool and
+    # leave a required check permanently red for every external contributor.
+    if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
     runs-on: uipath-ubuntu-latest
     timeout-minutes: 20
 
@@ -88,4 +93,25 @@ jobs:
         run: docker build -t byod-custom-image:0.1.0 templates/byod_smoke_test/
 
       - name: Run Harbor E2E scenarios
+        # Bedrock credentials are scoped to this one step, not the workflow-level
+        # env: above (where earlier revisions left them) -- so npm/pip/docker build
+        # steps that fetch third-party code never see them in their environment.
+        env:
+          AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }}
+          AWS_REGION: ${{ secrets.AWS_REGION }}
+          BEDROCK_MODEL: ${{ secrets.BEDROCK_MODEL }}
         run: python .github/scripts/harbor_e2e.py
+
+      - name: Upload failing scenario artifacts
+        # A failing scenario's full export/ + jobs/ tree (docker/agent logs,
+        # every task.json/trajectory.json, artifacts/ workspaces) zipped by the
+        # script above -- so a failure can be inspected after the fact instead
+        # of guessed at from stdout. Empty on a fully green run; `if: failure()`
+        # still uploads whatever any failing scenario left behind.
+        if: failure()
+        uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a  # v7.0.1
+        with:
+          name: harbor-e2e-failure-artifacts
+          path: tmp/harbor_e2e_failures/
+          retention-days: 14
+          if-no-files-found: ignore

docs/USER_GUIDE.md

@@ -267,6 +267,11 @@ mode grades **in place**, because copying filters build output — `node_modules
 `dist`, `build`, `.venv`, `.git` are all on the default ignore list, so a
 criterion like `test -f dist/bundle.js` would fail as a *copying artifact*
 rather than as a verdict. Override either default with `--in-place` / `--copy`.
+Grading in place can still need network and install time: if the workspace is
+missing `.venv`/`node_modules` (the ignore list above stripped them, or the run
+was captured across a container boundary) and the task declares
+`sandbox.python`/`sandbox.node` `env_packages`, grading re-provisions them before
+the criteria run, and can fail the grade outright if that install fails.
 
 | Flag | Description |
 | --- | --- |

pyproject.toml

@@ -326,7 +326,7 @@ line-ending = "auto"
 
 [tool.pyright]
 pythonVersion = "3.13"
-include = ["src/coder_eval"]
+include = ["src/coder_eval", ".github/scripts"]
 exclude = [
     "data",
     "tmp",

src/coder_eval/cli/evaluate_command.py

@@ -488,9 +488,11 @@ async def _setup_and_run() -> EvaluationResult:
 
     try:
         result = asyncio.run(_setup_and_run())
-    except RegradeError as e:
+    except (RegradeError, RuntimeError) as e:
         # Rendered like the three sibling handlers above: unwrapped, these
-        # operator-facing messages arrived as the tail of a stack trace.
+        # operator-facing messages arrived as the tail of a stack trace. RuntimeError
+        # is also caught here: both `Sandbox.setup` and `Sandbox.adopt` can raise one
+        # from a failed `env_packages` install (network, bad package name, timeout).
         console.print(f"[red]✗ {escape(str(e))}[/red]")
         raise typer.Exit(1) from e
     _report_and_exit(result, task=task, prior=prior, target=target, prepared_run_dir=prepared_run_dir)