diff --git a/eval/tests/test_proposer_sandbox.py b/eval/tests/test_proposer_sandbox.py index 2e30cf6a2..8af24c5fb 100644 --- a/eval/tests/test_proposer_sandbox.py +++ b/eval/tests/test_proposer_sandbox.py @@ -52,37 +52,49 @@ from workflow_bench.proposer_sandbox import ( from workflow_bench.task_assets import TaskAssetCache, stage_task_assets as stage_immutable_task_assets -@pytest.mark.parametrize("entry", ["file", "directory", "relative-link", "absolute-link"]) -def test_review_preparation_rejects_existing_output_without_touching_target(tmp_path, entry): +@pytest.mark.parametrize("entry", ["directory", "relative-link", "absolute-link"]) +def test_review_preparation_rejects_a_reused_artifact_directory(tmp_path, entry): clone = tmp_path / "clone" clone.mkdir() sentinel = tmp_path / "sentinel" sentinel.write_text("must survive") - output = clone / "review-output.json" - if entry == "file": - output.write_text("existing result") - elif entry == "directory": - output.mkdir() - else: - output.symlink_to(sentinel if entry == "absolute-link" else "../sentinel") with prepare_sandbox(clone=clone, claude_bin=sys.executable, backend="host-unsafe") as sandbox: + stale = proposer_sandbox.review_output_path(sandbox, "review-output.json").parent + if entry == "directory": + stale.mkdir() + (stale / "review-output.json").write_text("a previous cell's verdict") + else: + stale.symlink_to(sentinel if entry == "absolute-link" else "../sentinel") with pytest.raises(SandboxError, match="already exists"): proposer_sandbox.prepare_review_workspace(sandbox, "review-output.json") assert sentinel.read_text() == "must survive" - if entry == "file": - assert output.read_text() == "existing result" - if "link" in entry: - assert output.is_symlink() -def test_review_preparation_creates_a_private_regular_output(tmp_path): +def test_review_preparation_leaves_a_clone_entry_of_the_same_name_alone(tmp_path): + # The artifact no longer lives in the workspace, so a file that happens to + # share its name is just one of the repository's own files. + clone = tmp_path / "clone" + clone.mkdir() + (clone / "review-output.json").write_text("repository content") + with prepare_sandbox(clone=clone, claude_bin=sys.executable, backend="host-unsafe") as sandbox: + output = proposer_sandbox.prepare_review_workspace(sandbox, "review-output.json") + assert (clone / "review-output.json").read_text() == "repository content" + assert clone not in output.parents + + +def test_review_preparation_creates_a_private_directory_and_not_the_file(tmp_path): clone = tmp_path / "clone" clone.mkdir() with prepare_sandbox(clone=clone, claude_bin=sys.executable, backend="host-unsafe") as sandbox: output = proposer_sandbox.prepare_review_workspace(sandbox, "review-output.json") - assert output.read_bytes() == b"" - assert stat.S_ISREG(output.lstat().st_mode) - assert stat.S_IMODE(output.stat().st_mode) == 0o600 + assert output == proposer_sandbox.review_output_path(sandbox, "review-output.json") + # The DIRECTORY is what has to exist and be writable: the agent writes + # a temp file beside the target and renames it. + assert output.parent.is_dir() + assert stat.S_IMODE(output.parent.stat().st_mode) == 0o700 + # The file is deliberately absent — absence is how "never written" is + # told apart from "written badly". + assert not output.exists() def test_review_preparation_preserves_existing_runtime_files_and_tracks_only_created_paths(tmp_path): @@ -1418,3 +1430,41 @@ PY""" if not review_layout: assert (clone / "bash-called").read_text() == "canary" + + +def test_review_artifact_binds_a_writable_directory_outside_the_workspace(tmp_path): + """The bwrap argv, since the mount shape is the whole bug. + + bwrap cannot create a mount point inside an already-read-only bind, so a + writable path has to live outside /workspace — and it has to be the + directory, or the agent has nowhere to put the temp file it renames into + place. + """ + + clone = tmp_path / "clone" + clone.mkdir() + with prepare_sandbox(clone=clone, claude_bin=sys.executable, backend="host-unsafe") as session: + sandbox = replace(session, backend="bwrap") + output = proposer_sandbox.review_output_path(sandbox, "review-output.json") + output.parent.mkdir(mode=0o700) + argv = sandbox.command_prefix_for( + read_only_workspace=True, + extra_writable_mounts=( + proposer_sandbox.ReadOnlyMount( + source=output.parent, + target=proposer_sandbox.SANDBOX_REVIEW_OUTPUT, + ), + ), + ) + + target = proposer_sandbox.SANDBOX_REVIEW_OUTPUT + assert not target.startswith(proposer_sandbox.SANDBOX_WORKSPACE + "/") + # The workspace itself is bound read-only... + workspace_at = argv.index(proposer_sandbox.SANDBOX_WORKSPACE) + assert argv[workspace_at - 2] == "--ro-bind" + # ...and the artifact directory is bound writable, as a directory. + artifact_at = argv.index(target) + assert argv[artifact_at - 2] == "--bind" + assert Path(argv[artifact_at - 1]) == output.parent + assert Path(argv[artifact_at - 1]).is_dir() + assert f"{proposer_sandbox.SANDBOX_WORKSPACE}/review-output.json" not in argv diff --git a/eval/tests/test_review_scoring.py b/eval/tests/test_review_scoring.py index c1b081720..65bac2a5f 100644 --- a/eval/tests/test_review_scoring.py +++ b/eval/tests/test_review_scoring.py @@ -325,3 +325,31 @@ def test_clean_control_rewards_an_empty_approval_and_penalizes_noise(): assert noisy["recall"] is None assert noisy["clean_pass"] is False assert noisy["verdict_correct"] is False + + +def test_parse_review_output_names_the_actual_failure(tmp_path: Path): + """One message per cause. + + Folding these together is how a sandbox that made the artifact impossible + to write read for fifteen runs as an encoding fault: every cell reported + "not valid UTF-8 JSON" for a file the agent was never able to create. + """ + + missing = tmp_path / "never-written.json" + with pytest.raises(ValueError, match="was never written"): + parse_review_output(missing) + + empty = tmp_path / "empty.json" + empty.touch() + with pytest.raises(ValueError, match="is empty"): + parse_review_output(empty) + + not_utf8 = tmp_path / "latin1.json" + not_utf8.write_bytes(b'{"verdict": "\xff\xfe"}') + with pytest.raises(ValueError, match="not valid UTF-8"): + parse_review_output(not_utf8) + + prose = tmp_path / "prose.json" + prose.write_text("Here is my review of the changes.", encoding="utf-8") + with pytest.raises(ValueError, match="not valid JSON"): + parse_review_output(prose) diff --git a/eval/tests/test_runner_hardening.py b/eval/tests/test_runner_hardening.py index be196750b..51020cf7e 100644 --- a/eval/tests/test_runner_hardening.py +++ b/eval/tests/test_runner_hardening.py @@ -1,6 +1,7 @@ """Regression tests for benchmark evidence and phase-boundary hardening.""" import hashlib +import inspect import json import shutil from contextlib import nullcontext @@ -1002,3 +1003,43 @@ def test_progress_line_reports_the_numbers_a_real_run_measured(): assert "cost=$0.5" in line assert "took=12.0s" in line assert "error_kind=none" in line + + +def test_review_artifact_is_mounted_as_a_writable_directory_outside_the_workspace(): + """The regression that produced fifteen runs of empty evidence. + + A writable FILE inside a read-only directory is not writable to anything + that writes atomically. The Write tool creates `.tmp..` + beside the target and renames it, so a read-only parent fails the temp + create with EROFS and the artifact stays 0 bytes. The mount target must be + the directory, and it must sit outside the read-only workspace. + """ + + assert not runner.SANDBOX_REVIEW_OUTPUT.startswith(runner.SANDBOX_WORKSPACE + "/") + assert runner.SANDBOX_REVIEW_OUTPUT != runner.SANDBOX_WORKSPACE + + source = inspect.getsource(runner.run_arm) + mount = source[source.index("extra_writable_mounts=(") : source.index("with sandbox_workspace_write_boundary")] + assert "source=review_output.parent" in mount, "mount the directory, not the file" + assert "target=SANDBOX_REVIEW_OUTPUT" in mount + assert f"{{SANDBOX_WORKSPACE}}/{{REVIEW_OUTPUT}}" not in mount + + +def test_review_contract_tells_the_agent_the_writable_path(): + prompt = runner.REVIEW_PROMPT.format(task="task text") + assert f"{runner.SANDBOX_REVIEW_OUTPUT}/{runner.REVIEW_OUTPUT}" in prompt + assert f"{runner.SANDBOX_WORKSPACE}/{runner.REVIEW_OUTPUT}" not in prompt + # The JSON shape survives .format() with its braces intact. + assert '{"schema_version":1' in prompt + artifact = f"{runner.SANDBOX_REVIEW_OUTPUT}/{runner.REVIEW_OUTPUT}" + assert runner.CE_REVIEW_PROMPT.format(task="task text").count(artifact) == 1 + + +def test_enforce_phase_workspace_can_require_an_untouched_workspace(tmp_path): + (tmp_path / "tracked.py").write_text("original\n") + before = runner_artifacts.workspace_snapshot(tmp_path) + runner_artifacts.enforce_phase_workspace(tmp_path, before, allowed_artifact=None) + + (tmp_path / "tracked.py").write_text("the review edited the code it was reviewing\n") + with pytest.raises(ValueError, match="changed the read-only workspace"): + runner_artifacts.enforce_phase_workspace(tmp_path, before, allowed_artifact=None) diff --git a/eval/tests/test_workflow_bench_sessions.py b/eval/tests/test_workflow_bench_sessions.py index 9a7eb4e87..b4ae6c698 100644 --- a/eval/tests/test_workflow_bench_sessions.py +++ b/eval/tests/test_workflow_bench_sessions.py @@ -120,11 +120,16 @@ def skill_events(skill_input: dict, *, tool_id: str = "skill-1", is_error: bool def fake_sandbox(root: Path) -> SimpleNamespace: + # private_root is NOT the clone. Conflating them puts the review artifact + # directory inside the workspace, which the real sandbox never does and + # which hides whether the workspace was left untouched. + private_root = root.parent / f"{root.name}-sandbox-private" + private_root.mkdir(exist_ok=True) return SimpleNamespace( backend="test-double", claude_bin="claude", clone=root, - private_root=root, + private_root=private_root, command_prefix=[], command_prefix_for=lambda **_kwargs: [], settings_json="{}", @@ -1249,7 +1254,7 @@ def test_planning_cannot_change_source_tests_or_downstream_skill(monkeypatch, tm @pytest.mark.parametrize( ("attack", "expected_detail"), [ - ("workspace", "unauthorized workspace path"), + ("workspace", "changed the read-only workspace"), ("skill", "changed the evaluated skill fingerprint"), ], ) @@ -1265,9 +1270,11 @@ def test_review_phase_rejects_workspace_or_skill_mutation( expected_skill_digest = "expected-skill-fingerprint" def adversarial_review(prompt, *args, **kwargs): - (tmp_path / "review-output.json").write_text( - '{"schema_version":1,"verdict":"approve","findings":[]}' - ) + # Write where the contract now says: the artifact directory outside the + # workspace, which is the only place the agent can write atomically. + artifact = runner.review_output_path(sandbox, runner.REVIEW_OUTPUT) + artifact.parent.mkdir(parents=True, exist_ok=True) + artifact.write_text('{"schema_version":1,"verdict":"approve","findings":[]}') if attack == "workspace": source.write_text("review silently changed source") return session_record() diff --git a/eval/workflow_bench/proposer_sandbox.py b/eval/workflow_bench/proposer_sandbox.py index a2dfaa54a..d44a898c6 100644 --- a/eval/workflow_bench/proposer_sandbox.py +++ b/eval/workflow_bench/proposer_sandbox.py @@ -22,6 +22,15 @@ from .process_control import ManagedProcessResult, run_managed MAX_EVIDENCE_FILE_BYTES = 256 * 1024 MAX_BUNDLE_BYTES = 2 * 1024 * 1024 SANDBOX_WORKSPACE = "/workspace" +# The review artifact lives OUTSIDE the workspace, in its own writable +# directory. A writable FILE inside a read-only directory is not writable to +# anything that writes atomically: the Write tool creates +# `.tmp..` beside the target and renames it, so a read-only +# parent fails the temp create with EROFS and the artifact is never written. +# Binding a writable directory outside /workspace lets the rename land while +# the workspace itself stays entirely read-only. +SANDBOX_REVIEW_OUTPUT = "/review-output" +REVIEW_OUTPUT_DIRNAME = "review-output" SANDBOX_HOME = "/home/agent" SANDBOX_TMP = "/tmp" SANDBOX_CLAUDE = "/opt/claude/claude" @@ -118,36 +127,41 @@ REVIEW_RUNTIME_DIRECTORIES = ( ) -def prepare_review_workspace(sandbox: SandboxSession, artifact_name: str) -> Path: - """Prepare disposable mount targets; never truncate a pre-existing entry.""" +def review_output_path(sandbox: SandboxSession, artifact_name: str) -> Path: + """Host path of the review artifact: a private directory, not the clone. + + One source of truth for the location, so the mount, the parse and the + artifact copy cannot drift apart. + """ - clone = _real_directory(sandbox.clone, label="review clone") if PurePosixPath(artifact_name).name != artifact_name or "\\" in artifact_name or artifact_name in ("", ".", ".."): raise SandboxError("review artifact must be a root filename") - output = clone / artifact_name - # No agent runs while this private clone is being prepared. On POSIX the - # directory descriptor additionally binds the exclusive create to its owner. - directory_fd = None - try: - if os.name != "nt": - directory_fd = os.open(clone, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW) - fd = os.open( - artifact_name if directory_fd is not None else output, - os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0), - 0o600, - dir_fd=directory_fd, - ) - try: - if not stat.S_ISREG(os.fstat(fd).st_mode): - raise SandboxError("review artifact must be a regular file") - finally: - os.close(fd) - except FileExistsError as exc: - raise SandboxError("review artifact already exists") from exc - finally: - if directory_fd is not None: - os.close(directory_fd) + return Path(sandbox.private_root) / REVIEW_OUTPUT_DIRNAME / artifact_name + +def prepare_review_workspace(sandbox: SandboxSession, artifact_name: str) -> Path: + """Prepare disposable mount targets; never truncate a pre-existing entry. + + Creates the artifact's own directory and returns the path the agent is + expected to write. The file itself is deliberately NOT pre-created: the + agent writes it atomically (temp file beside the target, then rename), so + the directory is what has to be writable, and an existing empty file would + only be something for the write to trip over. Absence is meaningful — it is + how ``parse_review_output`` tells "never written" from "written badly". + """ + + output = review_output_path(sandbox, artifact_name) + # No agent runs while this private root is being prepared, and the + # exclusive create is what proves the directory is ours rather than + # something a previous cell left behind. + try: + output.parent.mkdir(mode=0o700, parents=False, exist_ok=False) + except FileExistsError as exc: + raise SandboxError("review artifact directory already exists") from exc + except OSError as exc: + raise SandboxError(f"review artifact directory is unavailable: {output.parent}") from exc + + clone = _real_directory(sandbox.clone, label="review clone") if sandbox.backend != "bwrap": return output created: list[str] = [] diff --git a/eval/workflow_bench/review_scoring.py b/eval/workflow_bench/review_scoring.py index b8ad0151b..e7672f736 100644 --- a/eval/workflow_bench/review_scoring.py +++ b/eval/workflow_bench/review_scoring.py @@ -112,13 +112,30 @@ def _parse_review_finding(raw: Any, index: int) -> ReviewFinding: def parse_review_output(path: Path) -> tuple[str, tuple[ReviewFinding, ...]]: - metadata = path.lstat() + # Distinguish these. Folding them into one message is how a sandbox that + # made the artifact impossible to write read for 15 runs as an encoding + # fault: every cell reported "not valid UTF-8 JSON" for a file the agent + # was never able to create. + try: + metadata = path.lstat() + except FileNotFoundError as exc: + raise ValueError("review output was never written") from exc + except OSError as exc: + raise ValueError(f"review output is unreadable: {exc.strerror}") from exc if path.is_symlink() or not path.is_file() or metadata.st_size > MAX_REVIEW_BYTES: raise ValueError("review output must be a bounded regular non-symlink file") + if metadata.st_size == 0: + raise ValueError("review output is empty") try: - raw = json.loads(path.read_text()) - except (OSError, UnicodeError, json.JSONDecodeError) as exc: - raise ValueError("review output is not valid UTF-8 JSON") from exc + text = path.read_text(encoding="utf-8") + except OSError as exc: + raise ValueError(f"review output is unreadable: {exc.strerror}") from exc + except UnicodeError as exc: + raise ValueError("review output is not valid UTF-8") from exc + try: + raw = json.loads(text) + except json.JSONDecodeError as exc: + raise ValueError(f"review output is not valid JSON: {exc.msg} at line {exc.lineno}") from exc if not isinstance(raw, Mapping) or set(raw) != {"schema_version", "verdict", "findings"}: raise ValueError("review output requires exactly schema_version, verdict, and findings") if raw["schema_version"] != REVIEW_SCHEMA_VERSION: diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index 3be83916b..72a2a412c 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -103,6 +103,7 @@ from .proposer_sandbox import ( SANDBOX_GITNEXUS_REGISTRY, SANDBOX_GITNEXUS_SHARED as SANDBOX_GITNEXUS_SHARED, SANDBOX_NODE as SANDBOX_NODE, + SANDBOX_REVIEW_OUTPUT, SANDBOX_WORKSPACE, ReadOnlyMount, SandboxError, @@ -113,6 +114,7 @@ from .proposer_sandbox import ( prepare_sandbox, prepare_review_workspace, redact_text, + review_output_path, require_claude_sandbox_helpers, sandbox_workspace_write_boundary, ) @@ -232,8 +234,9 @@ CE_WORK_DIRECT_PROMPT = ( # Review cell: setup applies a historical PR diff, then the model sees a # read-only checkout. Both arms emit the same strict artifact so quality can be # scored deterministically against labels that remain hidden until it exits. -REVIEW_OUTPUT_CONTRACT = """ -Write /workspace/review-output.json as UTF-8 JSON with exactly this shape: +# Concatenated, not an f-string: the JSON shape below keeps its braces doubled +# because the finished prompt is .format()-ed with the task text. +REVIEW_OUTPUT_CONTRACT = f"\nWrite {SANDBOX_REVIEW_OUTPUT}/{REVIEW_OUTPUT} " + """as UTF-8 JSON with exactly this shape: {{"schema_version":1,"verdict":"approve|comment|request_changes","findings":[{{ "id":"unique stable id","severity":"critical|high|medium|low", "path":"repository-relative changed file","line":1,"end_line":1, @@ -573,9 +576,13 @@ def run_arm( read_only_workspace=True, read_only_paths=_evaluated_skill_roots(worktree, arm), extra_writable_mounts=( + # The DIRECTORY, outside the workspace. Binding the file + # itself left the agent nowhere to put the temp file it + # renames into place, so every review artifact came back + # empty with EROFS in the transcript. ReadOnlyMount( - source=review_output, - target=f"{SANDBOX_WORKSPACE}/{REVIEW_OUTPUT}", + source=review_output.parent, + target=SANDBOX_REVIEW_OUTPUT, ), ), ), @@ -583,7 +590,8 @@ def run_arm( with sandbox_workspace_write_boundary( sandbox, read_only_workspace=True, - writable=(review_output,), + # Nothing in the workspace is writable now — the artifact left it. + writable=(), ): review_session = run_claude( host_text(review_prompt.format(task=task["prompt"])), @@ -594,11 +602,9 @@ def run_arm( sessions.append(review_session) if review_session["ok"] and phase_before is not None: try: - enforce_phase_workspace( - worktree, - phase_before, - allowed_artifact=worktree / REVIEW_OUTPUT, - ) + # The artifact is no longer in the workspace, so the review + # phase may now change nothing there at all. + enforce_phase_workspace(worktree, phase_before, allowed_artifact=None) require_skill_fingerprint( worktree, arm, @@ -669,13 +675,16 @@ def run_arm( review_score: dict[str, Any] | None = None if arm in ("review", "ce_review"): try: - verdict, findings = parse_review_output(worktree / REVIEW_OUTPUT) + verdict, findings = parse_review_output(review_output_path(sandbox, REVIEW_OUTPUT)) labels = expected_findings(oracle_snapshot) if oracle_snapshot is not None else () review_score = score_review(verdict, findings, labels) except (OSError, ValueError) as exc: record["ok"] = False record["error_kind"] = record["error_kind"] or "review-evidence-invalid" - record["error_detail"] = str(exc) + # Keep the FIRST detail, as error_kind already does. A phase- + # boundary violation is why the artifact is unparseable; reporting + # the parse failure over it buries the cause under the symptom. + record["error_detail"] = record.get("error_detail") or str(exc) record["review_score"] = review_score record["review_evidence_valid"] = review_score is not None if review_score is not None: @@ -1032,7 +1041,7 @@ def run_cell(ctx: TaskCellContext, run_idx: int, arm: str) -> dict[str, Any]: oracle_snapshot=ctx.oracle_snapshot, ) if execution_arm in ("review", "ce_review"): - review_source = worktree / REVIEW_OUTPUT + review_source = review_output_path(sandbox, REVIEW_OUTPUT) if review_source.is_file() and not review_source.is_symlink(): review_artifact = ctx.out_dir / f"{task['id']}-{arm}-run{run_idx}.review.json" review_artifact.write_bytes(_bounded_regular_bytes(review_source, limit=256 * 1024)) diff --git a/eval/workflow_bench/runner_artifacts.py b/eval/workflow_bench/runner_artifacts.py index 02dbd8a38..bdf46c8b8 100644 --- a/eval/workflow_bench/runner_artifacts.py +++ b/eval/workflow_bench/runner_artifacts.py @@ -217,11 +217,25 @@ def enforce_phase_workspace( worktree: Path, before: dict[str, str], *, - allowed_artifact: Path, + allowed_artifact: Path | None, ) -> None: - """Require a phase to change only its one explicit workspace artifact.""" + """Require a phase to change only its one explicit workspace artifact. + + ``allowed_artifact=None`` is the stricter contract: the phase must leave + the workspace byte-identical. That is what a review phase whose artifact + lives outside the workspace has to satisfy — there is nothing in there it + is entitled to touch. + """ root = worktree.expanduser().absolute() + if allowed_artifact is None: + after = workspace_snapshot(root) + changed = sorted( + path for path in before.keys() | after.keys() if before.get(path) != after.get(path) + ) + if changed: + raise ValueError(f"phase changed the read-only workspace: {', '.join(changed[:5])}") + return artifact = allowed_artifact.expanduser().absolute() try: relative = PurePosixPath(artifact.relative_to(root).as_posix())