fix(browserless): accept the timeout config key and harden coercion (#4519)

`browserless` reads `cfg["timeout_s"]`, while its sibling web providers
`crawl4ai` and `jina_ai` read `cfg["timeout"]`. Tool configs allow extra
fields, so the unrecognised spelling is dropped without a diagnostic: someone
adapting one provider's config snippet for another silently gets the 30s
default instead of the timeout they set. (Observed in the other direction, on a
deployment whose crawl4ai entry carried `timeout_s`.)

Accept both keys, preferring the documented `timeout_s` when both are present.

While adding coverage, two pre-existing bugs in the same three lines surfaced,
both already guarded in crawl4ai/jina_ai but not here:

- `timeout_s: "30s"` (or any non-numeric string) raised ValueError out of
  `float(raw)` during tool construction rather than falling back.
- `timeout_s: off` -- YAML parses that as `False`, and `float(False)` is
  `0.0`, so every request timed out immediately against a healthy server.

`_coerce_timeout` now mirrors the sibling providers: booleans and unparsable
strings fall back to the default, with a warning for the string case.

Tests: five cases in tests/test_browserless_client.py covering both keys, the
precedence order, and both coercion bugs. Verified red before the fix (3 of 5
fail) and green after.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
ajayr 2026-07-28 00:56:04 +01:00 committed by GitHub
parent d455a1815e
commit 6456c35675
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 67 additions and 3 deletions

View File

@ -44,6 +44,41 @@ def _get_tool_config(tool_name: str) -> dict | None:
return extras if extras is not None else {}
def _coerce_timeout(value: object, default: float) -> float:
"""Coerce a config timeout into seconds, falling back to ``default`` on bad input.
Mirrors ``crawl4ai._coerce_timeout`` / ``jina_ai._coerce_timeout``: booleans and
non-numeric strings fall back to the default, so ``timeout_s: off`` (YAML ``False``)
does not become ``0.0`` and time out every request against a healthy server, and a
typo'd value does not raise out of tool construction.
"""
if isinstance(value, bool):
return default
if isinstance(value, (int, float)):
return float(value)
if isinstance(value, str):
try:
return float(value)
except ValueError:
logger.warning("Browserless: invalid timeout %r in config; using %ss", value, default)
return default
def _resolve_timeout(cfg: dict, default: float) -> float:
"""Read the timeout, accepting this provider's key and the sibling providers' key.
``browserless`` documents ``timeout_s`` while ``crawl4ai`` and ``jina_ai`` read
``timeout``. An unrecognised key is dropped silently by the extra-fields tool config,
so someone adapting another provider's snippet got the default with no diagnostic.
Accept both, preferring the documented ``timeout_s`` when both are present.
"""
if "timeout_s" in cfg:
return _coerce_timeout(cfg["timeout_s"], default)
if "timeout" in cfg:
return _coerce_timeout(cfg["timeout"], default)
return default
def _get_browserless_client(tool_name: str = "web_fetch") -> BrowserlessClient:
cfg = _get_tool_config(tool_name)
base_url = "http://localhost:3032"
@ -52,8 +87,7 @@ def _get_browserless_client(tool_name: str = "web_fetch") -> BrowserlessClient:
if cfg is not None:
base_url = cfg.get("base_url", base_url)
token = cfg.get("token", token)
raw = cfg.get("timeout_s", timeout_s)
timeout_s = float(raw) if not isinstance(raw, float) else raw
timeout_s = _resolve_timeout(cfg, timeout_s)
return BrowserlessClient(base_url=base_url, token=token, timeout_s=timeout_s)

View File

@ -319,6 +319,36 @@ class TestBrowserlessTools:
assert client.token == "env-token"
def _client_with_config(self, cfg: dict):
with patch("deerflow.community.browserless.tools._get_tool_config") as mock_cfg:
mock_cfg.return_value = cfg
with patch.dict("os.environ", {}, clear=True):
return tools._get_browserless_client("web_capture")
async def test_timeout_s_config_key_is_honored(self):
"""The documented `timeout_s` key keeps working (back-compat)."""
assert self._client_with_config({"timeout_s": 45}).timeout_s == 45.0
async def test_timeout_config_key_is_honored(self):
"""`timeout` is accepted too: it is what crawl4ai and jina_ai read.
Copying that spelling across providers previously produced a silently
ignored key and the 30s default.
"""
assert self._client_with_config({"timeout": 45}).timeout_s == 45.0
async def test_timeout_s_wins_when_both_keys_are_present(self):
"""`timeout_s` is this provider's documented key, so it takes precedence."""
assert self._client_with_config({"timeout_s": 45, "timeout": 10}).timeout_s == 45.0
async def test_invalid_timeout_falls_back_to_default(self):
"""A non-numeric timeout must not crash tool construction."""
assert self._client_with_config({"timeout_s": "not-a-number"}).timeout_s == 30.0
async def test_boolean_timeout_falls_back_to_default(self):
"""YAML `timeout_s: off` parses as False; 0.0 would time out every request."""
assert self._client_with_config({"timeout_s": False}).timeout_s == 30.0
@patch("deerflow.community.browserless.tools._get_browserless_client")
async def test_web_fetch_tool_success(self, mock_get_client):
"""web_fetch_tool successfully fetches and extracts content."""

View File

@ -796,7 +796,7 @@ tools:
# use: deerflow.community.browserless.tools:web_capture_tool
# base_url: http://localhost:3032 # Browserless instance URL (Docker: http://browserless:3000)
# # token: $BROWSERLESS_TOKEN # Required for Browserless Cloud; optional for self-hosted
# timeout_s: 30 # Request timeout in seconds
# timeout_s: 30 # Request timeout in seconds (`timeout` also accepted)
# output_format: png # png, jpeg, or webp
# full_page: true # Capture entire page instead of viewport only
# viewport_width: 1280