mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
* fix(browser): never replace an existing output file with browser_screenshot browser_screenshot published straight onto outputs/<name> through the replacing atomic publisher, so a screenshot saved as report.png silently replaced the user's own report.png, an earlier capture, or another default-named capture taken in the same second (the default stem has second resolution). The sibling web_capture tool already keeps existing files and takes a -1, -2, ... suffix. Stage the complete capture under a hidden sibling and hard-link it onto the first free candidate name. The link is the whole no-overwrite guard: it fails with FileExistsError when the name exists as anything, so no name is checked before it is claimed and a concurrent writer cannot be replaced; the final name never shows a partial file. Staging shares the outputs directory, so the link is same-filesystem, and like Gateway uploads other link failures propagate instead of falling back to replacement. New files keep the publisher's normal creation mode. Hidden per-action progress frames keep the replacing _write_screenshot: their names carry microsecond timestamps in a server-owned directory. * docs(changelog): reference #6913 for the browser_screenshot no-overwrite fix * fix(browser): refuse a staged screenshot swapped before it is linked The sandbox can write outputs/, so it can see the hidden staged capture and replace it with a symlink before the hard link. os.link follows symlinks by default, which would publish a hard link to the host file as the screenshot artifact (the replacing publisher this PR moved away from never followed one). Stage the capture with an exclusively created descriptor and keep its identity from fstat, link without following symlinks where the platform supports it (decided once at import), and require the published name to be that inode; anything else is removed and the capture fails, like Gateway uploads' identity check. A failed staging write still leaves no files behind. * docs(readme): note that browser_screenshot never replaces an output file * fix(browser): keep the staged screenshot open until its link is verified Linux filesystems reuse a freed inode number at once. Closing the staged capture before linking let a file swapped in at the staged name take the same number and pass the samestat check, so the symlink and planted-file swap tests failed on Linux CI while passing on macOS. Hold the staged descriptor through the link and identity check, as Gateway uploads do, and require the published name to be a regular file. * test(browser): race the peer claim against the real publication step The claimed-name test patched _publish_bytes_atomically, which the screenshot path no longer calls, so its hook never fired and the race assertions passed vacuously. Plant the peer file right before os.link or os.replace lands bytes on a visible name, and assert that the claim happened, the capture moved to report-1.png, and the peer file survived. * refactor(capture): share one output-name candidate sequence browser_screenshot hand-copied web_capture's probe bound and suffix order, and the copies had already drifted: Browserless retried a held timestamped name five times, the screenshot tool once. Move the bound, the retry count and the candidate order into utils/output_names.py and have both tools iterate it, so the naming cannot drift again. Browserless behavior is unchanged; its tests now patch the shared module. --------- Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
238 lines
9.2 KiB
Python
238 lines
9.2 KiB
Python
# SPDX-License-Identifier: MIT
|
|
"""``browser_screenshot`` must never replace a file already in the outputs directory."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import errno
|
|
import os
|
|
import stat
|
|
from pathlib import Path
|
|
from types import SimpleNamespace
|
|
from unittest.mock import AsyncMock, MagicMock, patch
|
|
|
|
import pytest
|
|
|
|
from deerflow.community.browser_automation import tools
|
|
|
|
_SHOT = b"\x89PNG\r\n\x1a\nscreenshot"
|
|
|
|
|
|
async def _screenshot(outputs: Path, filename: str | None = "report.png"):
|
|
session = MagicMock()
|
|
session.screenshot_bytes = AsyncMock(return_value=_SHOT)
|
|
manager = MagicMock()
|
|
manager.get_session.return_value = session
|
|
runtime = SimpleNamespace(context={"thread_id": "thread-1"}, state={"thread_data": {"outputs_path": str(outputs)}})
|
|
with patch.object(tools, "get_browser_session_manager", return_value=manager), patch.object(tools, "_get_tool_config", return_value={}):
|
|
return await tools.browser_screenshot_tool.coroutine(runtime=runtime, tool_call_id="t1", filename=filename)
|
|
|
|
|
|
def _artifact(result) -> str:
|
|
return result.update["artifacts"][0]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_screenshot_keeps_an_existing_output_and_takes_the_next_name(tmp_path):
|
|
(tmp_path / "report.png").write_bytes(b"user report")
|
|
|
|
result = await _screenshot(tmp_path)
|
|
|
|
assert _artifact(result) == "/mnt/user-data/outputs/report-1.png"
|
|
assert "Saved browser screenshot: /mnt/user-data/outputs/report-1.png" in result.update["messages"][0].content
|
|
assert (tmp_path / "report.png").read_bytes() == b"user report"
|
|
assert (tmp_path / "report-1.png").read_bytes() == _SHOT
|
|
assert sorted(path.name for path in tmp_path.iterdir()) == ["report-1.png", "report.png"]
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_repeated_screenshots_with_one_filename_keep_every_capture(tmp_path):
|
|
names = [_artifact(await _screenshot(tmp_path)).rsplit("/", 1)[1] for _ in range(3)]
|
|
|
|
assert names == ["report.png", "report-1.png", "report-2.png"]
|
|
assert sorted(path.name for path in tmp_path.iterdir()) == sorted(names)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_name_claimed_after_the_choice_is_not_replaced(tmp_path, monkeypatch):
|
|
# A peer (another tool call, or the user) can create a name after the
|
|
# publisher chose it. Simulate that window at the publication step itself:
|
|
# right before a link or replace lands the capture on a visible name, a
|
|
# peer claims that name. Only a publication that refuses occupied names at
|
|
# that step keeps the peer file.
|
|
link, replace = os.link, os.replace
|
|
claimed: list[Path] = []
|
|
|
|
def peer_claims(destination) -> None:
|
|
destination = Path(destination)
|
|
if not claimed and not destination.name.startswith("."):
|
|
destination.write_bytes(b"peer output")
|
|
claimed.append(destination)
|
|
|
|
def link_after_peer(source, destination, *args, **kwargs):
|
|
peer_claims(destination)
|
|
return link(source, destination, *args, **kwargs)
|
|
|
|
def replace_after_peer(source, destination, *args, **kwargs):
|
|
peer_claims(destination)
|
|
return replace(source, destination, *args, **kwargs)
|
|
|
|
monkeypatch.setattr(os, "link", link_after_peer)
|
|
monkeypatch.setattr(os, "replace", replace_after_peer)
|
|
result = await _screenshot(tmp_path)
|
|
|
|
assert claimed == [tmp_path / "report.png"]
|
|
assert _artifact(result) == "/mnt/user-data/outputs/report-1.png"
|
|
assert (tmp_path / "report.png").read_bytes() == b"peer output"
|
|
assert (tmp_path / "report-1.png").read_bytes() == _SHOT
|
|
|
|
|
|
@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink creation")
|
|
@pytest.mark.asyncio
|
|
async def test_screenshot_does_not_replace_a_symlink_at_the_name(tmp_path):
|
|
outputs = tmp_path / "outputs"
|
|
outputs.mkdir()
|
|
(outputs / "report.png").symlink_to(tmp_path / "missing-target.png")
|
|
|
|
result = await _screenshot(outputs)
|
|
|
|
assert _artifact(result) == "/mnt/user-data/outputs/report-1.png"
|
|
assert (outputs / "report.png").is_symlink()
|
|
assert not (tmp_path / "missing-target.png").exists()
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_failed_publication_reports_an_error_and_leaves_no_files(tmp_path, monkeypatch):
|
|
(tmp_path / "report.png").write_bytes(b"user report")
|
|
|
|
def no_hard_links(*args, **kwargs):
|
|
raise OSError(errno.EPERM, "hard links unsupported")
|
|
|
|
monkeypatch.setattr(os, "link", no_hard_links)
|
|
result = await _screenshot(tmp_path)
|
|
|
|
assert "artifacts" not in result.update
|
|
assert "Error: could not capture screenshot" in result.update["messages"][0].content
|
|
assert sorted(path.name for path in tmp_path.iterdir()) == ["report.png"]
|
|
assert (tmp_path / "report.png").read_bytes() == b"user report"
|
|
|
|
|
|
def _swap_staged_before_link(monkeypatch, replace_staged, watch: Path | None = None) -> list[int]:
|
|
"""Replace the staged capture right before it is linked, as a sandbox writing ``outputs/`` could.
|
|
|
|
Returns the link counts of *watch* observed right after each link call.
|
|
"""
|
|
link = os.link
|
|
link_counts: list[int] = []
|
|
|
|
def swapped_link(source, destination, *args, **kwargs):
|
|
source = Path(source)
|
|
if source.name.startswith(".") and source.is_file() and not source.is_symlink():
|
|
source.unlink()
|
|
replace_staged(source)
|
|
try:
|
|
return link(source, destination, *args, **kwargs)
|
|
finally:
|
|
if watch is not None:
|
|
link_counts.append(watch.stat().st_nlink)
|
|
|
|
monkeypatch.setattr(os, "link", swapped_link)
|
|
return link_counts
|
|
|
|
|
|
@pytest.mark.skipif(os.name == "nt", reason="POSIX symlink creation")
|
|
@pytest.mark.asyncio
|
|
async def test_a_staged_capture_swapped_for_a_symlink_never_publishes_its_target(tmp_path, monkeypatch):
|
|
outputs = tmp_path / "outputs"
|
|
outputs.mkdir()
|
|
secret = tmp_path / "host-secret.txt"
|
|
secret.write_bytes(b"host secret")
|
|
link_counts = _swap_staged_before_link(monkeypatch, lambda staged: staged.symlink_to(secret), watch=secret)
|
|
|
|
result = await _screenshot(outputs)
|
|
|
|
assert "artifacts" not in result.update
|
|
assert "Error: could not capture screenshot" in result.update["messages"][0].content
|
|
assert list(outputs.iterdir()) == []
|
|
# The host file never gains a name in outputs/, not even briefly.
|
|
assert link_counts and set(link_counts) == {1}
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_staged_capture_swapped_for_another_file_is_not_published(tmp_path, monkeypatch):
|
|
_swap_staged_before_link(monkeypatch, lambda staged: staged.write_bytes(b"planted"))
|
|
|
|
result = await _screenshot(tmp_path)
|
|
|
|
assert "artifacts" not in result.update
|
|
assert "Error: could not capture screenshot" in result.update["messages"][0].content
|
|
assert list(tmp_path.iterdir()) == []
|
|
|
|
|
|
def _open_descriptors_for(target: os.stat_result) -> list[int]:
|
|
descriptors = []
|
|
for entry in os.listdir("/dev/fd"):
|
|
try:
|
|
if os.path.samestat(os.fstat(int(entry)), target):
|
|
descriptors.append(int(entry))
|
|
except OSError:
|
|
continue
|
|
return descriptors
|
|
|
|
|
|
@pytest.mark.skipif(not os.path.isdir("/dev/fd"), reason="needs /dev/fd to list open descriptors")
|
|
@pytest.mark.asyncio
|
|
async def test_the_staged_capture_stays_open_while_it_is_linked(tmp_path, monkeypatch):
|
|
# Linux filesystems hand a freed inode number to the next file at once, so
|
|
# a capture whose descriptor is closed before the link lets a swapped-in
|
|
# file reuse its number and pass the identity check. macOS rarely reuses
|
|
# numbers, so the swap tests above only catch that there; pin the guard
|
|
# itself: an open descriptor keeps the staged inode allocated.
|
|
link = os.link
|
|
open_at_link: list[list[int]] = []
|
|
|
|
def observing_link(source, destination, *args, **kwargs):
|
|
open_at_link.append(_open_descriptors_for(os.lstat(source)))
|
|
return link(source, destination, *args, **kwargs)
|
|
|
|
monkeypatch.setattr(os, "link", observing_link)
|
|
result = await _screenshot(tmp_path)
|
|
|
|
assert _artifact(result) == "/mnt/user-data/outputs/report.png"
|
|
assert open_at_link and all(open_at_link)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_a_failed_staging_write_leaves_no_files(tmp_path, monkeypatch):
|
|
def fail(*args):
|
|
raise OSError(errno.ENOSPC, "disk full")
|
|
|
|
monkeypatch.setattr(os, "fsync", fail)
|
|
result = await _screenshot(tmp_path)
|
|
|
|
assert "Error: could not capture screenshot" in result.update["messages"][0].content
|
|
assert list(tmp_path.iterdir()) == []
|
|
|
|
|
|
@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits and umask")
|
|
@pytest.mark.parametrize("mask", [0o022, 0o077])
|
|
@pytest.mark.asyncio
|
|
async def test_screenshot_uses_normal_creation_permissions(tmp_path, mask):
|
|
previous_mask = os.umask(mask)
|
|
try:
|
|
await _screenshot(tmp_path)
|
|
finally:
|
|
os.umask(previous_mask)
|
|
|
|
assert stat.S_IMODE((tmp_path / "report.png").stat().st_mode) == 0o666 & ~mask
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_default_names_from_the_same_second_do_not_collide(tmp_path, monkeypatch):
|
|
monkeypatch.setattr(tools, "_safe_screenshot_name", lambda filename: "browser-capture-20261015-120000.png")
|
|
|
|
first = _artifact(await _screenshot(tmp_path, filename=None))
|
|
second = _artifact(await _screenshot(tmp_path, filename=None))
|
|
|
|
assert first.endswith("/browser-capture-20261015-120000.png")
|
|
assert second.endswith("/browser-capture-20261015-120000-1.png")
|