refactor(sandbox): give the host→virtual output-mask regex a single owner (#4108)

* 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.

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]:

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)

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)
 
 

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