mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-27 00:17:53 +00:00
* 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.
130 lines
5.6 KiB
Python
130 lines
5.6 KiB
Python
"""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
|