From b97ad89f384fe508b3a29433df17fe9efbdb1836 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Mon, 7 Sep 2026 17:02:37 +0000 Subject: [PATCH] fix(eval): address PR review feedback (#3207) - aggregate: count admissible rows directly instead of subtracting the execution and evidence counters, which double-charged a row that is both a session error and invalid review evidence and could report UNUSABLE for an arm holding real measurements. - run_proposer: bound the session timeout by what is left of --max-runtime-seconds, so clearing the sweep minimum cannot start a full-length session past the instance window. - comparator reuse: hold one O_NOFOLLOW descriptor for the size check, digest and copy, and prove it is the inode that was checked, closing the swap window a concurrent writer of the reuse directory had. - Drive the review-artifact mount assertion through run_arm and the clone-template assertion through run_cell, instead of rebuilding the expected values in the tests (also removes the CodeQL unnecessary lambda). - Assert the workflow invokes run-evolution.sh rather than that its YAML mentions --max-runtime-seconds, which only appears in a comment. - Correct the parse_review_output failure-mode claim: the fold was empty artifacts reported as "not valid UTF-8 JSON"; a never-created file raised FileNotFoundError. - prettier: wrap the over-long readFileSync call flagged by PR autofix. Note: pre-existing failure in tests/test_model_gateway.py::test_locked_litellm_translates_messages_to_offline_responses (local LiteLLM proxy never becomes ready in this environment) not addressed by this PR. Co-Authored-By: Claude Opus 5 (1M context) --- eval/tests/test_comparator_reuse.py | 26 +++++ eval/tests/test_evolve.py | 46 ++++++++ eval/tests/test_review_scoring.py | 8 +- eval/tests/test_runner_hardening.py | 105 +++++++++++------- eval/tests/test_workflow_bench.py | 19 ++++ eval/tests/test_workflow_bench_sessions.py | 87 +++++++++------ eval/workflow_bench/comparator_reuse.py | 75 +++++++++---- eval/workflow_bench/evolve.py | 18 ++- eval/workflow_bench/review_scoring.py | 11 +- eval/workflow_bench/runner.py | 35 ++++-- .../unit/skill-evolution-workflow.test.ts | 13 ++- 11 files changed, 320 insertions(+), 123 deletions(-) diff --git a/eval/tests/test_comparator_reuse.py b/eval/tests/test_comparator_reuse.py index d56a09a1d..3ae739936 100644 --- a/eval/tests/test_comparator_reuse.py +++ b/eval/tests/test_comparator_reuse.py @@ -175,6 +175,32 @@ def test_materialize_copies_transcript_and_review_artifacts(tmp_path: Path) -> N assert copied["transcript_artifacts"][0]["sha256"] == hashlib.sha256(payload).hexdigest() +def test_a_reused_artifact_is_copied_from_the_inode_that_was_checked(tmp_path: Path) -> None: + """The reuse source is a directory another sweep wrote and may still write. + + Validating a path and then re-opening it hands a concurrent writer the gap: + replace the checked file with a symlink and the copy follows it out of the + results directory. Swapping the path while the descriptor is held is that + same substitution, made deterministic. + """ + + source = tmp_path / "transcript.jsonl" + source.write_bytes(b"verified\n") + decoy = tmp_path / "decoy.jsonl" + decoy.write_bytes(b"substituted\n") + destination = tmp_path / "copy.jsonl" + + with comparator_reuse._open_regular(source, label="transcript") as descriptor: + source.unlink() + source.symlink_to(decoy) + comparator_reuse._copy_owner_only(descriptor, destination) + + assert destination.read_bytes() == b"verified\n" + with pytest.raises(SandboxError, match="regular non-symlink"): + with comparator_reuse._open_regular(source, label="transcript"): + pass + + def test_materialize_rejects_same_directory_and_missing_transcript(tmp_path: Path) -> None: source = tmp_path / "prior" source.mkdir() diff --git a/eval/tests/test_evolve.py b/eval/tests/test_evolve.py index acf7bd85c..510946244 100644 --- a/eval/tests/test_evolve.py +++ b/eval/tests/test_evolve.py @@ -9,6 +9,7 @@ import time from contextlib import contextmanager from datetime import UTC, datetime, timedelta from pathlib import Path +from types import SimpleNamespace import pytest @@ -667,6 +668,51 @@ def test_run_proposer_hides_the_hidden_harness_and_keeps_the_full_tool_surface(m assert captured["settings_json"] == FakeSandbox.settings_json +def test_proposer_session_cannot_outlive_the_remaining_instance_window(monkeypatch, tmp_path): + """Clearing the sweep minimum is not a licence to run a full session. + + --timeout is sized for a whole generation, so a proposer started with the + minimum left would run far past --max-runtime-seconds and the box would take + the evidence with it. + """ + + captured: dict[str, object] = {} + + @contextmanager + def fake_prepare_sandbox(**_kwargs): + yield SimpleNamespace( + claude_bin="claude", + command_prefix=[], + settings_json="{}", + transcript_projects=tmp_path / "transcript-projects", + ) + + def fake_run_claude(*_args, **kwargs): + captured.update(kwargs) + return {"ok": False, "error_kind": "session-error"} + + monkeypatch.setattr(evolve.runner, "make_worktree", lambda _repo, _ref, destination: destination) + monkeypatch.setattr(evolve.runner, "remove_clone", lambda _clone: None) + monkeypatch.setattr(evolve, "sanitize_clone_for_hidden_oracles", lambda _clone: "0" * 40) + monkeypatch.setattr(evolve, "prepare_sandbox", fake_prepare_sandbox) + monkeypatch.setattr(evolve.runner, "run_claude", fake_run_claude) + args = build_parser().parse_args(["--tasks", "tasks.yaml", "--model", "model"]) + assert args.timeout > evolve.MIN_INSTANCE_SWEEP_SECONDS, "otherwise this test proves nothing" + + common = { + "overlay_dir": tmp_path / "overlay", + "proposal_path": tmp_path / "proposal.md", + "evidence_bundle": tmp_path / "evidence", + "bwrap_bin": tmp_path / "bwrap", + } + evolve.run_proposer("prompt", args, **common, remaining_seconds=evolve.MIN_INSTANCE_SWEEP_SECONDS + 1) + assert captured["timeout"] == evolve.MIN_INSTANCE_SWEEP_SECONDS + 1 + + # No cap configured means no budget to overrun: the session keeps its own. + evolve.run_proposer("prompt", args, **common) + assert captured["timeout"] == args.timeout + + def test_parser_defaults_match_the_gate_minimums(): args = build_parser().parse_args(["--tasks", "t.yaml", "--model", "pinned"]) assert args.runs == 3 diff --git a/eval/tests/test_review_scoring.py b/eval/tests/test_review_scoring.py index eb105a57a..1e6c8922e 100644 --- a/eval/tests/test_review_scoring.py +++ b/eval/tests/test_review_scoring.py @@ -330,9 +330,11 @@ def test_clean_control_rewards_an_empty_approval_and_penalizes_noise(): def test_parse_review_output_names_the_actual_failure(tmp_path: Path): """One message per cause. - Folding these together makes a sandbox that renders the artifact impossible - to write indistinguishable from an encoding fault: every cell reports "not - valid UTF-8 JSON" for a file the agent was never able to create. + Folding empty, malformed and encoding failures together makes a sandbox that + left the artifact at 0 bytes indistinguishable from an encoding fault: every + such cell reports "not valid UTF-8 JSON". A file the agent never created + escaped that fold — lstat sat outside the try, so it raised + FileNotFoundError — but only as a bare OSError, naming no cause at all. """ missing = tmp_path / "never-written.json" diff --git a/eval/tests/test_runner_hardening.py b/eval/tests/test_runner_hardening.py index 425109a94..111b3c3c5 100644 --- a/eval/tests/test_runner_hardening.py +++ b/eval/tests/test_runner_hardening.py @@ -3,6 +3,7 @@ import hashlib import json import shutil +import subprocess from contextlib import nullcontext from pathlib import Path from types import SimpleNamespace @@ -608,6 +609,67 @@ def test_run_cell_reports_a_cleanup_failure_over_its_primary_outcome(monkeypatch assert "clone is busy" in record["error_detail"] +def _git(repo, *args): + return subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True, text=True) + + +def test_run_cell_runs_the_arm_against_a_copy_of_the_clone_template(monkeypatch, tmp_path): + """run_cell must copy the template, never re-clone. + + run_cell takes the clone-template branch on essentially every multi-cell + sweep: it copies a pre-sanitized template rather than paying `git clone + --no-local` plus repack/prune/fsck per cell. Asserting on a copy the test + makes itself proves nothing about that branch — the clone the arm receives + is what has to come from the template, carrying the template's sanitized + HEAD rather than a recomputed one. + """ + + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "--quiet") + _git(repo, "checkout", "--quiet", "-b", "main") + (repo / "from-template.txt").write_text("sanitized\n") + _git(repo, "add", "-A") + _git(repo, "-c", "user.name=test", "-c", "user.email=test@invalid", "commit", "--quiet", "-m", "base") + sha = _git(repo, "rev-parse", "HEAD").stdout.strip() + trees = tmp_path / "trees" + trees.mkdir() + template = runner.make_worktree(repo, sha, trees) + template_head = _git(template, "rev-parse", "HEAD").stdout.strip() + + _stub_cell_dependencies(monkeypatch, tmp_path) + + def fail_if_recloned(*_args, **_kwargs): + raise AssertionError("clone template present: run_cell must not re-clone") + + monkeypatch.setattr(runner, "make_worktree", fail_if_recloned) + monkeypatch.setattr(runner, "sanitize_clone_for_hidden_oracles", fail_if_recloned) + + seen: dict[str, object] = {} + + def record_arm(_arm, _task, worktree, _args, **_kwargs): + seen["worktree"] = worktree + seen["head"] = _git(worktree, "rev-parse", "HEAD").stdout.strip() + seen["content"] = (worktree / "from-template.txt").read_text() + # The copy is a private checkout: what the cell writes must not reach + # the template the other cells of this task still copy from. + (worktree / "from-template.txt").write_text("cell-local\n") + return {"resolved": True, "ok": True, "error_kind": None} + + monkeypatch.setattr(runner, "run_arm", record_arm) + + runner.run_cell( + _cell_context(tmp_path, clone_template=template, sanitized_head=template_head), + 0, + "workflow", + ) + + assert seen["content"] == "sanitized\n" + assert seen["head"] == template_head + assert seen["worktree"] != template + assert (template / "from-template.txt").read_text() == "sanitized\n" + + 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. @@ -1004,49 +1066,6 @@ def test_progress_line_reports_the_numbers_a_real_run_measured(): assert "error_kind=none" in line -def test_review_artifact_is_mounted_as_a_writable_directory_outside_the_workspace(tmp_path): - """A writable file inside a read-only directory is not a writable path. - - 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 - - # Capture the real mount tuple run_arm builds, rather than matching source - # text: a string match passes on any wrong value whose literals survive, and - # fails on a behaviour-preserving refactor. - captured: dict[str, object] = {} - - class _Recorder(SimpleNamespace): - def command_prefix_for(self, **kwargs): - captured.update(kwargs) - return [] - - sandbox = _Recorder( - backend="test-double", - clone=tmp_path, - private_root=tmp_path / "private", - settings_json="{}", - host_text=lambda value: value, - host_path=lambda value: str(value), - ) - sandbox.private_root.mkdir(exist_ok=True) - review_output = runner.review_output_path(sandbox, runner.REVIEW_OUTPUT) - mounts = ( - runner.ReadOnlyMount(source=review_output.parent, target=runner.SANDBOX_REVIEW_OUTPUT), - ) - assert mounts[0].source == review_output.parent, "mount the directory, not the file" - assert mounts[0].target == runner.SANDBOX_REVIEW_OUTPUT - assert not mounts[0].target.startswith(f"{runner.SANDBOX_WORKSPACE}/") - # The artifact the harness later reads is the one inside that mount. - assert review_output.parent in review_output.parents - - def test_claude_settings_allow_the_review_artifact_directory(): """The second gate on the artifact path. diff --git a/eval/tests/test_workflow_bench.py b/eval/tests/test_workflow_bench.py index 34aecf64b..1d040c546 100644 --- a/eval/tests/test_workflow_bench.py +++ b/eval/tests/test_workflow_bench.py @@ -751,6 +751,25 @@ def test_one_admissible_cell_leaves_an_arm_degraded_not_healthy(): assert health.admissible == 1 +def test_a_row_that_fails_both_ways_is_only_subtracted_once(): + """run_arm can produce a row that is an execution AND an evidence failure. + + It keeps the first error_kind — a session-error survives — and still sets + review_evidence_valid=False when the artifact will not parse. Counting that + row against admissible twice zeroed an arm that held a real measurement, + which arm_health reports as UNUSABLE and the measurement gate then fails on. + """ + + both = _cell(resolved=False, ok=False, error_kind="session-error", review_evidence_valid=False) + results = _arms(review=[both, _cell(resolved=True, error_kind="oracle-failed")]) + health = arm_health(results, {"review"})["review"] + assert (health.execution_failures, health.evidence_failures) == (1, 1) + assert health.fresh_attempts == 2 + assert health.admissible == 1 + assert health.status == "DEGRADED" + assert unhealthy_arms(results, {"review"}) == [] + + def test_reused_rows_alone_leave_current_health_unknown(): """Historical success cannot certify this sweep's environment.""" diff --git a/eval/tests/test_workflow_bench_sessions.py b/eval/tests/test_workflow_bench_sessions.py index 267dba8b8..c2abd65c0 100644 --- a/eval/tests/test_workflow_bench_sessions.py +++ b/eval/tests/test_workflow_bench_sessions.py @@ -1305,6 +1305,57 @@ def test_review_phase_rejects_workspace_or_skill_mutation( assert expected_detail in rec["error_detail"] +@pytest.mark.parametrize("arm", ["review", "ce_review"]) +def test_run_arm_mounts_the_review_artifact_directory_outside_the_workspace(monkeypatch, tmp_path, arm): + """A writable FILE inside a read-only directory is not a writable path. + + 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. + + Driven through run_arm rather than rebuilt here: an expected tuple assembled + in the test passes whatever run_arm actually mounts, which is the one thing + this needs to prove. + """ + + assert not runner.SANDBOX_REVIEW_OUTPUT.startswith(runner.SANDBOX_WORKSPACE + "/") + assert runner.SANDBOX_REVIEW_OUTPUT != runner.SANDBOX_WORKSPACE + + verify_calls: list[dict] = [] + sandbox = fake_sandbox(tmp_path) + sandbox.command_prefix_for = lambda **kwargs: verify_calls.append(kwargs) or [] + + def review_session(prompt, *args, **kwargs): + 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":[]}') + return session_record() + + monkeypatch.setattr(runner, "run_claude", review_session) + monkeypatch.setattr(runner, "skill_fingerprint", lambda *_a, **_k: "skill-digest") + monkeypatch.setattr(runner, "run_verify", lambda *a, **k: (True, "ok")) + + runner.run_arm( + arm, + {"prompt": "p", "verify": "true"}, + tmp_path, + bench_args(), + sandbox=sandbox, + expected_skill_digest="skill-digest", + ) + + review_output = runner.review_output_path(sandbox, runner.REVIEW_OUTPUT) + mounts = [call["extra_read_only_mounts"] for call in verify_calls if "extra_read_only_mounts" in call] + assert mounts, "the verify invocation must be given the artifact mount" + assert mounts[-1] == ( + runner.ReadOnlyMount(source=review_output.parent, target=runner.SANDBOX_REVIEW_OUTPUT), + ), "mount the directory, not the file" + assert not mounts[-1][0].target.startswith(f"{runner.SANDBOX_WORKSPACE}/") + # The artifact the harness later reads is the one inside that mount. + assert review_output.parent in review_output.parents + + def _git(repo, *args, check=True): return subprocess.run(["git", "-C", str(repo), *args], check=check, capture_output=True, text=True) @@ -1378,39 +1429,3 @@ def test_copy_isolated_tree_does_not_share_git_objects_or_refs(tmp_path): assert copy_head == template_head == sha alternates = copy / ".git" / "objects" / "info" / "alternates" assert not alternates.exists() - - -def test_run_cell_uses_the_clone_template_instead_of_recloning(tmp_path, monkeypatch): - """run_cell must copy the template, never re-clone. - - run_cell takes the clone-template branch on essentially every multi-cell - sweep: it copies a pre-sanitized template rather than paying `git clone - --no-local` plus repack/prune/fsck per cell. Nothing asserted that the copy - is what the cell actually runs against, or that the template's sanitized - HEAD is carried through rather than recomputed. - """ - - repo = tmp_path / "repo" - repo.mkdir() - _git(repo, "init", "--quiet") - _git(repo, "checkout", "--quiet", "-b", "main") - sha = _git_commit(repo, "base") - clones = tmp_path / "clones" - clones.mkdir() - template = runner.make_worktree(repo, sha, clones) - (template / "from-template.txt").write_text("sanitized\n") - sanitized_head = _git(template, "rev-parse", "HEAD").stdout.strip() - - def fail_if_recloned(*args, **kwargs): - raise AssertionError("clone template present: run_cell must not re-clone") - - monkeypatch.setattr(runner, "make_worktree", fail_if_recloned) - monkeypatch.setattr(runner, "sanitize_clone_for_hidden_oracles", fail_if_recloned) - - worktree = runner.copy_isolated_tree(template, clones) - assert (worktree / "from-template.txt").read_text() == "sanitized\n" - assert _git(worktree, "rev-parse", "HEAD").stdout.strip() == sanitized_head - # The copy is a private checkout: writing it must not touch the template the - # other cells of this task still copy from. - (worktree / "from-template.txt").write_text("cell-local\n") - assert (template / "from-template.txt").read_text() == "sanitized\n" diff --git a/eval/workflow_bench/comparator_reuse.py b/eval/workflow_bench/comparator_reuse.py index 71d1d15cd..35353fa1b 100644 --- a/eval/workflow_bench/comparator_reuse.py +++ b/eval/workflow_bench/comparator_reuse.py @@ -19,7 +19,8 @@ import json import os import re import stat -from collections.abc import Mapping, Sequence +from collections.abc import Iterator, Mapping, Sequence +from contextlib import contextmanager from dataclasses import dataclass from datetime import UTC, datetime, timedelta from pathlib import Path, PurePosixPath @@ -360,18 +361,20 @@ def _resolved_directory(path: Path, *, label: str) -> Path: def _copy_transcript_artifact(source: Path, dest: Path, metadata: Mapping[str, Any]) -> dict[str, Any]: relative, expected_digest, expected_size = _transcript_metadata(metadata) - source_file = _regular_file(source / Path(*PurePosixPath(relative).parts), label="transcript") - actual_size = source_file.stat().st_size - if actual_size != expected_size: - raise SandboxError(f"reused transcript size drifted: {relative}") - digest = _sha256_file(source_file) - if digest != expected_digest: - raise SandboxError(f"reused transcript digest drifted: {relative}") dest_dir = dest / "transcripts" dest_dir.mkdir(mode=0o700, exist_ok=True) dest_dir.chmod(0o700) destination = dest_dir / PurePosixPath(relative).name - _copy_owner_only(source_file, destination) + # One descriptor for the size check, the digest and the copy. Re-opening the + # path between them is what let a concurrent writer swap the checked file + # for a symlink and have the copy follow it. + with _open_regular(source / Path(*PurePosixPath(relative).parts), label="transcript") as source_fd: + if os.fstat(source_fd).st_size != expected_size: + raise SandboxError(f"reused transcript size drifted: {relative}") + digest = _sha256_descriptor(source_fd) + if digest != expected_digest: + raise SandboxError(f"reused transcript digest drifted: {relative}") + _copy_owner_only(source_fd, destination) return {"path": relative, "sha256": digest, "bytes": expected_size, "source": PARENT_EVENT_STREAM_SOURCE} @@ -379,21 +382,42 @@ def _copy_named_artifact(source: Path, dest: Path, name: str, *, label: str) -> relative = PurePosixPath(name) if relative.is_absolute() or len(relative.parts) != 1 or relative.parts[0] in {"", ".", ".."}: raise SandboxError(f"unsafe {label} path: {name!r}") - source_file = _regular_file(source / name, label=label) - _copy_owner_only(source_file, dest / name) + with _open_regular(source / name, label=label) as source_fd: + _copy_owner_only(source_fd, dest / name) -def _regular_file(path: Path, *, label: str) -> Path: +@contextmanager +def _open_regular(path: Path, *, label: str) -> Iterator[int]: + """Open a regular non-symlink file and hold it open for every later read. + + Checking the path and then re-opening it is a race the reuse directory is + exposed to: it is written by a previous sweep and read by this one, so a + concurrent writer can replace a validated file with a symlink in between. + O_NOFOLLOW refuses the leaf link and the fstat comparison proves the open + descriptor is the inode that was checked — the same guarantee + evolution._bounded_regular_bytes makes for evidence files. + """ + try: - metadata = path.lstat() + before = path.lstat() except OSError as exc: raise SandboxError(f"{label} is missing: {path}: {exc}") from exc - if stat.S_ISLNK(metadata.st_mode) or not stat.S_ISREG(metadata.st_mode): + if stat.S_ISLNK(before.st_mode) or not stat.S_ISREG(before.st_mode): raise SandboxError(f"{label} must be a regular non-symlink file: {path}") - return path + try: + descriptor = os.open(path, os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0)) + except OSError as exc: + raise SandboxError(f"{label} is unreadable: {path}: {exc}") from exc + try: + opened = os.fstat(descriptor) + if not stat.S_ISREG(opened.st_mode) or (opened.st_dev, opened.st_ino) != (before.st_dev, before.st_ino): + raise SandboxError(f"{label} changed while opening: {path}") + yield descriptor + finally: + os.close(descriptor) -def _copy_owner_only(source: Path, destination: Path) -> None: +def _copy_owner_only(source: int, destination: Path) -> None: # O_CREAT|O_EXCL is the existence check, and unlike a stat beforehand it is # atomic: a file appearing between check and open cannot slip through. try: @@ -406,17 +430,20 @@ def _copy_owner_only(source: Path, destination: Path) -> None: raise SandboxError(f"reuse destination already exists: {destination}") from exc try: os.fchmod(descriptor, 0o600) - with open(source, "rb") as handle: - while True: - chunk = handle.read(COPY_CHUNK_BYTES) - if not chunk: - break - _write_all(descriptor, chunk) + os.lseek(source, 0, os.SEEK_SET) + while True: + chunk = os.read(source, COPY_CHUNK_BYTES) + if not chunk: + break + _write_all(descriptor, chunk) os.fsync(descriptor) finally: os.close(descriptor) -def _sha256_file(path: Path) -> str: - with open(path, "rb") as handle: +def _sha256_descriptor(descriptor: int) -> str: + os.lseek(descriptor, 0, os.SEEK_SET) + # dup so hashlib owns a file object it may close; the duplicate shares the + # offset, which is why every reader here seeks to 0 before it starts. + with os.fdopen(os.dup(descriptor), "rb") as handle: return hashlib.file_digest(handle, "sha256").hexdigest() diff --git a/eval/workflow_bench/evolve.py b/eval/workflow_bench/evolve.py index db8f54df9..bbc743f84 100644 --- a/eval/workflow_bench/evolve.py +++ b/eval/workflow_bench/evolve.py @@ -699,8 +699,17 @@ def run_proposer( bwrap_bin: Path, sandbox_backend: str = "bwrap", progress_label: str | None = None, + remaining_seconds: int | None = None, ) -> dict[str, Any]: - """Run one proposer in confinement and copy only validated outputs out.""" + """Run one proposer in confinement and copy only validated outputs out. + + ``remaining_seconds`` is what is left of ``--max-runtime-seconds``. The + per-session ``--timeout`` is sized for a whole generation, so a proposer + started with only the sweep minimum left would otherwise be allowed to run + far past the instance window the caller just checked. + """ + + session_timeout = args.timeout if remaining_seconds is None else max(1, min(args.timeout, remaining_seconds)) with tempfile.TemporaryDirectory(prefix="wfevolve-") as tmp: clone = runner.make_worktree(REPO_ROOT, "HEAD", Path(tmp)) @@ -737,7 +746,7 @@ def run_proposer( host_text(prompt), clone, claude_bin=sandbox.claude_bin, - timeout=args.timeout, + timeout=session_timeout, model=args.proposer_model, effort=args.effort, env=model_session_environment( @@ -1600,6 +1609,11 @@ def _run_generations( bwrap_bin=bwrap_bin, sandbox_backend=sandbox_backend, progress_label=f"gen {generation} proposer", + # Clearing the minimum is not a licence to run for a whole + # generation: the session timeout is the larger number, so + # without this a proposer started with 601s left could still + # burn the full --timeout past the instance window. + remaining_seconds=before_proposer, ) # Redact any API token echoed into the session record (e.g. an # error_detail stderr_tail) before it enters the uploaded artifact. diff --git a/eval/workflow_bench/review_scoring.py b/eval/workflow_bench/review_scoring.py index 051da2dae..eaf328e37 100644 --- a/eval/workflow_bench/review_scoring.py +++ b/eval/workflow_bench/review_scoring.py @@ -115,10 +115,13 @@ def _parse_review_finding(raw: Any, index: int) -> ReviewFinding: def parse_review_output(path: Path) -> tuple[str, tuple[ReviewFinding, ...]]: - # 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. + # Distinguish these. Folding empty, malformed and encoding failures into one + # message is how a sandbox that left the artifact at 0 bytes read for 15 + # runs as an encoding fault: json.loads("") raises, and every such cell + # reported "not valid UTF-8 JSON". A path the agent never created was not in + # that fold — the lstat below sat outside the try and raised + # FileNotFoundError — but it reached the caller as a bare OSError rather + # than saying what was wrong, which is why it is named here too. try: metadata = path.lstat() except FileNotFoundError as exc: diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index 903cc67c3..18b67dc71 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -778,6 +778,23 @@ EXCLUDED_ERROR_KINDS = REUSE_EXCLUDED_ERROR_KINDS EXECUTION_FAILURE_KINDS = frozenset({"session-error", "infra-error", "cleanup-failure", "cancelled"}) EVIDENCE_FAILURE_KINDS = frozenset({"review-evidence-invalid", "evidence-unverified", "skill-not-invoked"}) + +def execution_failed(record: Mapping[str, Any]) -> bool: + """The process or its tooling did not complete.""" + + return record.get("error_kind") in EXECUTION_FAILURE_KINDS + + +def evidence_failed(record: Mapping[str, Any]) -> bool: + """It completed, but what it produced cannot be trusted or scored.""" + + return ( + record.get("error_kind") in EVIDENCE_FAILURE_KINDS + or record.get("review_evidence_valid") is False + or record.get("transcript_missing") is True + ) + + SYSTEMIC_ERROR_KINDS = frozenset({"session-error", "infra-error", "cleanup-failure", "review-evidence-invalid"}) DEFAULT_OUTAGE_STREAK = 5 @@ -1310,17 +1327,17 @@ def aggregate(records: list[dict[str, Any]]) -> dict[str, Any]: ) fresh = [r for r in records if not r.get("reused")] out["fresh_attempts"] = len(fresh) - out["execution_failures"] = sum(1 for r in fresh if r.get("error_kind") in EXECUTION_FAILURE_KINDS) - out["evidence_failures"] = sum( - 1 - for r in fresh - if r.get("error_kind") in EVIDENCE_FAILURE_KINDS - or r.get("review_evidence_valid") is False - or r.get("transcript_missing") is True - ) + out["execution_failures"] = sum(1 for r in fresh if execution_failed(r)) + out["evidence_failures"] = sum(1 for r in fresh if evidence_failed(r)) # Admissible means the harness delivered a trustworthy measurement. It says # nothing about whether the answer was right, which is the whole point. - out["admissible"] = out["fresh_attempts"] - out["execution_failures"] - out["evidence_failures"] + # + # Count the rows that failed NEITHER way rather than subtracting both + # counters: run_arm keeps a pre-existing session error and still marks the + # review evidence invalid, so one row can land in both. Subtracting it twice + # drove an arm holding real measurements to admissible=0, which arm_health + # reads as UNUSABLE and enforce_measurement_health then fails the sweep on. + out["admissible"] = sum(1 for r in fresh if not execution_failed(r) and not evidence_failed(r)) out["health_reasons"] = sorted( { str(r.get("error_kind")) diff --git a/gitnexus/test/unit/skill-evolution-workflow.test.ts b/gitnexus/test/unit/skill-evolution-workflow.test.ts index 5b3890c3a..2bc4d1844 100644 --- a/gitnexus/test/unit/skill-evolution-workflow.test.ts +++ b/gitnexus/test/unit/skill-evolution-workflow.test.ts @@ -576,11 +576,20 @@ exit 1`); // A Friday dispatch inherits leftover uptime. The shared entrypoint — not // the workflow YAML — must cap the sweep so it fails in-process and the // always() upload still runs (run 33962002890). - const script = readFileSync(path.join(REPO_ROOT, 'eval/workflow_bench/run-evolution.sh'), 'utf8'); + const script = readFileSync( + path.join(REPO_ROOT, 'eval/workflow_bench/run-evolution.sh'), + 'utf8', + ); expect(script).toContain('--max-runtime-seconds'); expect(script).toContain('instance_window_budget_from_proc'); expect(script).toContain('export RUNTIME_DIGEST'); - expect(workflow).toContain('--max-runtime-seconds'); + // The workflow's half of that contract is calling the entrypoint, not + // naming the flag: its only occurrence in the YAML is the explanatory + // comment above, so asserting on it would reject a correct comment edit + // while passing a loop step that had stopped invoking the script at all. + expect(stepRun('Run the propose → benchmark → gate loop')).toContain( + './workflow_bench/run-evolution.sh --apply', + ); }); it('uploads benchmark evidence unconditionally, on a path it addresses itself', () => {