mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-06 02:49:56 +00:00
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.
This commit is contained in:
parent
66d69194bd
commit
b0030daf3a
2 changed files with 45 additions and 3 deletions
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue