mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-19 00:03:33 +00:00
fix(eval): give the review agent somewhere it can actually write
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 `<target>.tmp.<n>.<hex>` 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) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
87a5764c5f
commit
ccc9e470d2
8 changed files with 247 additions and 67 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 `<target>.tmp.<n>.<hex>`
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# `<target>.tmp.<n>.<hex>` 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] = []
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue