From bcee5a90618142bfbac46569aa4582a133c1a960 Mon Sep 17 00:00:00 2001 From: Aari Date: Mon, 13 Jul 2026 18:34:41 +0800 Subject: [PATCH] =?UTF-8?q?refactor(sandbox):=20give=20the=20host=E2=86=92?= =?UTF-8?q?virtual=20output-mask=20regex=20a=20single=20owner=20(#4108)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(sandbox): give the host→virtual output-mask regex a single owner Two call sites rewrite host paths back to their virtual form in text that reaches the model — LocalSandbox._reverse_output_patterns (bash output) and sandbox.tools._compiled_mask_patterns (glob/grep/ls results) — and each built the same `escape(base) + boundary + tail` rule from its own copy. That duplication has already produced two bugs: #4035 added the segment boundary to the reverse patterns and missed the masking patterns, and #4053 had to add the same boundary to the other copy. Extract the rule into sandbox/path_patterns.py so a third copy cannot silently disagree. The extraction is not a pure move: the two sites disagree on the base. tools.py derives bases from _path_variants (which yields Windows spellings) and matches them against output whose separators it does not control, so it relaxes the separators inside the base; LocalSandbox resolves its bases from the running platform and must not be widened. That difference is now an explicit `separator_agnostic` parameter rather than an accident of two implementations. The boundary and tail constants are private: build_output_mask_pattern is the only supported spelling, so a third site cannot import the pieces and hand-roll a variant. Behavior is unchanged at both sites — pinned by tests that reproduce each pre-extraction expression byte-for-byte. * test(sandbox): pin the base the helper must not normalize Review notes on #4108. The committed snapshot compares the helper against hand-copied literals of the pre-extraction expressions, so its red-ness rests on those literals, not on the length of _BASES -- both sides compute the same expression, and 5k fuzzed bases produce zero byte-differences. Mutating the helper one clause at a time (12 mutations over the boundary, the tail and the escape/replace) shows the seven committed bases catch 11: the miss is a helper that normalizes its input by rstripping a trailing separator. Only a trailing-slash base or a Windows drive root catches that, and Path.resolve() / str(Path(...)) strip trailing slashes, so neither call site can produce the former. C:\ survives resolve() with its separator intact, so that is the one base worth adding. Also point local_sandbox's comment at path_patterns, the owner, instead of citing _content_pattern as the class reference, and drop the rationale it now duplicates from the owner's docstring -- a second copy of the explanation drifts the same way the second copy of the regex did. The site-specific half stays. Comments and test data only; no behavior change. --- .../deerflow/sandbox/local/local_sandbox.py | 32 ++--- .../harness/deerflow/sandbox/path_patterns.py | 68 +++++++++ .../harness/deerflow/sandbox/tools.py | 26 ++-- backend/tests/test_sandbox_path_patterns.py | 129 ++++++++++++++++++ 4 files changed, 224 insertions(+), 31 deletions(-) create mode 100644 backend/packages/harness/deerflow/sandbox/path_patterns.py create mode 100644 backend/tests/test_sandbox_path_patterns.py diff --git a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py index 8a394b4dc..aafb1c529 100644 --- a/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py +++ b/backend/packages/harness/deerflow/sandbox/local/local_sandbox.py @@ -15,6 +15,7 @@ from typing import NamedTuple from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.sandbox.env_policy import build_sandbox_env from deerflow.sandbox.local.list_dir import list_dir +from deerflow.sandbox.path_patterns import build_output_mask_pattern from deerflow.sandbox.sandbox import Sandbox, _validate_extra_env from deerflow.sandbox.search import GrepMatch, find_glob_matches, find_grep_matches @@ -220,23 +221,22 @@ class LocalSandbox(Sandbox): @cached_property def _reverse_output_patterns(self) -> list[re.Pattern[str]]: """Compiled matchers for local paths in command output (longest local path first).""" - # Same segment-boundary lookahead as the forward patterns above, so a mount - # root does not match inside a sibling that merely shares its prefix - # (``.../skills`` inside ``.../skills-extra``). Without it the regex yields - # the bare root, which then *equals* the mount root and so satisfies - # ``_reverse_resolve_path``'s own ``+ "/"`` guard — the sibling is rewritten - # to a container path that forward resolution refuses to map back. + # The rule — segment boundary plus path tail — is owned by + # ``deerflow.sandbox.path_patterns`` and shared with + # ``sandbox.tools._compiled_mask_patterns``, the other site that rewrites host + # paths back to virtual ones. Its rationale (why the boundary class is + # text-oriented rather than shell-oriented like ``_command_pattern``, why ``$`` + # is load-bearing) lives with the owner rather than in a second copy here, which + # is what let the two drift before (#4035 added the boundary here and missed + # that site; #4053 added it there). # - # The boundary class mirrors ``_content_pattern``'s, not ``_command_pattern``'s: - # this runs over arbitrary command output, where a root can legitimately be - # followed by ``,`` ``:`` or ``\`` — all of which the shell-oriented class - # would reject. The trailing group keeps ``[/\\]`` so Windows paths still match. - # - # ``$`` is load-bearing: output ending exactly at a mount root would - # otherwise fail the lookahead and be emitted as the raw host path. - boundary = r"(?=/|$|[^\w./-])" - tail = r"(?:[/\\][^\s\"';&|<>()]*)?" - return [re.compile(re.escape(self._resolved_local_paths[m]) + boundary + tail) for m in self._mappings_by_local_specificity] + # What is specific to this site: without the boundary the regex yields the bare + # root, which then *equals* the mount root and so satisfies + # ``_reverse_resolve_path``'s own ``+ "/"`` guard — the sibling is rewritten to a + # container path that forward resolution refuses to map back. And bases stay + # separator-*sensitive*: they come from ``Path.resolve()`` and already carry the + # platform's separator, so relaxing them would widen what this masks. + return [build_output_mask_pattern(self._resolved_local_paths[m]) for m in self._mappings_by_local_specificity] @cached_property def _resolved_local_paths(self) -> dict[PathMapping, str]: diff --git a/backend/packages/harness/deerflow/sandbox/path_patterns.py b/backend/packages/harness/deerflow/sandbox/path_patterns.py new file mode 100644 index 000000000..2fa12077d --- /dev/null +++ b/backend/packages/harness/deerflow/sandbox/path_patterns.py @@ -0,0 +1,68 @@ +"""Shared construction of the host→virtual output-masking regexes. + +The boundary and tail are deliberately private: ``build_output_mask_pattern`` is +the only supported way to spell this rule, so a third site cannot import the +pieces and hand-roll a variant that drifts from the other two. + +Two independent call sites rewrite host paths back to their virtual form in +text that flows to the model: ``LocalSandbox._reverse_output_patterns`` (bash +output) and ``sandbox.tools._compiled_mask_patterns`` (glob/grep/ls results). +They must agree on where a host base is allowed to end, because both feed the +same downstream contract — a match that stops short of a real segment boundary +is rewritten to a container path that forward resolution then refuses to map +back. + +Keeping one copy of that rule per file is what let it drift: #4035 added the +segment boundary to the reverse patterns and missed the masking patterns, and +#4053 had to add the same boundary to the other copy. This module holds the +rule once so a third copy cannot silently disagree. + +The two sites are *not* identical, and the difference is deliberate — see +``separator_agnostic``. +""" + +from __future__ import annotations + +import re + +# Only match where a host base ends at a real path-segment boundary, so a mount +# root does not match inside a sibling that merely shares its prefix +# (``.../skills`` inside ``.../skills-extra``). +# +# The class is text-oriented, not shell-oriented (contrast +# ``LocalSandbox._command_pattern``): both callers run over arbitrary command +# output or file listings, where a root can legitimately be followed by ``,`` +# ``:`` or ``\``, all of which a shell-oriented class would reject. +# +# ``$`` is load-bearing: output ending exactly at a mount root would otherwise +# fail the lookahead and be emitted as the raw host path. +_SEGMENT_BOUNDARY = r"(?=/|$|[^\w./-])" + +# The path tail following the base. ``[/\\]`` keeps Windows-separated paths +# matching; the negated class stops at whitespace and shell punctuation so a +# path embedded in a larger line is not over-consumed. +_PATH_TAIL = r"(?:[/\\][^\s\"';&|<>()]*)?" + + +def build_output_mask_pattern(base: str, *, separator_agnostic: bool = False) -> re.Pattern[str]: + """Compile the matcher for one host ``base`` in model-visible output. + + Args: + base: Host path root to match (already resolved by the caller). + separator_agnostic: Accept either separator *inside* the base, so a + base captured with ``\\`` still matches output that spells the same + path with ``/``. ``sandbox.tools`` needs this because it derives its + bases from ``_path_variants`` (which yields Windows-style spellings) + and matches them against output whose separators it does not + control. ``LocalSandbox`` does not: its bases come from + ``Path.resolve()``, so they already carry the running platform's + separator, and relaxing them would widen what it masks. + + Returns: + A compiled pattern matching ``base`` at a segment boundary, plus an + optional path tail. + """ + escaped = re.escape(base) + if separator_agnostic: + escaped = escaped.replace(r"\\", r"[/\\]") + return re.compile(escaped + _SEGMENT_BOUNDARY + _PATH_TAIL) diff --git a/backend/packages/harness/deerflow/sandbox/tools.py b/backend/packages/harness/deerflow/sandbox/tools.py index 9d44d6a43..780d57931 100644 --- a/backend/packages/harness/deerflow/sandbox/tools.py +++ b/backend/packages/harness/deerflow/sandbox/tools.py @@ -23,6 +23,7 @@ from deerflow.sandbox.exceptions import ( SandboxRuntimeError, ) from deerflow.sandbox.file_operation_lock import get_file_operation_lock +from deerflow.sandbox.path_patterns import build_output_mask_pattern from deerflow.sandbox.sandbox import Sandbox from deerflow.sandbox.sandbox_provider import get_sandbox_provider from deerflow.sandbox.search import GrepMatch @@ -727,20 +728,16 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple glob/grep match, so without this the same patterns are recompiled per match. """ - # Same segment-boundary lookahead as ``LocalSandbox._reverse_output_patterns`` - # (#4035), so a host base does not match inside a sibling that merely shares - # its prefix (``.../skills`` inside ``.../skills-extra``). Without it the - # regex yields the bare base, which then *equals* ``base`` in - # ``replace_match`` and so the sibling is rewritten to a container path that - # forward resolution refuses to map back. + # The segment boundary and path tail are shared with + # ``LocalSandbox._reverse_output_patterns`` — see + # ``deerflow.sandbox.path_patterns``, which owns that rule so the two copies + # cannot drift again (#4035 fixed one and missed the other; #4053 fixed the + # other). # - # The class mirrors ``_content_pattern``'s: this runs over arbitrary command - # output, where a base can legitimately be followed by ``,`` ``:`` or ``\``. - # ``$`` is load-bearing — output ending exactly at a base would otherwise - # fail the lookahead and be emitted as the raw host path. - boundary = r"(?=/|$|[^\w./-])" - tail = r"(?:[/\\][^\s\"';&|<>()]*)?" - + # ``separator_agnostic=True`` is the one thing this site does differently: + # its bases come from ``_path_variants``, which yields Windows-style + # spellings, and they are matched against output whose separators this layer + # does not control. compiled: list[tuple[re.Pattern[str], str, str]] = [] for host_base, virtual_base in sources: seen: set[str] = set() @@ -753,8 +750,7 @@ def _compiled_mask_patterns(sources: tuple[tuple[str, str], ...]) -> tuple[tuple if variant in seen: continue seen.add(variant) - escaped = re.escape(variant).replace(r"\\", r"[/\\]") - compiled.append((re.compile(escaped + boundary + tail), variant, virtual_base)) + compiled.append((build_output_mask_pattern(variant, separator_agnostic=True), variant, virtual_base)) return tuple(compiled) diff --git a/backend/tests/test_sandbox_path_patterns.py b/backend/tests/test_sandbox_path_patterns.py new file mode 100644 index 000000000..46385bfec --- /dev/null +++ b/backend/tests/test_sandbox_path_patterns.py @@ -0,0 +1,129 @@ +"""Tests for the shared host→virtual output-mask pattern (``sandbox/path_patterns.py``). + +The rule these pin is not "the regex is correct" — that is #4035/#4053 — but +"there is exactly one copy of it, and extracting it did not change either call +site's matching". The two sites differ on one axis only (separator handling), +and that asymmetry is load-bearing: erasing it would widen ``LocalSandbox``'s +masking or narrow ``sandbox.tools``'s. + +The move itself was cleared by a differential against the *real* pre-extraction +expressions, run once on the parent commit. That run cannot be committed: after +this lands there is no old inline expression left to diff against, only the +frozen copies below. So the committed guard is the weaker snapshot, and its +red-ness rests on those literals — not on the length of ``_BASES``. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping +from deerflow.sandbox.path_patterns import build_output_mask_pattern +from deerflow.sandbox.tools import _compiled_mask_patterns + + +def _legacy_tools_pattern(base: str) -> re.Pattern[str]: + """The expression ``_compiled_mask_patterns`` inlined before the extraction.""" + escaped = re.escape(base).replace(r"\\", r"[/\\]") + return re.compile(escaped + r"(?=/|$|[^\w./-])" + r"(?:[/\\][^\s\"';&|<>()]*)?") + + +def _legacy_local_pattern(base: str) -> re.Pattern[str]: + """The expression ``_reverse_output_patterns`` inlined before the extraction.""" + return re.compile(re.escape(base) + r"(?=/|$|[^\w./-])" + r"(?:[/\\][^\s\"';&|<>()]*)?") + + +_BASES = [ + "/host/skills", + "/host/dir with spaces", + "/host/re+meta(chars)[x]", + "/host/dots.in.name", + "/Users/a/.deer-flow/users/u1/threads/t1/user-data", + "C:\\host\\skills", + "/host/技能", + # Drive root: the only base either caller can hand the helper that still ends in a + # separator (``Path.resolve()`` strips them everywhere else), so it is the one shape + # that goes red if the helper starts normalizing the base it is given. + "C:\\", +] + + +@pytest.mark.parametrize("base", _BASES) +def test_helper_reproduces_the_pre_extraction_expressions(base: str) -> None: + """Byte-identical to what each call site built inline, for both separator modes. + + This is the anchor for the move itself: edit the helper in a way that changes + either site's regex and this goes red. + """ + assert build_output_mask_pattern(base, separator_agnostic=True).pattern == _legacy_tools_pattern(base).pattern + assert build_output_mask_pattern(base).pattern == _legacy_local_pattern(base).pattern + + +def test_separator_agnostic_is_the_only_difference_between_the_two_modes() -> None: + """The asymmetry the helper must preserve rather than unify. + + ``sandbox.tools`` derives bases from ``_path_variants`` (Windows spellings) + and matches them against output whose separators it does not control, so a + ``\\``-spelled base must still match ``/``-spelled output. ``LocalSandbox`` + resolves its bases from the running platform and must not be widened. + """ + windows_base = "C:\\host\\skills" + posix_spelling = "C:/host/skills/file.md" + + assert build_output_mask_pattern(windows_base, separator_agnostic=True).search(posix_spelling) + assert build_output_mask_pattern(windows_base).search(posix_spelling) is None + + # On a base with no separator ambiguity the two modes agree exactly. + posix_base = "/host/skills" + assert build_output_mask_pattern(posix_base, separator_agnostic=True).pattern == build_output_mask_pattern(posix_base).pattern + + +def test_boundary_still_rejects_prefix_siblings_and_accepts_real_segments() -> None: + """The #4035/#4053 rule itself, now asserted once against the shared helper.""" + pattern = build_output_mask_pattern("/host/skills") + + # Matches: the root itself, a child, a Windows-separated child, and a root + # followed by text punctuation (``$`` and the ``[^\w./-]`` class). + assert pattern.fullmatch("/host/skills") + assert pattern.match("/host/skills/a/b.md") + assert pattern.match("/host/skills\\a\\b.md") + assert pattern.search("paths: /host/skills, and more") + + # Does not match inside a sibling that merely shares the prefix. + assert pattern.search("/host/skills-extra/file.md") is None + assert pattern.search("/host/skills.bak") is None + assert pattern.search("/host/skills2/file.md") is None + + +def test_local_sandbox_reverse_patterns_route_through_the_helper(tmp_path: Path) -> None: + """Call-site wiring: a re-inlined copy that *diverges* from the shared rule goes red. + + It does not (and cannot) catch a byte-identical re-inline — that is not yet a + defect. What it catches is the shape of the actual regression: #4035 changed + one copy of the rule and left the other behind. + """ + local = tmp_path / "skills" + local.mkdir() + sandbox = LocalSandbox( + id="local", + path_mappings=[PathMapping(container_path="/mnt/skills", local_path=str(local), read_only=True)], + ) + + resolved = str(Path(local).resolve()) + assert [p.pattern for p in sandbox._reverse_output_patterns] == [build_output_mask_pattern(resolved).pattern] + + +def test_tools_mask_patterns_route_through_the_helper(tmp_path: Path) -> None: + """Same wiring check for the other copy — and it must stay separator-agnostic.""" + host = tmp_path / "skills" + host.mkdir() + + compiled = _compiled_mask_patterns(((str(host), "/mnt/skills"),)) + + assert compiled + for pattern, variant, virtual_base in compiled: + assert virtual_base == "/mnt/skills" + assert pattern.pattern == build_output_mask_pattern(variant, separator_agnostic=True).pattern