From ccc9e470d2a05bf135604f860228ee2e2b874e2c Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sun, 6 Sep 2026 15:20:11 +0000 Subject: [PATCH] fix(eval): give the review agent somewhere it can actually write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every review cell in the last recorded generation returned unusable evidence. Not some — all 41, across all three arms and all six tasks, at $3653 for the run. The transcripts say why, 127 times across 35 of 35 sessions: EROFS: read-only file system, open '/workspace/review-output.json.tmp.2.90a76e583b0c' The review arm mounted the artifact as a writable FILE at /workspace/review-output.json while binding /workspace read-only. The Write tool writes atomically: it creates `.tmp..` beside the target and renames it. The parent was read-only, so the temp create failed and the artifact was never written. A writable file inside a read-only directory is not writable to anything that writes atomically. Agents tried /proc/self/root/workspace/... and /proc/1/root/workspace/... to get around it; all 41 artifacts came back 0 bytes. The artifact now lives in its own writable directory bound at /review-output, outside the workspace. That is what a rename needs, and it lets the workspace get stricter rather than looser: the review phase may now change nothing there at all (enforce_phase_workspace gained allowed_artifact=None), where before it was entitled to one path inside it. The file is no longer pre-created — the agent writes it, and absence is now meaningful evidence. parse_review_output reported every one of these as "review output is not valid UTF-8 JSON". The file was empty, and its except folded OSError, UnicodeError and JSONDecodeError into that one string, so a sandbox that made writing impossible was indistinguishable from an encoding fault. That is why this read as an agent-quality problem for fifteen consecutive non-green runs. Each cause now names itself: never written, empty, not valid UTF-8, not valid JSON with the decoder's position. run_arm also keeps the FIRST error_detail, as it already did for error_kind, so a phase-boundary violation is no longer buried under the parse failure it causes. The test double conflated sandbox.private_root with the clone, which put the artifact directory inside the workspace and would have hidden the stricter check. Regression tests pin the mount shape in the generated bwrap argv, the contract path in the prompt, the four parse diagnostics, and the untouched-workspace contract. Verified by unit tests only: this container has unprivileged user namespaces disabled, so bwrap cannot run here and the mount was not exercised end to end. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Opus 5 (1M context) --- eval/tests/test_proposer_sandbox.py | 84 +++++++++++++++++----- eval/tests/test_review_scoring.py | 28 ++++++++ eval/tests/test_runner_hardening.py | 41 +++++++++++ eval/tests/test_workflow_bench_sessions.py | 17 +++-- eval/workflow_bench/proposer_sandbox.py | 66 ++++++++++------- eval/workflow_bench/review_scoring.py | 25 +++++-- eval/workflow_bench/runner.py | 35 +++++---- eval/workflow_bench/runner_artifacts.py | 18 ++++- 8 files changed, 247 insertions(+), 67 deletions(-) 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())