mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(sandbox): keep document bytes when write_file translates paths (#7462)
* fix(sandbox): keep document bytes when write_file translates paths LocalSandbox.write_file rewrote container paths inside every file it stored, so a dictated README.md ended up holding a host path while read_file masked the rewrite for agent-written files: the read-back showed the virtual path, and the agent confirmed content the file did not contain (#7461). Translation now applies only to executable content, classified with the shared is_code_file rule from deerflow.skills.package_files that the skill installer, the export guard, and skill_manage_tool already use. Code suffixes and extensionless files that open with a shebang keep the host-path rewrite they need to run under a host interpreter (PR #1935); documents, data files, and configs are stored verbatim and read back as stored. * fix(sandbox): translate paths for interpreter scripts and stop reading pipes The classifier added for #7461 asked only the skill-package rule, which does not list `.awk` (nor `.sed`, `.lua`, `.r`, `.tcl`), so a written AWK program kept container paths that the host awk cannot open when it runs `awk -f`. Those suffixes are now the sandbox's own set: the shared rule also decides what SkillScan and the installer treat as package code, and widening it there is a different call. Classification no longer touches the destination unless the path is extensionless. A suffix answers the question by itself, so an append to `events.py` no longer opens the file first, and the extensionless shebang sniff goes through `os.open(O_NONBLOCK)` plus an `fstat` regular file check, which is what keeps a FIFO append from blocking. * test(sandbox): pin the non-blocking sniff with a suffixless pipe append
This commit is contained in:
parent
6213fee192
commit
228913b6b7
@ -14,7 +14,7 @@ import threading
|
||||
from contextlib import suppress
|
||||
from dataclasses import dataclass
|
||||
from functools import cached_property
|
||||
from pathlib import Path
|
||||
from pathlib import Path, PurePosixPath
|
||||
from typing import NamedTuple
|
||||
|
||||
from deerflow.config.paths import VIRTUAL_PATH_PREFIX
|
||||
@ -38,6 +38,69 @@ _COMMAND_CAPTURE_LIMIT_BYTES = 10 * 1024 * 1024
|
||||
_PIPE_DRAIN_JOIN_TIMEOUT_SECONDS = 0.2
|
||||
|
||||
|
||||
#: Script suffixes a host interpreter opens by path but the shared skill-package
|
||||
#: classifier does not list. ``awk -f``, ``sed -f``, ``lua``, ``Rscript``, and
|
||||
#: ``tclsh`` all read the file named on the command line, so container paths
|
||||
#: inside it need the same translation as ``.py``/``.sh``. The shared set is
|
||||
#: left alone: it also decides what SkillScan and the installer treat as
|
||||
#: package code, which is a separate contract from what the host shell runs.
|
||||
_INTERPRETER_SCRIPT_SUFFIXES = frozenset({".awk", ".lua", ".r", ".sed", ".tcl"})
|
||||
|
||||
|
||||
def _existing_regular_file_head(path: str) -> bytes:
|
||||
"""Return the first two bytes of an existing regular file, else ``b""``.
|
||||
|
||||
``write_file(append=True)`` asks this for the interpreter line of an
|
||||
extensionless destination. Only a regular file may be opened: opening a
|
||||
FIFO for reading blocks until the other end connects, which would turn an
|
||||
append that used to complete into a hang. ``O_NONBLOCK`` keeps the open
|
||||
itself from waiting, and the ``fstat`` check rejects the handle before any
|
||||
read.
|
||||
"""
|
||||
try:
|
||||
fd = os.open(path, os.O_RDONLY | getattr(os, "O_NONBLOCK", 0))
|
||||
except OSError:
|
||||
return b""
|
||||
try:
|
||||
if not stat.S_ISREG(os.fstat(fd).st_mode):
|
||||
return b""
|
||||
return os.read(fd, 2)
|
||||
except OSError:
|
||||
return b""
|
||||
finally:
|
||||
with suppress(OSError):
|
||||
os.close(fd)
|
||||
|
||||
|
||||
def _content_path_translation_applies(path: str, content: str, *, append: bool, resolved_path: str) -> bool:
|
||||
"""Whether container paths inside ``content`` are translated to host paths.
|
||||
|
||||
The translation exists so a script the agent wrote can run under a host
|
||||
interpreter (PR #1935). Documents, data files, and configs are not
|
||||
executed, so their bytes are stored verbatim: rewriting them put host
|
||||
paths into deliverables while ``read_file`` masked the rewrite back to
|
||||
virtual paths (#7461).
|
||||
|
||||
A suffix settles the classification by itself, so only an extensionless
|
||||
destination is inspected for a shebang. That keeps named files free of
|
||||
I/O and leaves an append to a pipe or device untouched.
|
||||
"""
|
||||
from deerflow.skills.package_files import is_code_file, is_code_path
|
||||
|
||||
posix = PurePosixPath(path.replace("\\", "/"))
|
||||
if is_code_path(path) or posix.suffix.lower() in _INTERPRETER_SCRIPT_SUFFIXES:
|
||||
return True
|
||||
if posix.suffix:
|
||||
# A non-script suffix settles it: a shebang cannot make ``.md`` a program.
|
||||
return False
|
||||
head = content[:2].encode("utf-8", "surrogatepass")
|
||||
if append and not head.startswith(b"#!"):
|
||||
# An extensionless script's interpreter line sits at the top of the
|
||||
# existing file, not in the text being appended.
|
||||
head = _existing_regular_file_head(resolved_path) or head
|
||||
return is_code_file(path, head)
|
||||
|
||||
|
||||
def _publish_file_revision(path: str, content: str) -> None:
|
||||
target = Path(os.path.realpath(path))
|
||||
# Rename checks the parent, not the target's write permission. Check the
|
||||
@ -1237,9 +1300,11 @@ class LocalSandbox(Sandbox):
|
||||
dir_path = os.path.dirname(resolved_path)
|
||||
if dir_path:
|
||||
os.makedirs(dir_path, exist_ok=True)
|
||||
# Resolve container paths in content to local paths
|
||||
# using the content-specific resolver (forward-slash safe)
|
||||
resolved_content = self._resolve_paths_in_content(content)
|
||||
# Resolve container paths in content to local paths, but only for
|
||||
# executable content: a script needs host-resolvable paths when
|
||||
# bash runs it, while a document's bytes must survive the write.
|
||||
translate_paths = _content_path_translation_applies(path, content, append=append, resolved_path=resolved_path)
|
||||
resolved_content = self._resolve_paths_in_content(content) if translate_paths else content
|
||||
if append or sys.platform not in {"darwin", "linux"}:
|
||||
# Other hosts retain their existing write/ACL semantics.
|
||||
with open(resolved_path, "a" if append else "w", encoding="utf-8", newline="") as f:
|
||||
|
||||
@ -90,15 +90,17 @@ def test_failed_save_keeps_previous_file_and_cleans_staging(tmp_path, monkeypatc
|
||||
|
||||
@pytest.mark.parametrize("existing", [False, True])
|
||||
@pytest.mark.parametrize("content", ["", "first\r\nsecond\r\n", "path=/mnt/user-data/outputs/data.csv\n"])
|
||||
def test_successful_save_preserves_bytes_and_path_translation(tmp_path, sandbox, existing, content):
|
||||
def test_successful_save_preserves_bytes_verbatim(tmp_path, sandbox, existing, content):
|
||||
target = tmp_path / "report.md"
|
||||
if existing:
|
||||
target.write_bytes(b"previous report\n")
|
||||
|
||||
sandbox.write_file("/mnt/user-data/outputs/report.md", content)
|
||||
|
||||
expected = content.replace("/mnt/user-data/outputs", tmp_path.as_posix())
|
||||
assert target.read_bytes() == expected.encode("utf-8")
|
||||
# Markdown is a document, so write_file stores it verbatim: container-path
|
||||
# translation is limited to executable content (see
|
||||
# test_local_sandbox_content_path_translation.py).
|
||||
assert target.read_bytes() == content.encode("utf-8")
|
||||
assert sandbox.read_file("/mnt/user-data/outputs/report.md") == content
|
||||
assert list(tmp_path.iterdir()) == [target]
|
||||
|
||||
|
||||
206
backend/tests/test_local_sandbox_content_path_translation.py
Normal file
206
backend/tests/test_local_sandbox_content_path_translation.py
Normal file
@ -0,0 +1,206 @@
|
||||
"""Issue #7461 — only executable content gets container paths translated.
|
||||
|
||||
``write_file`` rewrites container paths inside the bytes it stores so a script
|
||||
the agent wrote can run under a host interpreter (PR #1935). Applying that to
|
||||
documents put host paths into deliverables, and ``read_file`` masked the
|
||||
rewrite for agent-written files, so the agent read back the virtual path and
|
||||
reported content it had not stored. Documents, data, and configs are now
|
||||
stored verbatim; code files keep the translation.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import builtins
|
||||
import os
|
||||
import shutil
|
||||
import threading
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from deerflow.sandbox.local.local_sandbox import LocalSandbox, PathMapping
|
||||
|
||||
|
||||
def _sandbox(tmp_path: Path) -> LocalSandbox:
|
||||
outputs = tmp_path / "outputs"
|
||||
workspace = tmp_path / "workspace"
|
||||
outputs.mkdir()
|
||||
workspace.mkdir()
|
||||
return LocalSandbox(
|
||||
id="test",
|
||||
path_mappings=[
|
||||
PathMapping(container_path="/mnt/user-data/outputs", local_path=str(outputs)),
|
||||
PathMapping(container_path="/mnt/user-data/workspace", local_path=str(workspace)),
|
||||
],
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"name,content",
|
||||
[
|
||||
("README.md", "# Run notes\nThe results table is saved at /mnt/user-data/outputs/results.csv\n"),
|
||||
("notes.txt", "see /mnt/user-data/outputs/results.csv and /mnt/user-data/workspace/plot.png\n"),
|
||||
("data.csv", "label,path\nrows,/mnt/user-data/outputs/results.csv\n"),
|
||||
("settings.json", '{"output": "/mnt/user-data/outputs/results.csv"}\n'),
|
||||
("export.yaml", "output: /mnt/user-data/outputs/results.csv\n"),
|
||||
],
|
||||
ids=["markdown", "text", "csv", "json", "yaml"],
|
||||
)
|
||||
def test_non_executable_content_is_stored_verbatim(tmp_path, name, content):
|
||||
"""A dictation of exact bytes must survive the write and the read-back."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
virtual = f"/mnt/user-data/outputs/{name}"
|
||||
|
||||
sandbox.write_file(virtual, content)
|
||||
|
||||
stored = (tmp_path / "outputs" / name).read_bytes().decode("utf-8")
|
||||
assert stored == content
|
||||
# read_file returns the stored bytes, so the agent cannot claim a rewrite it did not see.
|
||||
assert sandbox.read_file(virtual) == content
|
||||
|
||||
|
||||
def test_extensionless_document_without_shebang_is_stored_verbatim(tmp_path):
|
||||
"""``README`` with no suffix is a document, not a program (the shared rule's negative case)."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
content = "Install notes: /mnt/user-data/workspace/setup.sh\n"
|
||||
|
||||
sandbox.write_file("/mnt/user-data/outputs/README", content)
|
||||
|
||||
assert (tmp_path / "outputs" / "README").read_text(encoding="utf-8") == content
|
||||
|
||||
|
||||
def test_append_keeps_document_paths_virtual(tmp_path):
|
||||
sandbox = _sandbox(tmp_path)
|
||||
sandbox.write_file("/mnt/user-data/outputs/README.md", "# Run notes\n")
|
||||
sandbox.write_file("/mnt/user-data/outputs/README.md", "saved at /mnt/user-data/outputs/results.csv\n", append=True)
|
||||
|
||||
assert (tmp_path / "outputs" / "README.md").read_text(encoding="utf-8") == ("# Run notes\nsaved at /mnt/user-data/outputs/results.csv\n")
|
||||
|
||||
|
||||
def test_append_to_shebang_script_still_translates_paths(tmp_path):
|
||||
"""An extensionless script's interpreter line stays at the top of the file."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
virtual = "/mnt/user-data/workspace/collect"
|
||||
sandbox.write_file(virtual, "#!/bin/bash\nset -eu\n")
|
||||
sandbox.write_file(virtual, "cat /mnt/user-data/outputs/results.csv\n", append=True)
|
||||
|
||||
stored = (tmp_path / "workspace" / "collect").read_text(encoding="utf-8")
|
||||
assert stored.startswith("#!/bin/bash\n")
|
||||
assert "/mnt/user-data" not in stored
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"name,content",
|
||||
[
|
||||
("plot.py", 'OUTPUT = "/mnt/user-data/outputs/results.csv"\n'),
|
||||
("run.sh", 'export PATH="/mnt/user-data/workspace/bin:/mnt/user-data/outputs/bin"\n'),
|
||||
("run", "#!/bin/bash\ncat /mnt/user-data/outputs/results.csv\n"),
|
||||
],
|
||||
ids=["python", "shell", "shebang-without-suffix"],
|
||||
)
|
||||
def test_executable_content_still_translates_paths(tmp_path, name, content):
|
||||
"""PR #1935: a script must see host paths, or ``bash``/``python`` cannot open them."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
virtual = f"/mnt/user-data/workspace/{name}"
|
||||
|
||||
sandbox.write_file(virtual, content)
|
||||
|
||||
stored = (tmp_path / "workspace" / name).read_text(encoding="utf-8")
|
||||
assert "/mnt/user-data" not in stored
|
||||
assert Path(os.path.realpath(tmp_path)).as_posix() in stored
|
||||
# The reverse translation keeps the agent's view consistent with what it wrote.
|
||||
assert sandbox.read_file(virtual) == content
|
||||
|
||||
|
||||
@pytest.mark.parametrize("name", ["report.awk", "cleanup.sed", "plugin.lua", "plot.r", "run.tcl", "REPORT.AWK"])
|
||||
def test_interpreter_script_suffixes_translate_container_paths(tmp_path, name):
|
||||
"""``awk -f`` and friends open the script itself, so its container paths need host paths."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
content = 'BEGIN { while ((getline line < "/mnt/user-data/outputs/data.txt") > 0) print line }\n'
|
||||
virtual = f"/mnt/user-data/workspace/{name}"
|
||||
|
||||
sandbox.write_file(virtual, content)
|
||||
|
||||
stored = (tmp_path / "workspace" / name).read_text(encoding="utf-8")
|
||||
assert "/mnt/user-data" not in stored
|
||||
assert Path(os.path.realpath(tmp_path)).as_posix() in stored
|
||||
assert sandbox.read_file(virtual) == content
|
||||
|
||||
|
||||
@pytest.mark.skipif(shutil.which("awk") is None, reason="awk is not installed")
|
||||
def test_awk_script_written_through_the_sandbox_still_runs(tmp_path):
|
||||
"""An AWK program that opens a container path must run through ``execute_command``."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
(tmp_path / "outputs" / "data.txt").write_text("alpha\nbeta\n", encoding="utf-8")
|
||||
sandbox.write_file(
|
||||
"/mnt/user-data/workspace/report.awk",
|
||||
'BEGIN { while ((getline line < "/mnt/user-data/outputs/data.txt") > 0) print line }\n',
|
||||
)
|
||||
|
||||
output = sandbox.execute_command("awk -f /mnt/user-data/workspace/report.awk /dev/null", timeout=20)
|
||||
|
||||
assert "alpha" in output
|
||||
assert "beta" in output
|
||||
|
||||
|
||||
def test_append_does_not_read_the_destination_before_classifying(tmp_path, monkeypatch):
|
||||
"""A ``.py`` suffix already settles it, so the append must not open the file for reading."""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
virtual = "/mnt/user-data/outputs/events.py"
|
||||
sandbox.write_file(virtual, "print('ready')\n")
|
||||
destination = os.path.realpath(tmp_path / "outputs" / "events.py")
|
||||
real_open = builtins.open
|
||||
|
||||
def guard_reads(file, mode="r", *args, **kwargs):
|
||||
reads_only = not set(mode) & set("wa+")
|
||||
if isinstance(file, str) and reads_only and os.path.realpath(file) == destination:
|
||||
raise AssertionError("append opened the destination for reading")
|
||||
return real_open(file, mode, *args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(builtins, "open", guard_reads)
|
||||
|
||||
sandbox.write_file(virtual, "print('appended')\n", append=True)
|
||||
|
||||
assert (tmp_path / "outputs" / "events.py").read_text(encoding="utf-8") == "print('ready')\nprint('appended')\n"
|
||||
|
||||
|
||||
@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="named pipes are a POSIX primitive")
|
||||
@pytest.mark.parametrize("name", ["events.py", "events"], ids=["suffix-settles-the-answer", "extensionless-shebang-sniff"])
|
||||
def test_append_to_a_named_pipe_does_not_block_while_classifying(tmp_path, name):
|
||||
"""Classification must not wait on the destination, whichever shape settles it.
|
||||
|
||||
A ``.py`` suffix settles the answer with no I/O; an extensionless name
|
||||
reaches the shebang sniff, which has to open the destination without
|
||||
waiting and reject the non-regular handle before reading. Reading a FIFO
|
||||
waits for the other end and for data; the append used to complete as soon
|
||||
as a reader was connected.
|
||||
"""
|
||||
sandbox = _sandbox(tmp_path)
|
||||
fifo = tmp_path / "outputs" / name
|
||||
os.mkfifo(fifo)
|
||||
virtual = f"/mnt/user-data/outputs/{name}"
|
||||
received: list[str] = []
|
||||
|
||||
def read_pipe() -> None:
|
||||
with open(fifo, encoding="utf-8") as reader:
|
||||
received.append(reader.read())
|
||||
|
||||
reader_thread = threading.Thread(target=read_pipe, daemon=True)
|
||||
reader_thread.start()
|
||||
|
||||
errors: list[BaseException] = []
|
||||
|
||||
def append_pipe() -> None:
|
||||
try:
|
||||
sandbox.write_file(virtual, "ready\n", append=True)
|
||||
except BaseException as exc: # noqa: BLE001 - surfaced by the assertion below
|
||||
errors.append(exc)
|
||||
|
||||
writer_thread = threading.Thread(target=append_pipe, daemon=True)
|
||||
writer_thread.start()
|
||||
writer_thread.join(5)
|
||||
|
||||
assert not writer_thread.is_alive(), "append blocked while classifying the destination"
|
||||
assert errors == []
|
||||
reader_thread.join(5)
|
||||
assert received == ["ready\n"]
|
||||
Loading…
x
Reference in New Issue
Block a user