fix(browserless): surface target-page error status in web_fetch_tool (#4239)

* fix(browserless): surface target-page error status in web_fetch_tool Browserless returns HTTP 200 for the render request itself even when the target page responded with a 4xx/5xx or served an anti-bot block page, tagging the real outcome on X-Response-Code/X-Response-Status headers. capture_screenshot/web_capture_tool already reads these headers and surfaces a warning via _target_status_warning. fetch_html only logged them at debug level and web_fetch_tool returned the block/error page's raw text as if it were a normal successful fetch, with no indication anything was wrong. fetch_html now returns a BrowserlessFetchResult carrying the rendered HTML plus the target-status headers (mirroring BrowserlessScreenshotResult), and web_fetch_tool appends the same _target_status_warning used by web_capture_tool when the target page errored. Legitimate 200-target fetches are unaffected. * fix(browserless): keep fetch_html() returning a plain string BrowserlessClient is re-exported from deerflow.community.browserless.__all__, so fetch_html() is public harness API with an established str-only contract: the rendered HTML on success, or an "Error: ..." string on failure. Changing its return type to BrowserlessFetchResult broke that contract for any caller that treats the result as a string (.lower(), concatenation, passing to a parser), even when the fetch itself succeeded. fetch_html() is now a thin wrapper that always unwraps back to the original str contract. The richer, status-aware result (needed to tell a genuine 200 apart from a render-succeeded-but-target-errored response) moves to a new fetch_html_with_status() method, which web_fetch_tool now calls instead so it keeps surfacing the target-page-error warning. Tests: retarget the tool-level mocks onto fetch_html_with_status, add a direct regression asserting fetch_html() returns str on success - including when the target page itself errored under a 200 render response - and keep the status-aware coverage on the new method.
2026-07-21 02:05:45 +00:00 · 2026-07-19 20:08:52 +08:00
parent 5b65d543b1
commit d075be0277
3 changed files with 265 additions and 19 deletions
@@ -7,6 +7,13 @@ import httpx
 logger = logging.getLogger(__name__)


+@dataclass(frozen=True)
+class BrowserlessFetchResult:
+    html: str
+    target_status_code: str
+    target_status: str
+
+
@dataclass(frozen=True)
 class BrowserlessScreenshotResult:
    content: bytes
@@ -43,6 +50,53 @@ class BrowserlessClient:
    ) -> str:
        """Fetch the rendered HTML of a page using Browserless.

+        Public string contract: this always returns either the rendered HTML or
+        an "Error: ..." string, never a richer result object. Callers that also
+        need the target page's real status (Browserless returns HTTP 200 for the
+        render request itself even when the target page responded with a 4xx/5xx
+        or an anti-bot block page) should use fetch_html_with_status() instead.
+
+        Args:
+            url: The URL to fetch.
+            wait_for_event: Wait for a page event (e.g. "networkidle", "load").
+            wait_for_timeout_ms: Extra wait after page load.
+            wait_for_selector: CSS selector to wait for.
+            wait_for_selector_timeout_ms: Timeout for selector wait.
+            reject_resource_types: Resource types to block (e.g. ["image"]).
+            reject_request_pattern: URL patterns to block.
+
+        Returns:
+            Rendered HTML content, or an "Error: ..." string on failure.
+        """
+        result = await self.fetch_html_with_status(
+            url=url,
+            wait_for_event=wait_for_event,
+            wait_for_timeout_ms=wait_for_timeout_ms,
+            wait_for_selector=wait_for_selector,
+            wait_for_selector_timeout_ms=wait_for_selector_timeout_ms,
+            reject_resource_types=reject_resource_types,
+            reject_request_pattern=reject_request_pattern,
+        )
+        return result.html if isinstance(result, BrowserlessFetchResult) else result
+
+    async def fetch_html_with_status(
+        self,
+        url: str,
+        wait_for_event: str = "",
+        wait_for_timeout_ms: int = 0,
+        wait_for_selector: str = "",
+        wait_for_selector_timeout_ms: int = 5000,
+        reject_resource_types: list[str] | None = None,
+        reject_request_pattern: list[str] | None = None,
+    ) -> BrowserlessFetchResult | str:
+        """Fetch the rendered HTML of a page using Browserless, with target status.
+
+        Same request/response handling as fetch_html(), except a successful fetch
+        returns a BrowserlessFetchResult carrying the target page's real status
+        headers instead of a bare string, so a caller can tell a genuine 200 apart
+        from a render-succeeded-but-target-errored (or anti-bot blocked) response.
+        Use fetch_html() instead when only the HTML/error string is needed.
+
        Only sends accepted parameters for the current Browserless API version.
        Sets a default navigation timeout (30s) via query param.

@@ -56,7 +110,8 @@ class BrowserlessClient:
            reject_request_pattern: URL patterns to block.

        Returns:
-            Rendered HTML content.
+            Fetch result with the rendered HTML and target-page status headers,
+            or an "Error: ..." string on failure.
        """
        payload: dict[str, Any] = {
            "url": url,
@@ -103,7 +158,11 @@ class BrowserlessClient:
                if not html or not html.strip():
                    return "Error: Browserless returned empty response"

-                return html
+                return BrowserlessFetchResult(
+                    html=html,
+                    target_status_code=_get_header(resp.headers, "X-Response-Code"),
+                    target_status=_get_header(resp.headers, "X-Response-Status"),
+                )

        except httpx.TimeoutException:
            return f"Error: Browserless request timed out after {self.timeout_s}s"
@@ -18,7 +18,7 @@ from deerflow.config.paths import VIRTUAL_PATH_PREFIX
 from deerflow.tools.types import Runtime
 from deerflow.utils.readability import ReadabilityExtractor

-from .browserless_client import BrowserlessClient, BrowserlessScreenshotResult
+from .browserless_client import BrowserlessClient, BrowserlessFetchResult, BrowserlessScreenshotResult

 logger = logging.getLogger(__name__)

@@ -173,13 +173,13 @@ def _write_capture_output(outputs_path: Path, output_name: str, content: bytes)
    return final_name


-def _target_status_warning(result: BrowserlessScreenshotResult) -> str:
-    """Return a human-readable warning when the captured page itself errored.
+def _target_status_warning(result: BrowserlessScreenshotResult | BrowserlessFetchResult) -> str:
+    """Return a human-readable warning when the fetched/captured page itself errored.

    Browserless returns HTTP 200 for the render request even when the target
    page responded with a 4xx/5xx (or was an error/anti-bot page), so the raw
-    image alone cannot be trusted as valid visual evidence. The target's real
-    status is surfaced via the X-Response-Code header.
+    content alone cannot be trusted as a normal successful response. The
+    target's real status is surfaced via the X-Response-Code header.
    """
    code = result.target_status_code.strip()
    if not code or code.startswith(("2", "3")):
@@ -224,7 +224,7 @@ async def web_fetch_tool(url: str) -> str:
        wait_for_selector = cfg.get("wait_for_selector", wait_for_selector)

        client = _get_browserless_client("web_fetch")
-        html = await client.fetch_html(
+        result = await client.fetch_html_with_status(
            url=url,
            wait_for_event=wait_for_event,
            wait_for_timeout_ms=wait_for_timeout_ms,
@@ -234,11 +234,11 @@ async def web_fetch_tool(url: str) -> str:
            reject_request_pattern=reject_request_pattern,
        )

-        if html.startswith("Error:"):
-            return html
+        if isinstance(result, str):
+            return result

-        article = await asyncio.to_thread(_readability_extractor.extract_article, html)
-        return article.to_markdown()[:4096]
+        article = await asyncio.to_thread(_readability_extractor.extract_article, result.html)
+        return f"{article.to_markdown()[:4096]}{_target_status_warning(result)}"

    except Exception as e:
        logger.error(f"Error in web_fetch_tool: {e}")
@@ -7,7 +7,7 @@ from unittest.mock import MagicMock, patch
 import pytest

 from deerflow.community.browserless import tools
-from deerflow.community.browserless.browserless_client import BrowserlessClient, BrowserlessScreenshotResult
+from deerflow.community.browserless.browserless_client import BrowserlessClient, BrowserlessFetchResult, BrowserlessScreenshotResult


 class AsyncMock(MagicMock):
@@ -22,7 +22,15 @@ class TestBrowserlessClient:
    """Tests for the BrowserlessClient class."""

    async def test_fetch_html_success(self):
-        """fetch_html returns HTML content on success."""
+        """fetch_html returns the rendered HTML as a plain string on success.
+
+        Regression guard for the fetch_html() public string contract:
+        BrowserlessClient is re-exported from deerflow.community.browserless.__all__,
+        so harness consumers that call string methods, compare the result, or pass
+        it directly to a parser must keep getting a str back. Status-aware callers
+        (e.g. web_fetch_tool) use fetch_html_with_status() instead - see
+        test_fetch_html_with_status_surfaces_target_status_headers below.
+        """
        with patch("deerflow.community.browserless.browserless_client.httpx.AsyncClient") as mock_cls:
            mock_ctx = MagicMock()
            mock_cls.return_value.__aenter__.return_value = mock_ctx
@@ -36,6 +44,7 @@ class TestBrowserlessClient:
            client = BrowserlessClient(base_url="http://browserless:3000")
            result = await client.fetch_html("https://example.com")

+            assert isinstance(result, str)
            assert result == "<html><body>Page content</body></html>"
            call_kwargs = mock_ctx.post.call_args.kwargs
            assert call_kwargs["json"]["url"] == "https://example.com"
@@ -43,6 +52,67 @@ class TestBrowserlessClient:
            assert "gotoTimeout" not in call_kwargs["json"]
            assert "bestAttempt" not in call_kwargs["json"]

+    async def test_fetch_html_returns_plain_string_even_when_target_page_errored(self):
+        """fetch_html stays a plain string even when the target page itself errored.
+
+        Browserless returns HTTP 200 for the render request itself even when the
+        target page responded with a 404 (or an anti-bot block page). Before this
+        fix, a successful fetch_html() call started returning a BrowserlessFetchResult
+        object instead of a string in exactly this case, breaking existing callers
+        (.lower(), string concatenation, parsers expecting str) even though their
+        fetch technically succeeded. fetch_html() must keep unwrapping to the plain
+        HTML string regardless of the target status; only fetch_html_with_status()
+        exposes the richer result.
+        """
+        with patch("deerflow.community.browserless.browserless_client.httpx.AsyncClient") as mock_cls:
+            mock_ctx = MagicMock()
+            mock_cls.return_value.__aenter__.return_value = mock_ctx
+
+            mock_resp = MagicMock()
+            mock_resp.status_code = 200
+            mock_resp.text = "<html><body>Access Denied</body></html>"
+            mock_resp.headers = {
+                "X-Response-Code": "404",
+                "X-Response-Status": "Not Found",
+            }
+            mock_ctx.post = AsyncMock(return_value=mock_resp)
+
+            client = BrowserlessClient(base_url="http://browserless:3000")
+            result = await client.fetch_html("https://example.com/blocked")
+
+            assert isinstance(result, str)
+            assert result == "<html><body>Access Denied</body></html>"
+
+    async def test_fetch_html_with_status_surfaces_target_status_headers(self):
+        """fetch_html_with_status carries the target page's real status headers on the result.
+
+        Browserless returns HTTP 200 for the render request itself even when the
+        target page responded with a 404 (or an anti-bot block page), so status-aware
+        callers need the X-Response-Code/X-Response-Status headers to tell the two
+        apart. This richer result is opt-in via fetch_html_with_status(); plain
+        fetch_html() stays a str (see test_fetch_html_success above).
+        """
+        with patch("deerflow.community.browserless.browserless_client.httpx.AsyncClient") as mock_cls:
+            mock_ctx = MagicMock()
+            mock_cls.return_value.__aenter__.return_value = mock_ctx
+
+            mock_resp = MagicMock()
+            mock_resp.status_code = 200
+            mock_resp.text = "<html><body>Access Denied</body></html>"
+            mock_resp.headers = {
+                "X-Response-Code": "404",
+                "X-Response-Status": "Not Found",
+            }
+            mock_ctx.post = AsyncMock(return_value=mock_resp)
+
+            client = BrowserlessClient(base_url="http://browserless:3000")
+            result = await client.fetch_html_with_status("https://example.com/blocked")
+
+            assert isinstance(result, BrowserlessFetchResult)
+            assert result.html == "<html><body>Access Denied</body></html>"
+            assert result.target_status_code == "404"
+            assert result.target_status == "Not Found"
+
    async def test_fetch_html_empty_response(self):
        """fetch_html returns error for empty response."""
        with patch("deerflow.community.browserless.browserless_client.httpx.AsyncClient") as mock_cls:
@@ -253,19 +323,26 @@ class TestBrowserlessTools:
    async def test_web_fetch_tool_success(self, mock_get_client):
        """web_fetch_tool successfully fetches and extracts content."""
        mock_client = MagicMock()
-        mock_client.fetch_html = AsyncMock(return_value="<html><body><article><h1>Title</h1><p>Content</p></article></body></html>")
+        mock_client.fetch_html_with_status = AsyncMock(
+            return_value=BrowserlessFetchResult(
+                html="<html><body><article><h1>Title</h1><p>Content</p></article></body></html>",
+                target_status_code="200",
+                target_status="OK",
+            )
+        )
        mock_get_client.return_value = mock_client

        with patch("deerflow.community.browserless.tools._get_tool_config", return_value=None):
            result = await tools.web_fetch_tool.ainvoke("https://example.com/article")

        assert "Error:" not in result
+        assert "warning:" not in result

    @patch("deerflow.community.browserless.tools._get_browserless_client")
    async def test_web_fetch_tool_error(self, mock_get_client):
        """web_fetch_tool returns error when fetch fails."""
        mock_client = MagicMock()
-        mock_client.fetch_html = AsyncMock(return_value="Error: Browserless returned empty response")
+        mock_client.fetch_html_with_status = AsyncMock(return_value="Error: Browserless returned empty response")
        mock_get_client.return_value = mock_client

        with patch("deerflow.community.browserless.tools._get_tool_config", return_value=None):
@@ -277,7 +354,7 @@ class TestBrowserlessTools:
    async def test_web_fetch_tool_exception(self, mock_get_client):
        """web_fetch_tool returns error when client raises exception."""
        mock_client = MagicMock()
-        mock_client.fetch_html = AsyncMock(side_effect=Exception("Unexpected error"))
+        mock_client.fetch_html_with_status = AsyncMock(side_effect=Exception("Unexpected error"))
        mock_get_client.return_value = mock_client

        with patch("deerflow.community.browserless.tools._get_tool_config", return_value=None):
@@ -311,14 +388,124 @@ class TestBrowserlessTools:
    async def test_web_fetch_tool_allows_private_when_opted_in(self, mock_get_client):
        """web_fetch_tool allows internal targets only when explicitly configured."""
        mock_client = MagicMock()
-        mock_client.fetch_html = AsyncMock(return_value="<html><body><article><p>internal</p></article></body></html>")
+        mock_client.fetch_html_with_status = AsyncMock(
+            return_value=BrowserlessFetchResult(
+                html="<html><body><article><p>internal</p></article></body></html>",
+                target_status_code="200",
+                target_status="OK",
+            )
+        )
        mock_get_client.return_value = mock_client

        with patch("deerflow.community.browserless.tools._get_tool_config", return_value={"allow_private_addresses": True}):
            result = await tools.web_fetch_tool.ainvoke("http://10.0.0.5/dashboard")

        assert "Error:" not in result
-        mock_client.fetch_html.assert_called_once()
+        mock_client.fetch_html_with_status.assert_called_once()
+
+    @patch("deerflow.community.browserless.tools._get_browserless_client")
+    async def test_web_fetch_tool_warns_on_target_error_status(self, mock_get_client):
+        """web_fetch_tool surfaces a warning when the fetched page itself errored.
+
+        Mirrors test_web_capture_tool_warns_on_target_error_status below:
+        Browserless returns HTTP 200 for the render request even when the target
+        page is a 404 or an anti-bot block page, so fetch_html_with_status's
+        target-status headers must produce the same visible warning
+        web_capture_tool already gives via _target_status_warning.
+        """
+        mock_client = MagicMock()
+        mock_client.fetch_html_with_status = AsyncMock(
+            return_value=BrowserlessFetchResult(
+                html="<html><body><article><h1>Not Found</h1><p>The page does not exist.</p></article></body></html>",
+                target_status_code="404",
+                target_status="Not Found",
+            )
+        )
+        mock_get_client.return_value = mock_client
+
+        with patch("deerflow.community.browserless.tools._get_tool_config", return_value=None):
+            result = await tools.web_fetch_tool.ainvoke("https://example.com/missing")
+
+        assert "Error:" not in result
+        assert "warning: target page responded 404 Not Found" in result
+
+    @patch("deerflow.community.browserless.tools._get_browserless_client")
+    async def test_web_fetch_tool_no_warning_for_normal_target_status(self, mock_get_client):
+        """web_fetch_tool does not warn when the target page responded normally.
+
+        Regression guard: a legitimate 200-target-page fetch must be unaffected
+        by the target-status warning added for error/blocked pages.
+        """
+        mock_client = MagicMock()
+        mock_client.fetch_html_with_status = AsyncMock(
+            return_value=BrowserlessFetchResult(
+                html="<html><body><article><h1>Title</h1><p>Content</p></article></body></html>",
+                target_status_code="200",
+                target_status="OK",
+            )
+        )
+        mock_get_client.return_value = mock_client
+
+        with patch("deerflow.community.browserless.tools._get_tool_config", return_value=None):
+            result = await tools.web_fetch_tool.ainvoke("https://example.com/article")
+
+        assert "Error:" not in result
+        assert "warning:" not in result
+
+    async def test_web_fetch_and_web_capture_tools_agree_on_target_error_warning(self, tmp_path):
+        """web_fetch_tool and web_capture_tool surface the identical warning for identical target-error headers.
+
+        Both tools sit on top of the same Browserless target-status headers
+        (X-Response-Code / X-Response-Status). Before this fix, only
+        web_capture_tool (via capture_screenshot + _target_status_warning)
+        surfaced them; web_fetch_tool (via fetch_html_with_status) silently
+        returned the blocked page's raw content with no indication anything was
+        wrong. This drives both real tool functions with the same headers and
+        asserts they now agree.
+        """
+        outputs_dir = tmp_path / "outputs"
+        outputs_dir.mkdir()
+        runtime = SimpleNamespace(state={"thread_data": {"outputs_path": str(outputs_dir)}})
+
+        fetch_client = MagicMock()
+        fetch_client.fetch_html_with_status = AsyncMock(
+            return_value=BrowserlessFetchResult(
+                html="<html><body><article><h1>Blocked</h1><p>Access denied.</p></article></body></html>",
+                target_status_code="404",
+                target_status="Not Found",
+            )
+        )
+
+        capture_client = MagicMock()
+        capture_client.capture_screenshot = AsyncMock(
+            return_value=BrowserlessScreenshotResult(
+                content=b"\x89PNG\r\n\x1a\nimage",
+                content_type="image/png",
+                target_status_code="404",
+                target_status="Not Found",
+                final_url="https://example.com/missing",
+            )
+        )
+
+        with patch("deerflow.community.browserless.tools._get_tool_config", return_value=None):
+            with patch("deerflow.community.browserless.tools._get_browserless_client", return_value=fetch_client):
+                fetch_result = await tools.web_fetch_tool.ainvoke("https://example.com/missing")
+
+            with patch(
+                "deerflow.community.browserless.tools._resolve_host_addresses",
+                return_value=[ipaddress.ip_address("93.184.216.34")],
+            ):
+                with patch("deerflow.community.browserless.tools._get_browserless_client", return_value=capture_client):
+                    capture_command = await tools.web_capture_tool.coroutine(
+                        runtime=runtime,
+                        url="https://example.com/missing",
+                        tool_call_id="tool-1",
+                    )
+
+        capture_message = capture_command.update["messages"][0].content
+
+        assert "warning: target page responded 404 Not Found" in fetch_result
+        assert "warning: target page responded 404 Not Found" in capture_message

    @patch("deerflow.community.browserless.tools._get_browserless_client")
    async def test_web_capture_tool_writes_artifact(self, mock_get_client, tmp_path):