mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-09 03:17:54 +00:00
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 <cursoragent@cursor.com>
This commit is contained in:
parent
951e272557
commit
7fabbb044a
4 changed files with 137 additions and 23 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue