From 7fabbb044a0102598582e6143afe66f17a4d81e1 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Fri, 4 Sep 2026 19:43:47 +0000 Subject: [PATCH] fix(eval): stop hiding review patches from sandboxed git apply The oracle-mask overlay covered the same path review setup reads, so every historical cell died with can't-open-patch. Leave the staged copy visible for apply, then fail closed if it is still there when the model starts. Co-authored-by: Cursor --- eval/tests/test_oracle_assets.py | 14 ++++++ eval/tests/test_runner_hardening.py | 71 +++++++++++++++++++++++++++- eval/workflow_bench/oracle_assets.py | 42 +++++++++++++++- eval/workflow_bench/runner.py | 33 +++++-------- 4 files changed, 137 insertions(+), 23 deletions(-) diff --git a/eval/tests/test_oracle_assets.py b/eval/tests/test_oracle_assets.py index c8bf2ccad..6d41ef6cc 100644 --- a/eval/tests/test_oracle_assets.py +++ b/eval/tests/test_oracle_assets.py @@ -16,6 +16,7 @@ from workflow_bench import oracle_assets, runner from workflow_bench.evolution import evaluate_candidate from workflow_bench.oracle_assets import ( capture_task_oracle, + require_hidden_harness_absent, review_case_setup_command, staged_task_oracle, with_hidden_harness_apply_exclude, @@ -155,6 +156,19 @@ def test_clone_sanitization_prunes_harness_checkout_and_recoverable_history(tmp_ assert git(clone, "status", "--porcelain=v1", "--untracked-files=all").stdout == "" +def test_require_hidden_harness_absent_fails_closed_on_leftover_tree(tmp_path: Path) -> None: + clone = tmp_path / "clone" + hidden = clone / "eval" / "workflow_bench" + hidden.mkdir(parents=True) + (hidden / "review_cases").mkdir() + + with pytest.raises(ValueError, match="hidden harness visible"): + require_hidden_harness_absent(clone) + + shutil.rmtree(hidden) + require_hidden_harness_absent(clone) + + def test_hidden_harness_apply_exclude_is_idempotent() -> None: raw = "git apply eval/workflow_bench/review_cases/pr.patch && rm -rf eval/workflow_bench" once = with_hidden_harness_apply_exclude(raw) diff --git a/eval/tests/test_runner_hardening.py b/eval/tests/test_runner_hardening.py index c1d08a0c3..785707b5a 100644 --- a/eval/tests/test_runner_hardening.py +++ b/eval/tests/test_runner_hardening.py @@ -2,6 +2,7 @@ import hashlib import json +import shutil from contextlib import nullcontext from pathlib import Path from types import SimpleNamespace @@ -10,6 +11,7 @@ import pytest from workflow_bench import runner, runner_artifacts, runner_sessions from workflow_bench.evolution import skill_fingerprint +from workflow_bench.oracle_assets import review_case_setup_command from workflow_bench.process_control import ManagedProcessError, ManagedProcessResult from workflow_bench.proposer_sandbox import SandboxError @@ -456,7 +458,6 @@ def _cell_context(tmp_path, **overrides): auth_token=None, ), "out_dir": tmp_path / "out", - "oracle_mask": tmp_path / "mask", "ce_plugin_snapshot": None, "trees_dir": tmp_path / "trees", "bwrap_bin": tmp_path / "bwrap", @@ -607,6 +608,74 @@ def test_run_cell_reports_a_cleanup_failure_over_its_primary_outcome(monkeypatch assert "clone is busy" in record["error_detail"] +def test_run_cell_does_not_mask_the_staged_review_patch_before_setup(monkeypatch, tmp_path): + """Review setup applies a patch staged under eval/workflow_bench. + + Overlaying the empty oracle mask on that path is the CI abort: + `git apply` dies with `can't open patch`. The staged copy must stay + visible to sandboxed setup, then be gone before the model starts. + """ + + worktree, _ = _stub_cell_dependencies(monkeypatch, tmp_path) + patch = worktree / "eval" / "workflow_bench" / "review_cases" / "pr-2718.patch" + patch.parent.mkdir(parents=True) + patch.write_text("diff --git a/visible.py b/visible.py\n") + captured: dict[str, object] = {} + + def fake_prepare(**kwargs): + captured["mounts"] = kwargs.get("read_only_mounts", []) + + def run(_command, **_kwargs): + leftover = worktree / "eval" / "workflow_bench" + if leftover.exists(): + shutil.rmtree(leftover) + return SimpleNamespace(ok=True) + + return nullcontext(SimpleNamespace(run=run)) + + monkeypatch.setattr(runner, "prepare_sandbox", fake_prepare) + context = _cell_context( + tmp_path, + task={ + "id": "review-pr-2718-defect", + "class": "review-defect", + "prompt": "review the local diff", + "setup": review_case_setup_command("pr-2718.patch"), + }, + ) + record = runner.run_cell(context, 0, "workflow") + + assert record.get("error_kind") is None + targets = [getattr(mount, "target", None) for mount in captured["mounts"] if mount is not None] + assert not any(target and "eval/workflow_bench" in str(target) for target in targets) + assert not (worktree / "eval" / "workflow_bench").exists() + + +def test_run_cell_fails_closed_when_setup_leaves_the_hidden_harness(monkeypatch, tmp_path): + worktree, _ = _stub_cell_dependencies(monkeypatch, tmp_path) + leftover = worktree / "eval" / "workflow_bench" / "review_cases" + leftover.mkdir(parents=True) + (leftover / "pr-2718.patch").write_text("diff\n") + + def fake_prepare(**kwargs): + return nullcontext(SimpleNamespace(run=lambda *_a, **_k: SimpleNamespace(ok=True))) + + monkeypatch.setattr(runner, "prepare_sandbox", fake_prepare) + context = _cell_context( + tmp_path, + task={ + "id": "review-pr-2718-defect", + "class": "review-defect", + "prompt": "review the local diff", + "setup": review_case_setup_command("pr-2718.patch"), + }, + ) + record = runner.run_cell(context, 0, "workflow") + + assert record["error_kind"] == "infra-error" + assert "hidden harness visible" in str(record["error_detail"]) + + def test_run_cell_fails_closed_when_a_per_task_snapshot_never_materialized(tmp_path): # The snapshots are prepared once per task, before any cell. If that failed, # every cell of the task has to record it rather than run against nothing. diff --git a/eval/workflow_bench/oracle_assets.py b/eval/workflow_bench/oracle_assets.py index f7c686506..a2b033729 100644 --- a/eval/workflow_bench/oracle_assets.py +++ b/eval/workflow_bench/oracle_assets.py @@ -244,6 +244,36 @@ def _git_checked( return result.stdout_tail.strip() +def hidden_harness_dir(clone: Path) -> Path: + """Checkout-relative path of the hidden benchmark harness tree.""" + + return clone / HIDDEN_HARNESS_PATH + + +def hidden_harness_is_visible(clone: Path) -> bool: + """True when the hidden harness still exists as a file, directory, or symlink.""" + + leftover = hidden_harness_dir(clone) + return leftover.exists() or leftover.is_symlink() + + +def require_hidden_harness_absent(clone: Path) -> None: + """Fail closed if setup left ``eval/workflow_bench`` visible to the model. + + Review cells stage a historical patch under this tree so ``git apply`` can + read it. Overlaying an empty mask on the same path hides that file and + aborts every cell with ``can't open patch``. The runner therefore leaves + the staged copy visible during sandboxed setup, then requires this tree to + be gone before the model session starts. + """ + + if hidden_harness_is_visible(clone): + raise ValueError( + "task setup left the hidden harness visible to the model: " + f"{HIDDEN_HARNESS_PATH.as_posix()}" + ) + + def with_hidden_harness_apply_exclude(setup: str) -> str: """Skip hunks for the harness tree sanitization already deleted. @@ -252,6 +282,10 @@ def with_hidden_harness_apply_exclude(setup: str) -> str: (``learnings.jsonl`` on review-pr-2718-defect). Those files are gone from the parentless snapshot, and setup deletes the directory again after apply, so the hunks are never model-visible. + + The runner must not overlay a mask on ``eval/workflow_bench`` before that + ``git apply``: the staged patch lives at the same path, and a mask makes + ``git apply`` fail with ``can't open patch``. """ if "git apply" not in setup: @@ -263,7 +297,13 @@ def with_hidden_harness_apply_exclude(setup: str) -> str: def review_case_setup_command(patch_name: str) -> str: - """Setup that applies one review-case patch and then hides the harness.""" + """Apply one review-case patch, then delete the staged harness copy. + + The patch is staged back under ``eval/workflow_bench`` after sanitization + so this command can read it inside the sandbox. The trailing ``rm -rf`` + is what hides the tree from the model — not an empty overlay on the + same path, which would hide the patch from ``git apply``. + """ if not patch_name or "/" in patch_name or "\\" in patch_name or patch_name in {".", ".."}: raise ValueError(f"review patch name must be a single path segment: {patch_name!r}") diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index 9ac16de30..3b4635520 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -83,6 +83,7 @@ from .oracle_assets import ( ORACLE_ENV_VAR, TaskOracleSnapshot, capture_task_oracles, + require_hidden_harness_absent, sanitize_clone_for_hidden_oracles, staged_task_oracle, with_hidden_harness_apply_exclude, @@ -864,7 +865,6 @@ class TaskCellContext: asset_snapshot_error: BaseException | None args: argparse.Namespace out_dir: Path - oracle_mask: Path ce_plugin_snapshot: CePluginSnapshot | None trees_dir: Path bwrap_bin: Path @@ -906,18 +906,6 @@ def run_cell(ctx: TaskCellContext, run_idx: int, arm: str) -> dict[str, Any]: snapshot=ctx.asset_snapshot, ) registry_mount = isolated_gitnexus_registry_mount(worktree, ctx.trees_dir) - hidden_harness = worktree / "eval" / "workflow_bench" - oracle_visibility_mounts: list[ReadOnlyMount] = [] - if hidden_harness.exists() or hidden_harness.is_symlink(): - hidden_metadata = hidden_harness.lstat() - if stat.S_ISLNK(hidden_metadata.st_mode) or not stat.S_ISDIR(hidden_metadata.st_mode): - raise SandboxError("benchmark harness path must be a real directory before it can be hidden") - oracle_visibility_mounts.append( - ReadOnlyMount( - source=ctx.oracle_mask, - target=f"{SANDBOX_WORKSPACE}/eval/workflow_bench", - ) - ) execution_arm = CANDIDATE_ARMS.get(arm, arm) ce_mounts = ce_plugin_mounts_for_arm(execution_arm, ctx.ce_plugin_snapshot) with prepare_sandbox( @@ -929,7 +917,6 @@ def run_cell(ctx: TaskCellContext, run_idx: int, arm: str) -> dict[str, Any]: *ctx.runtime_mounts, registry_mount, *ce_mounts, - *oracle_visibility_mounts, ], preflight=False, backend=ctx.sandbox_backend, @@ -952,9 +939,13 @@ def run_cell(ctx: TaskCellContext, run_idx: int, arm: str) -> dict[str, Any]: ) base_skill_digest = skill_fingerprint(worktree, execution_arm) if task.get("setup"): - # Sanitization already removed eval/workflow_bench. A historical - # PR patch that still edits that tree (gitignored learnings.jsonl) - # must skip those hunks or `git apply` fails closed. + # Sanitization already removed eval/workflow_bench. Review + # cells copy the historical PR patch back under that path so + # `git apply` can read it. Do not overlay an empty mask on + # the same tree first — that hides the patch file and every + # cell dies with `can't open patch`. A historical patch that + # still edits the harness (gitignored learnings.jsonl) must + # skip those hunks or apply fails closed. setup_command = ["/bin/sh", "-lc", with_hidden_harness_apply_exclude(str(task["setup"]))] setup = sandbox.run( setup_command, @@ -963,6 +954,10 @@ def run_cell(ctx: TaskCellContext, run_idx: int, arm: str) -> dict[str, Any]: ) if not setup.ok: raise ManagedProcessError(setup_command, setup) + # Setup (or its absence) must leave the staged harness copy gone + # before the model session starts. Fail closed rather than hide + # the tree with a mask that would also hide the patch from apply. + require_hidden_harness_absent(worktree) # Tamper-evidence: setup must not have rewritten the base # skills, verified before any candidate overlay lands. require_skill_fingerprint( @@ -1716,9 +1711,6 @@ def _run_sweep( except (OSError, SandboxError, ValueError) as exc: parser.error(str(exc)) raise AssertionError("ArgumentParser.error() returned unexpectedly") - oracle_mask = Path(trees) / ".oracle-mask" - oracle_mask.mkdir(mode=0o500) - oracle_mask.chmod(0o500) graph_snapshots: dict[tuple[str, str], SanitizedGraphSnapshot] = {} graph_snapshot_errors: dict[tuple[str, str], BaseException] = {} for task, task_binding, oracle_snapshot in zip( @@ -1777,7 +1769,6 @@ def _run_sweep( asset_snapshot_error=asset_snapshot_error, args=args, out_dir=out_dir, - oracle_mask=oracle_mask, ce_plugin_snapshot=ce_plugin_snapshot, trees_dir=Path(trees), bwrap_bin=bwrap_bin,