From b0030daf3a42436b7b0de8f8dc14837e0f2075e5 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Mon, 7 Sep 2026 06:14:39 +0000 Subject: [PATCH] refactor(eval): name the reuse directory check for the promise it makes The simplification pass left one finding open: comparator_reuse and proposer_sandbox both defined `_real_directory`, same name and same shape, with different guarantees. The sandbox one rejects every symlink hop in the path; the reuse one checks only the leaf and resolves through parents. Sharing the name invites a consolidation that would silently tighten one of them. They should not be merged, so the name stops claiming they could be. proposer_sandbox guards a mount root, where a symlink hop changes what an untrusted session is handed. comparator_reuse guards a data directory whose contents are already validated one file at a time - reads go through _regular_file, which lstats and rejects symlinks, and writes through O_NOFOLLOW. A symlinked parent therefore grants nothing those guards do not already cover, while refusing one would reject a symlinked artifacts directory or macOS's /var for no gain. Renamed to _resolved_directory, with the reasoning recorded at the definition, and a test that pins both halves: a symlinked parent is accepted and resolved, a symlinked leaf is still refused. Behavior is unchanged. 591 eval tests pass, ruff clean. The two test_model_gateway.py failures are pre-existing and fail on main. --- eval/tests/test_comparator_reuse.py | 27 +++++++++++++++++++++++++ eval/workflow_bench/comparator_reuse.py | 21 ++++++++++++++++--- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/eval/tests/test_comparator_reuse.py b/eval/tests/test_comparator_reuse.py index 57f327ba4..d56a09a1d 100644 --- a/eval/tests/test_comparator_reuse.py +++ b/eval/tests/test_comparator_reuse.py @@ -8,6 +8,7 @@ from pathlib import Path import pytest +from workflow_bench import comparator_reuse from workflow_bench.comparator_reuse import ( ComparatorReuseExpectation, TaskReuseBinding, @@ -227,3 +228,29 @@ def test_a_changed_sandbox_dependency_is_not_the_same_baseline(): ) is False # A row that predates the field is not evidence of agreement either. assert row_is_reusable_comparator(_row(sandbox_dependency_manifest_digest=None), _expected()) is False + + +def test_reuse_directories_allow_a_symlinked_parent_but_not_a_symlinked_leaf(tmp_path: Path): + """Pins a deliberate difference from the sandbox's mount-root check. + + proposer_sandbox refuses every symlink hop because a hop changes what an + untrusted session is handed. A reuse directory is data, and every file + inside it is validated on its own, so a symlinked parent is allowed - + rejecting it would break a symlinked artifacts directory or macOS's /var + for no gain. The leaf itself must still be a real directory. + """ + + real = tmp_path / "real" + real.mkdir() + (real / "inner").mkdir() + linked_parent = tmp_path / "linked" + linked_parent.symlink_to(real, target_is_directory=True) + + # Reached through a symlinked parent: allowed, and resolved to the real path. + assert comparator_reuse._resolved_directory( + linked_parent / "inner", label="probe" + ) == (real / "inner").resolve() + + # The leaf itself being a symlink is still refused. + with pytest.raises(SandboxError, match="must be a real directory"): + comparator_reuse._resolved_directory(linked_parent, label="probe") diff --git a/eval/workflow_bench/comparator_reuse.py b/eval/workflow_bench/comparator_reuse.py index 3878f8fef..71d1d15cd 100644 --- a/eval/workflow_bench/comparator_reuse.py +++ b/eval/workflow_bench/comparator_reuse.py @@ -246,8 +246,8 @@ def materialize_reused_row( ) -> dict[str, Any]: """Copy digest-bound artifacts into this sweep's evidence dir and stamp reuse.""" - source = _real_directory(source_dir, label="reuse source") - dest = _real_directory(dest_dir, label="reuse destination") + source = _resolved_directory(source_dir, label="reuse source") + dest = _resolved_directory(dest_dir, label="reuse destination") if source == dest: raise SandboxError("comparator reuse cannot read and write the same results directory") @@ -332,7 +332,22 @@ def _transcript_metadata(metadata: Any) -> tuple[str, str, int]: return relative, digest, size -def _real_directory(path: Path, *, label: str) -> Path: +def _resolved_directory(path: Path, *, label: str) -> Path: + """An existing, non-symlink directory, resolved through its parents. + + Deliberately weaker than proposer_sandbox's same-shaped helper, which + refuses every symlink hop in the path. That one guards a MOUNT ROOT, where + a hop changes what an untrusted session is handed. This one guards a DATA + directory whose contents are validated individually anyway - every file + read goes through ``_regular_file`` (lstat, symlinks rejected) and every + write through ``O_NOFOLLOW`` - so a symlinked parent grants nothing those + guards do not already cover, while refusing one would reject ordinary + setups such as a symlinked artifacts directory or macOS's /var. + + Separately named because they make different promises. Do not merge them + without first deciding which promise the reuse path should make. + """ + resolved = path.expanduser() try: metadata = resolved.lstat()