diff --git a/README.md b/README.md index 6e77f757c0..3eec9f46cc 100644 --- a/README.md +++ b/README.md @@ -2439,6 +2439,11 @@ This is the difference between a chatbot with tool access and an agent with an a ### Agentic Browser Control +Browser screenshots and Browserless capture/fetch outputs are published only +after the complete file is written. New files use the host's normal creation +permissions (typically `0644`), and replacements preserve existing permissions, +so sandbox code can read captures when those permissions allow it. + Browser dependency auto-detection accepts `name`, `group`, and `use` in any order within a tool entry, with either indented or indentless YAML lists. diff --git a/backend/packages/harness/deerflow/community/browser_automation/tools.py b/backend/packages/harness/deerflow/community/browser_automation/tools.py index 32693b2b44..034d257f9d 100644 --- a/backend/packages/harness/deerflow/community/browser_automation/tools.py +++ b/backend/packages/harness/deerflow/community/browser_automation/tools.py @@ -34,6 +34,7 @@ from deerflow.config import get_app_config from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.constants import BROWSER_FRAMES_DIRNAME from deerflow.tools.types import Runtime +from deerflow.utils.atomic_write import publish_bytes_atomically as _publish_bytes_atomically from .session import BrowserSession, BrowserSessionManager, PageSnapshot, ScreenshotType, get_browser_session_manager @@ -435,7 +436,7 @@ def _thread_outputs_path(runtime: Runtime) -> Path | str: def _write_screenshot(outputs_path: Path, name: str, content: bytes) -> str: outputs_path.mkdir(parents=True, exist_ok=True) - (outputs_path / name).write_bytes(content) + _publish_bytes_atomically(outputs_path / name, content) return name diff --git a/backend/packages/harness/deerflow/community/browserless/tools.py b/backend/packages/harness/deerflow/community/browserless/tools.py index 9ec9ecf82a..53dd293aa4 100644 --- a/backend/packages/harness/deerflow/community/browserless/tools.py +++ b/backend/packages/harness/deerflow/community/browserless/tools.py @@ -16,6 +16,7 @@ from deerflow.community.url_safety import validate_delegated_backend_url, valida from deerflow.config import get_app_config from deerflow.config.paths import VIRTUAL_PATH_PREFIX from deerflow.tools.types import Runtime +from deerflow.utils.atomic_write import publish_bytes_atomically as _publish_bytes_atomically from deerflow.utils.readability import ReadabilityExtractor from .browserless_client import BrowserlessClient, BrowserlessFetchResult, BrowserlessScreenshotResult @@ -247,7 +248,7 @@ def _write_capture_output(outputs_path: Path, output_name: str, content: bytes) """Write ``content`` into ``outputs_path`` and return the actual filename used.""" outputs_path.mkdir(parents=True, exist_ok=True) final_name = _dedupe_output_name(outputs_path, output_name) - (outputs_path / final_name).write_bytes(content) + _publish_bytes_atomically(outputs_path / final_name, content) return final_name diff --git a/backend/packages/harness/deerflow/utils/AGENTS.md b/backend/packages/harness/deerflow/utils/AGENTS.md index 1a96df8c34..8c52ed3118 100644 --- a/backend/packages/harness/deerflow/utils/AGENTS.md +++ b/backend/packages/harness/deerflow/utils/AGENTS.md @@ -1,3 +1,12 @@ +### Atomic Byte Publication + +`atomic_write.publish_bytes_atomically` owns browser screenshot and Browserless +capture/fetch publication. It writes and fsyncs a hidden exclusive sibling before +`os.replace`, cleaning it on failure. New files use `0666` filtered by the host +umask; replacements preserve existing permission bits before publication. Never +read the process umask by changing it in this threaded runtime. Behavioral tests +for both tool imports live in `tests/test_browser_outputs_atomic.py`. + ### Model Retry Hints `retry_after.py::bounded_retry_after_ms` shares the 24-hour provider-hint ceiling diff --git a/backend/packages/harness/deerflow/utils/atomic_write.py b/backend/packages/harness/deerflow/utils/atomic_write.py new file mode 100644 index 0000000000..737d29833d --- /dev/null +++ b/backend/packages/harness/deerflow/utils/atomic_write.py @@ -0,0 +1,46 @@ +"""Atomic byte publication with normal file-creation permissions.""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path +from uuid import uuid4 + + +def publish_bytes_atomically(path: Path, content: bytes) -> None: + """Replace *path* with complete bytes, preserving its existing mode. + + New files use ``0666 & ~umask``, like ``Path.write_bytes``, so captures + remain readable through sandbox mounts under the usual ``0022`` umask. + A hidden, exclusively created sibling keeps partial writes away from the + final path. Failure before replacement leaves the previous file intact. + """ + try: + mode = stat.S_IMODE(path.stat().st_mode) + except FileNotFoundError: + mode = None + + temporary = path.with_name(f".{path.name}.{uuid4().hex}.tmp") + flags = os.O_CREAT | os.O_EXCL | os.O_WRONLY | getattr(os, "O_BINARY", 0) + # Let the kernel apply umask; reading it via os.umask would mutate + # process-global state while other threads may be creating files. + fd = os.open(temporary, flags, 0o666) + try: + handle = os.fdopen(fd, "wb") + fd = None # The file object now owns the descriptor. + with handle: + handle.write(content) + handle.flush() + if mode is not None: + if os.name == "nt": + os.chmod(temporary, mode) + else: + os.fchmod(handle.fileno(), mode) + os.fsync(handle.fileno()) + os.replace(temporary, path) + except BaseException: + if fd is not None: + os.close(fd) + temporary.unlink(missing_ok=True) + raise diff --git a/backend/tests/test_browser_outputs_atomic.py b/backend/tests/test_browser_outputs_atomic.py new file mode 100644 index 0000000000..ad6b5e6266 --- /dev/null +++ b/backend/tests/test_browser_outputs_atomic.py @@ -0,0 +1,158 @@ +# SPDX-License-Identifier: MIT +"""Browser captures must not be published half written under the final name.""" + +from __future__ import annotations + +import inspect +import os +import stat +from pathlib import Path + +import pytest + +from deerflow.community.browser_automation import tools as browser_automation_tools +from deerflow.community.browserless import tools as browserless_tools +from deerflow.utils.atomic_write import publish_bytes_atomically + + +@pytest.fixture(params=[browser_automation_tools, browserless_tools], ids=["browser", "browserless"]) +def publisher(request): + return request.param._publish_bytes_atomically + + +def test_publish_replaces_only_after_content_is_complete(tmp_path, monkeypatch, publisher): + target = tmp_path / "shot.png" + target.write_bytes(b"old") + replace = os.replace + publications = [] + + def observe_replace(source, destination): + assert target.read_bytes() == b"old" + assert destination == target + temporary = Path(source) + assert temporary.parent == target.parent + assert temporary.name.startswith(".") + assert temporary.read_bytes() == b"new complete capture" + if os.name != "nt": + assert stat.S_IMODE(temporary.stat().st_mode) == stat.S_IMODE(target.stat().st_mode) + publications.append(destination) + replace(source, destination) + + monkeypatch.setattr(os, "replace", observe_replace) + publisher(target, b"new complete capture") + + assert publications == [target] + assert target.read_bytes() == b"new complete capture" + assert list(tmp_path.iterdir()) == [target] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits and umask") +@pytest.mark.parametrize("mode", [0o600, 0o640, 0o644]) +def test_publish_preserves_existing_permissions(tmp_path, publisher, mode): + target = tmp_path / "shot.png" + target.write_bytes(b"old") + target.chmod(mode) + + publisher(target, b"new") + + assert target.read_bytes() == b"new" + assert stat.S_IMODE(target.stat().st_mode) == mode + assert list(tmp_path.iterdir()) == [target] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits and umask") +@pytest.mark.parametrize("mask", [0o022, 0o077]) +def test_publish_new_file_uses_normal_creation_permissions(tmp_path, publisher, mask): + target = tmp_path / "shot.png" + previous_mask = os.umask(mask) + try: + publisher(target, b"new") + finally: + os.umask(previous_mask) + + assert target.read_bytes() == b"new" + assert stat.S_IMODE(target.stat().st_mode) == 0o666 & ~mask + assert list(tmp_path.iterdir()) == [target] + + +@pytest.mark.parametrize("stage", ["write", "fsync", "replace"]) +@pytest.mark.parametrize("existing", [False, True], ids=["new", "existing"]) +def test_publish_failure_preserves_target_and_cleans_temporary(tmp_path, monkeypatch, publisher, stage, existing): + target = tmp_path / "shot.png" + if existing: + target.write_bytes(b"old") + old_mode = target.stat().st_mode if existing else None + + def fail(*args, **kwargs): + raise OSError("publication failed") + + if stage == "write": + fdopen = os.fdopen + + class PartialWriter: + def __init__(self, handle): + self.handle = handle + + def __enter__(self): + return self + + def __exit__(self, *args): + self.handle.close() + + def write(self, content): + self.handle.write(content[:2]) + fail() + + monkeypatch.setattr(os, "fdopen", lambda *args, **kwargs: PartialWriter(fdopen(*args, **kwargs))) + else: + monkeypatch.setattr(os, stage, fail) + + with pytest.raises(OSError, match="publication failed"): + publisher(target, b"new complete capture") + + if existing: + assert target.read_bytes() == b"old" + assert target.stat().st_mode == old_mode + assert list(tmp_path.iterdir()) == [target] + else: + assert not target.exists() + assert list(tmp_path.iterdir()) == [] + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor permissions") +def test_publish_permission_failure_keeps_previous_file(tmp_path, monkeypatch, publisher): + target = tmp_path / "shot.png" + target.write_bytes(b"old") + target.chmod(0o640) + + def fail(*args): + raise OSError("permission setup failed") + + monkeypatch.setattr(os, "fchmod", fail) + with pytest.raises(OSError, match="permission setup failed"): + publisher(target, b"new") + + assert target.read_bytes() == b"old" + assert stat.S_IMODE(target.stat().st_mode) == 0o640 + assert list(tmp_path.iterdir()) == [target] + + +def test_screenshot_writes_go_through_the_atomic_publish(): + source = inspect.getsource(browser_automation_tools) + + # A truncating write left a partial capture behind while the function returned + # the name as if it had been saved. + assert "(outputs_path / name).write_bytes(" not in source + assert "_publish_bytes_atomically(outputs_path / name, content)" in source + + +def test_capture_writes_go_through_the_atomic_publish(): + source = inspect.getsource(browserless_tools) + + assert "(outputs_path / final_name).write_bytes(" not in source + assert "_publish_bytes_atomically(outputs_path / final_name, content)" in source + + +def test_both_tools_share_the_atomic_publisher(): + for module in (browser_automation_tools, browserless_tools): + assert module._publish_bytes_atomically is publish_bytes_atomically