diff --git a/CHANGELOG.md b/CHANGELOG.md index caa6786652..f259b8e9d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1247,6 +1247,19 @@ This release closes that milestone with **621 merged pull requests**. ### Fixed +- **browser:** `browser_screenshot` no longer replaces a file that already + exists in the thread's outputs directory. A screenshot saved as + `report.png` used to overwrite the user's own `report.png`, an earlier + capture, or another default-named capture taken in the same second; it now + takes the next free name (`report-1.png`, `report-2.png`, ...) like + `web_capture`, and the tool result reports the name actually used. The + complete capture is hard-linked onto the name, so a name taken concurrently + is skipped rather than replaced and the final name never shows a partial + file. Like Gateway uploads, the outputs directory needs hard-link support; + without it the tool reports an error instead of overwriting. A staged + capture swapped before publication, for example for a symlink to a host + file, is never linked into outputs; the tool reports an error. ([#6913]) + - **llm:** A provider error that arrives inside a streaming response that has already returned `200` is now retried like the same error sent as an HTTP status. An OpenAI-compatible gateway that answers `200` early, to keep slow @@ -11785,5 +11798,6 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6876]: https://github.com/bytedance/deer-flow/pull/6876 [#6897]: https://github.com/bytedance/deer-flow/pull/6897 [#6909]: https://github.com/bytedance/deer-flow/pull/6909 +[#6913]: https://github.com/bytedance/deer-flow/pull/6913 [#6930]: https://github.com/bytedance/deer-flow/pull/6930 [#6991]: https://github.com/bytedance/deer-flow/pull/6991 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index f93cf82cc6..915559b778 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -1002,6 +1002,15 @@ ### 修复 +- **浏览器：** `browser_screenshot` 不再替换线程 outputs 目录中已存在的文件。此前以 + `report.png` 保存的截图会覆盖用户自己的 `report.png`、更早的截图，或同一秒内生成的 + 另一张默认命名截图；现在会像 `web_capture` 一样改用下一个空闲名称（`report-1.png`、 + `report-2.png`……），工具结果会报告实际使用的名称。完整截图通过硬链接发布到目标名称， + 因此并发占用的名称会被跳过而不是被替换，最终名称也不会暴露写了一半的文件。与 Gateway + 上传相同，outputs 目录需要支持硬链接；不支持时工具会报错，而不会覆盖已有文件。发布前 + 被替换的暂存截图（例如换成指向宿主机文件的符号链接）绝不会被链接到 outputs，工具会 + 报错。 ([#6913]) + - **llm：** 流式响应已返回 `200` 之后才在流内报告的 provider 错误，现在会像以 HTTP 状态码返回的同一错误一样重试。为了用 SSE keepalive 维持慢请求而提前返回 `200` 的 OpenAI 兼容网关，会把之后发生的 504 作为 SSE `error` 事件发出；openai SDK 将其抛出为 @@ -9396,5 +9405,6 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#6876]: https://github.com/bytedance/deer-flow/pull/6876 [#6897]: https://github.com/bytedance/deer-flow/pull/6897 [#6909]: https://github.com/bytedance/deer-flow/pull/6909 +[#6913]: https://github.com/bytedance/deer-flow/pull/6913 [#6930]: https://github.com/bytedance/deer-flow/pull/6930 [#6991]: https://github.com/bytedance/deer-flow/pull/6991 diff --git a/README.md b/README.md index 03b035c398..06661b2917 100644 --- a/README.md +++ b/README.md @@ -2707,6 +2707,9 @@ captures from overwriting one another. Failed publication releases the placehold if cleanup fails, Gateway logs a warning with its path and preserves the original publication error or interruption. +`browser_screenshot` never replaces an existing `outputs/` file: a taken name +moves to the next free one, such as `report-1.png`. + 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/AGENTS.md b/backend/packages/harness/deerflow/AGENTS.md index 8390040b28..56968beebc 100644 --- a/backend/packages/harness/deerflow/AGENTS.md +++ b/backend/packages/harness/deerflow/AGENTS.md @@ -59,7 +59,8 @@ storage and transfer cost bounded relative to lossless PNG. The explicit `browser_screenshot` tool remains PNG because it creates a user-requested artifact. New automatic capture entry points must reuse the shared progress encoding definition in `tools.py` so the byte encoding and `.jpg` suffix cannot -drift. +drift. `browser_screenshot` never replaces an `outputs/` file: it hard-links +onto the first free `-N` name. ### Embedded Client (`packages/harness/deerflow/client.py`) diff --git a/backend/packages/harness/deerflow/community/browser_automation/tools.py b/backend/packages/harness/deerflow/community/browser_automation/tools.py index 034d257f9d..4116e4b2de 100644 --- a/backend/packages/harness/deerflow/community/browser_automation/tools.py +++ b/backend/packages/harness/deerflow/community/browser_automation/tools.py @@ -17,12 +17,16 @@ from __future__ import annotations import asyncio import contextlib +import errno import logging +import os import re +import stat from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path from typing import Annotated +from uuid import uuid4 from langchain.tools import InjectedToolCallId, tool from langchain_core.messages import ToolMessage @@ -35,6 +39,7 @@ 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 deerflow.utils.output_names import output_name_candidates from .session import BrowserSession, BrowserSessionManager, PageSnapshot, ScreenshotType, get_browser_session_manager @@ -48,6 +53,8 @@ _OUTPUTS_VIRTUAL_PREFIX = f"{VIRTUAL_PATH_PREFIX}/outputs" _BROWSER_FRAMES_DIRNAME = BROWSER_FRAMES_DIRNAME _FRAMES_VIRTUAL_PREFIX = f"{_OUTPUTS_VIRTUAL_PREFIX}/{_BROWSER_FRAMES_DIRNAME}" _SAFE_FILENAME_RE = re.compile(r"[^A-Za-z0-9._-]+") +# Without no-follow support, os.link may follow a symlink at the staged name. +_LINK_CAN_SKIP_SYMLINKS = os.link in os.supports_follow_symlinks @dataclass(frozen=True) @@ -440,6 +447,64 @@ def _write_screenshot(outputs_path: Path, name: str, content: bytes) -> str: return name +def _publish_screenshot_no_overwrite(outputs_path: Path, name: str, content: bytes) -> str: + """Publish *content* under *name*, or its first free ``-1``, ``-2``, ... variant. + + The outputs directory also holds the user's own deliverables, so a + screenshot never replaces an existing file. The complete capture is staged + under a hidden, exclusively created sibling (``0666 & ~umask``, like + ``Path.write_bytes``), then hard-linked onto each candidate name: the + ``os.link`` is the whole no-overwrite guard, failing with + :class:`FileExistsError` when the name exists as anything (a file, a + symlink, a concurrent capture), so no name is checked before it is claimed. + The final name never shows a partial capture. Staging shares the outputs + directory, so the link is same-filesystem; like Gateway uploads, outputs + need hard-link support and other link failures propagate. + + The sandbox can write ``outputs/`` too, so it could swap the staged name for + a symlink to a host file before the link. The link does not follow symlinks + where supported, and the published name must be the regular-file inode + written here; anything else is removed and the capture fails. The staged + descriptor stays open until that check: an unlinked but open inode cannot be + freed, so a swapped-in file can never reuse its number (Linux filesystems + reuse a freed inode number at once) and pass as the capture. + """ + outputs_path.mkdir(parents=True, exist_ok=True) + staged = outputs_path / f".{name}.{uuid4().hex}.tmp" + fd = os.open(staged, os.O_CREAT | os.O_EXCL | os.O_WRONLY | getattr(os, "O_BINARY", 0), 0o666) + try: + try: + handle = os.fdopen(fd, "wb") + except BaseException: + os.close(fd) + raise + with handle: + handle.write(content) + handle.flush() + os.fsync(handle.fileno()) + staged_stat = os.fstat(handle.fileno()) + for candidate in output_name_candidates(name): + destination = outputs_path / candidate + try: + if _LINK_CAN_SKIP_SYMLINKS: + os.link(staged, destination, follow_symlinks=False) + else: + os.link(staged, destination) + except FileExistsError: + continue + published = os.lstat(destination) + if not stat.S_ISREG(published.st_mode) or not os.path.samestat(published, staged_stat): + destination.unlink(missing_ok=True) + raise OSError(f"Staged browser screenshot changed before publication: {staged.name}") + return candidate + raise FileExistsError(errno.EEXIST, "No free screenshot name", str(outputs_path / name)) + finally: + try: + staged.unlink(missing_ok=True) + except OSError: + logger.warning("Failed to remove staged browser screenshot: %s", staged, exc_info=True) + + @tool("browser_screenshot", parse_docstring=True) async def browser_screenshot_tool( runtime: Runtime, @@ -454,7 +519,7 @@ async def browser_screenshot_tool( stateless page load. Args: - filename: Optional output filename (extension is forced to .png). + filename: Optional output filename (extension is forced to .png). An existing file is never replaced; the screenshot takes the next free name such as report-1.png. full_page: Capture the full scrollable page instead of just the viewport. """ try: @@ -464,7 +529,7 @@ async def browser_screenshot_tool( with _resolve_session(runtime, "browser_screenshot") as session: content = await session.screenshot_bytes(full_page=full_page) name = _safe_screenshot_name(filename) - final_name = await asyncio.to_thread(_write_screenshot, outputs_path, name, content) + final_name = await asyncio.to_thread(_publish_screenshot_no_overwrite, outputs_path, name, content) virtual_path = f"{_OUTPUTS_VIRTUAL_PREFIX}/{final_name}" return Command( update={ diff --git a/backend/packages/harness/deerflow/community/browserless/tools.py b/backend/packages/harness/deerflow/community/browserless/tools.py index 9881d80ccf..0937a858ef 100644 --- a/backend/packages/harness/deerflow/community/browserless/tools.py +++ b/backend/packages/harness/deerflow/community/browserless/tools.py @@ -17,6 +17,7 @@ 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.output_names import output_name_candidates from deerflow.utils.readability import ReadabilityExtractor from .browserless_client import BrowserlessClient, BrowserlessFetchResult, BrowserlessScreenshotResult @@ -32,11 +33,6 @@ _OUTPUT_FORMAT_TO_EXTENSION = { "webp": "webp", } _SAFE_FILENAME_RE = re.compile(r"[^A-Za-z0-9._-]+") -# Cap collision-suffix probing so a saturated outputs directory cannot spin forever. -_MAX_FILENAME_COLLISION_PROBES = 1000 -# Retry bound for the timestamped fallback: the microseconds stamp can only repeat -# when another writer claimed that exact name within the same microsecond. -_MAX_TIMESTAMP_CLAIM_ATTEMPTS = 5 DEFAULT_BASE_URL = "http://localhost:3032" _UNSET = object() @@ -257,21 +253,9 @@ def _dedupe_output_name(outputs_path: Path, output_name: str) -> str: means another writer is holding them; returning a name anyway would reintroduce the silent overwrite this function exists to prevent. """ - stem = Path(output_name).stem - suffix = Path(output_name).suffix - if _reserve_output_name(outputs_path, output_name): - return output_name - - for index in range(1, _MAX_FILENAME_COLLISION_PROBES + 1): - probe = f"{stem}-{index}{suffix}" - if _reserve_output_name(outputs_path, probe): - return probe - - for _ in range(_MAX_TIMESTAMP_CLAIM_ATTEMPTS): - timestamp = datetime.now(UTC).strftime("%Y%m%d-%H%M%S-%f") - fallback = f"{stem}-{timestamp}{suffix}" - if _reserve_output_name(outputs_path, fallback): - return fallback + for candidate in output_name_candidates(output_name): + if _reserve_output_name(outputs_path, candidate): + return candidate raise RuntimeError(f"could not reserve an output filename for {output_name!r} in {outputs_path}") diff --git a/backend/packages/harness/deerflow/utils/AGENTS.md b/backend/packages/harness/deerflow/utils/AGENTS.md index a67b21d725..88d007d757 100644 --- a/backend/packages/harness/deerflow/utils/AGENTS.md +++ b/backend/packages/harness/deerflow/utils/AGENTS.md @@ -6,6 +6,8 @@ capture/fetch publication. It writes and fsyncs a hidden exclusive sibling befor 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`. +Both tools take collision names from `output_names.output_name_candidates` +(name, `-1`..`-1000`, five timestamps); claim each atomically, never pre-check. ### Model Retry Hints diff --git a/backend/packages/harness/deerflow/utils/output_names.py b/backend/packages/harness/deerflow/utils/output_names.py new file mode 100644 index 0000000000..973c542a0f --- /dev/null +++ b/backend/packages/harness/deerflow/utils/output_names.py @@ -0,0 +1,28 @@ +"""Collision-free output file names shared by the capture tools.""" + +from __future__ import annotations + +from collections.abc import Iterator +from datetime import UTC, datetime +from pathlib import Path + +# Cap collision-suffix probing so a saturated outputs directory cannot spin forever. +MAX_COLLISION_PROBES = 1000 +# Retry bound for the timestamped fallback: the microseconds stamp can only repeat +# when another writer claimed that exact name within the same microsecond. +MAX_TIMESTAMP_ATTEMPTS = 5 + + +def output_name_candidates(name: str) -> Iterator[str]: + """Yield *name*, then ``stem-1`` .. ``stem-1000``, then timestamped fallbacks. + + Callers claim each candidate atomically and move on when it is taken; the + generator never checks the filesystem, so naming cannot race the claim. + Timestamps are taken lazily, one per fallback attempt. + """ + yield name + stem, suffix = Path(name).stem, Path(name).suffix + for index in range(1, MAX_COLLISION_PROBES + 1): + yield f"{stem}-{index}{suffix}" + for _ in range(MAX_TIMESTAMP_ATTEMPTS): + yield f"{stem}-{datetime.now(UTC).strftime('%Y%m%d-%H%M%S-%f')}{suffix}" diff --git a/backend/tests/test_browser_screenshot_no_overwrite.py b/backend/tests/test_browser_screenshot_no_overwrite.py new file mode 100644 index 0000000000..dec3f0fe90 --- /dev/null +++ b/backend/tests/test_browser_screenshot_no_overwrite.py @@ -0,0 +1,237 @@ +# 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") diff --git a/backend/tests/test_browserless_capture_name_reservation.py b/backend/tests/test_browserless_capture_name_reservation.py index 7c73583484..1afd54647d 100644 --- a/backend/tests/test_browserless_capture_name_reservation.py +++ b/backend/tests/test_browserless_capture_name_reservation.py @@ -22,6 +22,7 @@ from datetime import UTC, datetime import pytest tools = importlib.import_module("deerflow.community.browserless.tools") +output_names = importlib.import_module("deerflow.utils.output_names") @pytest.fixture() @@ -152,7 +153,7 @@ def test_interruption_propagates_and_frees_the_name(outputs, monkeypatch): def test_probe_bound_is_inclusive(outputs, monkeypatch): """The last probe index is reachable, so a directory full of ``-1``/``-2`` still gets ``-3``.""" - monkeypatch.setattr(tools, "_MAX_FILENAME_COLLISION_PROBES", 3) + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 3) for name in ("report.png", "report-1.png", "report-2.png"): (outputs / name).write_bytes(b"earlier") @@ -160,7 +161,7 @@ def test_probe_bound_is_inclusive(outputs, monkeypatch): def test_saturated_probe_range_still_reserves_a_fresh_name(outputs, monkeypatch): - monkeypatch.setattr(tools, "_MAX_FILENAME_COLLISION_PROBES", 2) + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 2) taken = ["report.png", "report-1.png", "report-2.png", "report-3.png"] for name in taken: (outputs / name).write_bytes(b"earlier") @@ -185,8 +186,8 @@ def test_exhausted_timestamp_claims_refuse_to_overwrite(outputs, monkeypatch): def now(cls, tz=None): return datetime(2026, 10, 14, 12, 0, 0, 123456, tzinfo=UTC) - monkeypatch.setattr(tools, "_MAX_FILENAME_COLLISION_PROBES", 1) - monkeypatch.setattr(tools, "datetime", _FrozenClock) + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 1) + monkeypatch.setattr(output_names, "datetime", _FrozenClock) held = ["report.png", "report-1.png", "report-20261014-120000-123456.png"] for name in held: (outputs / name).write_bytes(b"earlier") diff --git a/backend/tests/test_output_names.py b/backend/tests/test_output_names.py new file mode 100644 index 0000000000..67e34e78d5 --- /dev/null +++ b/backend/tests/test_output_names.py @@ -0,0 +1,78 @@ +# SPDX-License-Identifier: MIT +"""Both capture tools take output names from one shared candidate sequence.""" + +from __future__ import annotations + +import importlib +from datetime import UTC, datetime +from itertools import islice +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from deerflow.utils import output_names + +browser_automation_tools = importlib.import_module("deerflow.community.browser_automation.tools") +browserless_tools = importlib.import_module("deerflow.community.browserless.tools") + + +class _StepClock(datetime): + """Each call advances one microsecond, so every fallback stamp is distinct.""" + + ticks = 0 + + @classmethod + def now(cls, tz=None): + cls.ticks += 1 + return datetime(2026, 10, 18, 12, 0, 0, cls.ticks, tzinfo=UTC) + + +def test_candidates_keep_the_name_then_number_then_timestamp(monkeypatch): + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 2) + monkeypatch.setattr(_StepClock, "ticks", 0) + monkeypatch.setattr(output_names, "datetime", _StepClock) + + assert list(output_names.output_name_candidates("report.png")) == [ + "report.png", + "report-1.png", + "report-2.png", + *(f"report-20261018-120000-{tick:06d}.png" for tick in range(1, output_names.MAX_TIMESTAMP_ATTEMPTS + 1)), + ] + + +def test_candidates_read_the_clock_only_when_a_fallback_is_reached(monkeypatch): + monkeypatch.setattr(_StepClock, "ticks", 0) + monkeypatch.setattr(output_names, "datetime", _StepClock) + + assert len(list(islice(output_names.output_name_candidates("a.png"), output_names.MAX_COLLISION_PROBES + 1))) == output_names.MAX_COLLISION_PROBES + 1 + assert _StepClock.ticks == 0 + + +def test_both_capture_tools_share_the_candidate_sequence(): + for module in (browser_automation_tools, browserless_tools): + assert module.output_name_candidates is output_names.output_name_candidates + + +@pytest.mark.asyncio +async def test_screenshot_retries_a_held_timestamp_fallback(tmp_path, monkeypatch): + # Browserless already retried a held timestamped name; the screenshot tool + # now shares that bound instead of giving up after one stamp. + monkeypatch.setattr(output_names, "MAX_COLLISION_PROBES", 1) + monkeypatch.setattr(_StepClock, "ticks", 0) + monkeypatch.setattr(output_names, "datetime", _StepClock) + held = ["report.png", "report-1.png", "report-20261018-120000-000001.png"] + for name in held: + (tmp_path / name).write_bytes(b"earlier") + + session = MagicMock() + session.screenshot_bytes = AsyncMock(return_value=b"shot") + manager = MagicMock() + manager.get_session.return_value = session + runtime = SimpleNamespace(context={"thread_id": "thread-1"}, state={"thread_data": {"outputs_path": str(tmp_path)}}) + with patch.object(browser_automation_tools, "get_browser_session_manager", return_value=manager), patch.object(browser_automation_tools, "_get_tool_config", return_value={}): + result = await browser_automation_tools.browser_screenshot_tool.coroutine(runtime=runtime, tool_call_id="t1", filename="report.png") + + assert result.update["artifacts"] == ["/mnt/user-data/outputs/report-20261018-120000-000002.png"] + assert (tmp_path / "report-20261018-120000-000002.png").read_bytes() == b"shot" + assert all((tmp_path / name).read_bytes() == b"earlier" for name in held)