diff --git a/backend/packages/harness/deerflow/community/ddg_search/tools.py b/backend/packages/harness/deerflow/community/ddg_search/tools.py index 7cbdbe79d0..9b6998ead2 100644 --- a/backend/packages/harness/deerflow/community/ddg_search/tools.py +++ b/backend/packages/harness/deerflow/community/ddg_search/tools.py @@ -7,13 +7,13 @@ import logging from langchain.tools import tool +from deerflow.community.search_max_results import DEFAULT_MAX_RESULTS, coerce_max_results from deerflow.community.search_time_range import DDGS_TIMELIMIT_BY_TIME_RANGE, SearchTimeRange from deerflow.config import get_app_config logger = logging.getLogger(__name__) DEFAULT_BACKEND = "auto" -DEFAULT_MAX_RESULTS = 5 DEFAULT_REGION = "wt-wt" DEFAULT_SAFESEARCH = "moderate" DEFAULT_WIKIPEDIA_REGION = "us-en" @@ -31,22 +31,6 @@ WIKIPEDIA_LANGUAGE_ALIASES = { } -def _coerce_max_results(value: object) -> int: - """Normalize config/parameter values before passing them to DDGS.""" - if isinstance(value, bool) or (isinstance(value, float) and not value.is_integer()): - # int() accepts booleans and silently truncates a YAML value such as 3.5. - count = 0 - else: - try: - count = int(value) # type: ignore[call-overload] - except (TypeError, ValueError, OverflowError): - count = 0 - if count <= 0: - logger.warning("Invalid DDG Search max_results=%r; using default %s", value, DEFAULT_MAX_RESULTS) - return DEFAULT_MAX_RESULTS - return count - - def _normalize_backend(backend: str | list[str] | tuple[str, ...] | None) -> str: if backend is None: return DEFAULT_BACKEND @@ -197,7 +181,7 @@ def web_search_tool( results = _search_text( query=query, - max_results=_coerce_max_results(max_results), + max_results=coerce_max_results(max_results, provider="DDG Search", logger=logger), region=region, safesearch=safesearch, backend=backend, diff --git a/backend/packages/harness/deerflow/community/fastcrw/tools.py b/backend/packages/harness/deerflow/community/fastcrw/tools.py index 8035eaa085..0bc09d8566 100644 --- a/backend/packages/harness/deerflow/community/fastcrw/tools.py +++ b/backend/packages/harness/deerflow/community/fastcrw/tools.py @@ -5,6 +5,7 @@ import os from firecrawl import FirecrawlApp from langchain.tools import tool +from deerflow.community.search_max_results import DEFAULT_MAX_RESULTS, coerce_max_results from deerflow.community.url_safety import validate_public_http_url from deerflow.config import get_app_config @@ -15,23 +16,6 @@ logger = logging.getLogger(__name__) # Firecrawl client and only swaps the base URL. Cloud default points at the managed # service; override `base_url` in the tool config (or set CRW_API_URL) for self-host. DEFAULT_BASE_URL = "https://fastcrw.com/api" -DEFAULT_MAX_RESULTS = 5 - - -def _coerce_max_results(value: object) -> int: - """Normalize the configured ``max_results`` before handing it to the client.""" - if isinstance(value, bool) or (isinstance(value, float) and not value.is_integer()): - # int() accepts booleans and silently truncates a YAML value such as 3.5. - count = 0 - else: - try: - count = int(value) # type: ignore[call-overload] - except (TypeError, ValueError, OverflowError): - count = 0 - if count <= 0: - logger.warning("Invalid fastCRW max_results=%r; using default %s", value, DEFAULT_MAX_RESULTS) - return DEFAULT_MAX_RESULTS - return count def _get_fastcrw_client(tool_name: str = "web_search") -> FirecrawlApp: @@ -78,7 +62,7 @@ def web_search_tool(query: str) -> str: config = get_app_config().get_tool_config("web_search") max_results = DEFAULT_MAX_RESULTS if config is not None: - max_results = _coerce_max_results(config.model_extra.get("max_results", max_results)) + max_results = coerce_max_results(config.model_extra.get("max_results", max_results), provider="fastCRW", logger=logger) client = _get_fastcrw_client("web_search") result = client.search(query, limit=max_results) diff --git a/backend/packages/harness/deerflow/community/firecrawl/tools.py b/backend/packages/harness/deerflow/community/firecrawl/tools.py index 0837119d83..9f741fbfd1 100644 --- a/backend/packages/harness/deerflow/community/firecrawl/tools.py +++ b/backend/packages/harness/deerflow/community/firecrawl/tools.py @@ -5,6 +5,7 @@ import logging from firecrawl import AsyncFirecrawlApp from langchain.tools import tool +from deerflow.community.search_max_results import DEFAULT_MAX_RESULTS, coerce_max_results from deerflow.config import get_app_config logger = logging.getLogger(__name__) @@ -34,25 +35,6 @@ async def _aclose_firecrawl_client(client: AsyncFirecrawlApp) -> None: logger.warning("Failed to close the Firecrawl async HTTP pool", exc_info=True) -DEFAULT_MAX_RESULTS = 5 - - -def _coerce_max_results(value: object) -> int: - """Normalize the configured ``max_results`` before handing it to Firecrawl.""" - if isinstance(value, bool) or (isinstance(value, float) and not value.is_integer()): - # int() accepts booleans and silently truncates a YAML value such as 3.5. - count = 0 - else: - try: - count = int(value) # type: ignore[call-overload] - except (TypeError, ValueError, OverflowError): - count = 0 - if count <= 0: - logger.warning("Invalid Firecrawl max_results=%r; using default %s", value, DEFAULT_MAX_RESULTS) - return DEFAULT_MAX_RESULTS - return count - - def _get_firecrawl_client(tool_name: str = "web_search") -> AsyncFirecrawlApp: config = get_app_config().get_tool_config(tool_name) api_key = None @@ -80,7 +62,7 @@ async def web_search_tool(query: str) -> str: config = get_app_config().get_tool_config("web_search") max_results = DEFAULT_MAX_RESULTS if config is not None: - max_results = _coerce_max_results(config.model_extra.get("max_results", max_results)) + max_results = coerce_max_results(config.model_extra.get("max_results", max_results), provider="Firecrawl", logger=logger) client = _get_firecrawl_client("web_search") result = await client.search(query, limit=max_results) diff --git a/backend/packages/harness/deerflow/community/image_search/tools.py b/backend/packages/harness/deerflow/community/image_search/tools.py index cf9674dbe8..c69a638c9b 100644 --- a/backend/packages/harness/deerflow/community/image_search/tools.py +++ b/backend/packages/harness/deerflow/community/image_search/tools.py @@ -8,32 +8,15 @@ import logging from langchain.tools import tool +from deerflow.community.search_max_results import DEFAULT_MAX_RESULTS, coerce_max_results from deerflow.config import get_app_config logger = logging.getLogger(__name__) -DEFAULT_MAX_RESULTS = 5 - - -def _coerce_max_results(value: object) -> int: - """Normalize config/parameter values before passing them to DDGS.""" - if isinstance(value, bool) or (isinstance(value, float) and not value.is_integer()): - # int() accepts booleans and silently truncates a YAML value such as 3.5. - count = 0 - else: - try: - count = int(value) # type: ignore[call-overload] - except (TypeError, ValueError, OverflowError): - count = 0 - if count <= 0: - logger.warning("Invalid DDG image search max_results=%r; using default %s", value, DEFAULT_MAX_RESULTS) - return DEFAULT_MAX_RESULTS - return count - def _search_images( query: str, - max_results: int = 5, + max_results: int = DEFAULT_MAX_RESULTS, region: str = "wt-wt", safesearch: str = "moderate", size: str | None = None, @@ -96,7 +79,7 @@ def _search_images( @tool("image_search", parse_docstring=True) async def image_search_tool( query: str, - max_results: int = 5, + max_results: int = DEFAULT_MAX_RESULTS, size: str | None = None, color: str | None = None, type_image: str | None = None, @@ -136,7 +119,7 @@ async def image_search_tool( return _search_images( query=query, - max_results=_coerce_max_results(resolved_max_results), + max_results=coerce_max_results(resolved_max_results, provider="DDG image search", logger=logger), size=size, color=color, type_image=type_image, diff --git a/backend/packages/harness/deerflow/community/search_max_results.py b/backend/packages/harness/deerflow/community/search_max_results.py new file mode 100644 index 0000000000..023a499c54 --- /dev/null +++ b/backend/packages/harness/deerflow/community/search_max_results.py @@ -0,0 +1,59 @@ +"""Shared ``max_results`` coercion for the migrated bundled search providers. + +Four providers (``ddg_search``, ``image_search``, ``fastcrw``, ``firecrawl``) +carried byte-identical copies of this coercion, differing only in the provider +label of their warning line. #5865 audited the family value by value against +the #5852 bar; folding the identical copies into one function is what lets the +remaining providers be brought onto the same bar one at a time without +re-deciding in each file what "invalid" means. + +Other providers retain local normalization, including Exa's generic integer +helper and SearXNG's inline validation; Tavily passes configured values through. +``tests/test_search_max_results_single_owner.py`` tracks the known helper and +inline coercion patterns, rather than proving arbitrary provider semantics. + +Deliberately *not* decided here: whether an otherwise-valid value has an upper +bound. Providers still differ on clamping versus passing the configured number +straight to the API. Changing that policy needs a maintainer call. + +``logger`` is a parameter rather than a module-level logger here on purpose: +callers pass their own logger so a warning keeps its originating module in +``record.name`` (``tests/test_ddg_search_tools.py`` pins that). +""" + +from __future__ import annotations + +import logging + +DEFAULT_MAX_RESULTS = 5 + + +def coerce_max_results( + value: object, + *, + provider: str, + logger: logging.Logger, + default: int = DEFAULT_MAX_RESULTS, +) -> int: + """Normalize a configured or parameter ``max_results`` into a positive int. + + Booleans and non-integral floats are rejected rather than coerced: ``bool`` + is an ``int`` subclass, so a bare ``int(value)`` turns ``true`` into ``1``, + and ``int(3.5)`` silently truncates a YAML value the operator wrote wrong. + An out-of-range float such as an unquoted ``max_results: .inf`` makes + ``int()`` raise ``OverflowError``, which is caught here so the tool call + falls back to the documented default instead of failing outright. + + Integral floats such as ``4.0`` stay valid, and no upper bound is applied. + """ + if isinstance(value, bool) or (isinstance(value, float) and not value.is_integer()): + count = 0 + else: + try: + count = int(value) # type: ignore[call-overload] + except (TypeError, ValueError, OverflowError): + count = 0 + if count <= 0: + logger.warning("Invalid %s max_results=%r; using default %s", provider, value, default) + return default + return count diff --git a/backend/tests/AGENTS.md b/backend/tests/AGENTS.md index 56d285c595..78bd5ae7c4 100644 --- a/backend/tests/AGENTS.md +++ b/backend/tests/AGENTS.md @@ -2,6 +2,12 @@ Backend tests must preserve the runtime invariants they exercise without changing production execution topology. +Search coercion ownership tests audit executable helper bodies/call sites and +inline integer assignments. Include Exa and SearXNG in the deferred census; +pin new-provider copies and docstring false positives with source fixtures. +This pattern gate does not establish arbitrary provider semantics or change +deferred providers' validation policy. + Mixed-version resume admission in `test_thread_run_idempotency.py` uses a real shared SQL repository and an old-column projection. The frozen pre-6499 helper models only null-input resume retry matching from `02ce9ab2`; do not update it diff --git a/backend/tests/test_search_max_results_single_owner.py b/backend/tests/test_search_max_results_single_owner.py new file mode 100644 index 0000000000..ff83dfb125 --- /dev/null +++ b/backend/tests/test_search_max_results_single_owner.py @@ -0,0 +1,235 @@ +"""Ownership gate for shared ``max_results`` coercion and deferred patterns. + +#5865 audited every bundled provider's ``max_results`` coercion against the +#5852 rule and found seven copies that diverged on booleans, non-integral +floats and ``OverflowError``. Those audits had to be read file by file because +each provider owned its own copy of the answer. + +``deerflow.community.search_max_results`` owns coercion for the four migrated +providers whose copies were byte-identical. These tests pin three things the +refactor must not silently give back: + +1. the shared function's behaviour, value by value (the bar itself); +2. that a folded provider does not grow a private copy again, and still warns + under its own historical label and its own logger; +3. that the known local coercion patterns match the declared deferred providers. + The AST census recognizes private helpers through their code and call sites, + plus inline integer assignments; it ignores documentation. New copies using + those patterns and deferred providers migrating away both fail. It is a + source-pattern gate, not a semantic proof of arbitrary provider code. +""" + +import ast +import logging +import sys +from pathlib import Path + +import pytest + +from deerflow.community.ddg_search import tools as ddg_search_tools +from deerflow.community.fastcrw import tools as fastcrw_tools +from deerflow.community.firecrawl import tools as firecrawl_tools +from deerflow.community.image_search import tools as image_search_tools +from deerflow.community.search_max_results import DEFAULT_MAX_RESULTS, coerce_max_results + +COMMUNITY_ROOT = Path(ddg_search_tools.__file__).resolve().parent.parent + +# Providers folded onto the shared owner, with the exact provider text their +# warning used before the extraction. The label is kept so existing log-based +# tests (tests/test_fastcrw_tools.py, tests/test_firecrawl_tools.py) stay +# meaningful; the module is imported so a coercer cannot come back at runtime. +SHARED_OWNER_PROVIDERS = { + "ddg_search": ("DDG Search", ddg_search_tools), + "image_search": ("DDG image search", image_search_tools), + "fastcrw": ("fastCRW", fastcrw_tools), + "firecrawl": ("Firecrawl", firecrawl_tools), +} + +# Providers that still normalize locally, including Exa's generic helper and +# SearXNG's inline validation. Their differing bounds or rejection profiles mean +# folding them is a behaviour change +# that #5865's follow-up has to settle first. The equality assertion in +# test_deferral_list_matches_the_providers_that_still_copy is what forces this +# list to shrink as they land -- see PRs #5866 / #5867. +DEFERRED_PROVIDERS = { + "brave", + "exa", + "groundroute", + "searxng", + "serper", + "serply", + "sofya", + "tencent_wsa", +} + + +def _module_ast(provider: str) -> ast.Module: + path = COMMUNITY_ROOT / provider / "tools.py" + return ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + + +def _code_nodes(tree: ast.AST): + """Walk executable syntax, excluding module/class/function docstrings.""" + yield tree + docstring = None + if isinstance(tree, (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)) and ast.get_docstring(tree) is not None: + docstring = tree.body[0] + for child in ast.iter_child_nodes(tree): + if child is not docstring: + yield from _code_nodes(child) + + +def _references_max_results(tree: ast.AST) -> bool: + return any( + (isinstance(node, ast.Name) and node.id == "max_results") or (isinstance(node, ast.Constant) and isinstance(node.value, str) and "max_results" in node.value) or (isinstance(node, ast.keyword) and node.arg == "max_results") + for node in _code_nodes(tree) + ) + + +def _local_coercer_names(tree: ast.Module) -> set[str]: + """Find local helpers and inline integer assignments for ``max_results``.""" + calls = [node for node in _code_nodes(tree) if isinstance(node, ast.Call)] + called_for_max_results = {getattr(call.func, "id", None) for call in calls if _references_max_results(call)} + owned: set[str] = set() + for function in tree.body: + if not isinstance(function, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + if function.name.startswith("_coerce") and ("max_results" in function.name or _references_max_results(function) or function.name in called_for_max_results): + owned.add(function.name) + for node in _code_nodes(function): + if isinstance(node, ast.Assign): + targets = node.targets + elif isinstance(node, ast.AnnAssign): + targets = [node.target] + else: + continue + if node.value is not None and any(_references_max_results(target) for target in targets): + if any(isinstance(call, ast.Call) and getattr(call.func, "id", None) == "int" for call in _code_nodes(node.value)): + owned.add(function.name) + return owned + + +def _shared_calls(tree: ast.Module) -> list[ast.Call]: + """Every call to the shared owner, with its keyword arguments intact.""" + calls = [] + for node in ast.walk(tree): + if isinstance(node, ast.Call) and getattr(node.func, "id", None) == "coerce_max_results": + calls.append(node) + return calls + + +def _imports_shared_owner(tree: ast.Module) -> bool: + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module == "deerflow.community.search_max_results": + return True + return False + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + (5, 5), + ("5", 5), + (4.0, 4), # integral floats are a valid way to write 4 + (True, DEFAULT_MAX_RESULTS), # bool is an int subclass: never 1 + (False, DEFAULT_MAX_RESULTS), + (3.5, DEFAULT_MAX_RESULTS), # never silently truncated to 3 + ("abc", DEFAULT_MAX_RESULTS), + ("", DEFAULT_MAX_RESULTS), + (None, DEFAULT_MAX_RESULTS), + (0, DEFAULT_MAX_RESULTS), + (-2, DEFAULT_MAX_RESULTS), + (float("inf"), DEFAULT_MAX_RESULTS), # int() would raise OverflowError + (10_000, 10_000), # no upper bound: that policy is still per-provider + ], +) +def test_shared_owner_is_the_bar_for_every_value(value, expected, caplog): + with caplog.at_level(logging.WARNING): + assert coerce_max_results(value, provider="Probe", logger=logging.getLogger("probe")) == expected + + +def test_every_folded_provider_delegates_instead_of_copying(): + for provider, (_label, module) in sorted(SHARED_OWNER_PROVIDERS.items()): + tree = _module_ast(provider) + assert not _local_coercer_names(tree), f"{provider} grew a private max_results coercer again" + assert _imports_shared_owner(tree), f"{provider} no longer imports the shared owner" + assert not any(name.startswith("_coerce_max_results") for name in vars(module)), f"{provider} rebound a private coercer" + + +def test_folded_providers_keep_their_historical_warning_label(): + for provider, (label, _module) in sorted(SHARED_OWNER_PROVIDERS.items()): + calls = _shared_calls(_module_ast(provider)) + assert calls, f"{provider} no longer calls the shared owner" + for call in calls: + kwargs = {kw.arg: kw.value for kw in call.keywords} + assert isinstance(kwargs["provider"], ast.Constant), f"{provider}: provider label must stay a literal" + assert kwargs["provider"].value == label, f"{provider}: warning label drifted to {kwargs['provider'].value!r}" + assert getattr(kwargs["logger"], "id", None) == "logger", f"{provider}: must pass its module logger" + + +def test_warnings_stay_on_the_calling_provider_logger(caplog): + """Records must keep module attribution: tests/test_ddg_search_tools.py filters on ``record.name``.""" + provider_logger = logging.getLogger("deerflow.community.example") + with caplog.at_level(logging.WARNING): + coerce_max_results("abc", provider="Example", logger=provider_logger) + assert [record.name for record in caplog.records] == ["deerflow.community.example"] + assert "Invalid Example max_results='abc'; using default 5" in caplog.text + + +def test_deferral_list_matches_the_providers_that_still_copy(): + found = set() + for tools_path in sorted(COMMUNITY_ROOT.glob("*/tools.py")): + provider = tools_path.parent.name + if provider in SHARED_OWNER_PROVIDERS: + continue + if _local_coercer_names(_module_ast(provider)): + found.add(provider) + assert found == DEFERRED_PROVIDERS, f"providers hand-rolling max_results coercion drifted; newly copied: {sorted(found - DEFERRED_PROVIDERS)}, folded but still declared: {sorted(DEFERRED_PROVIDERS - found)}" + + +@pytest.mark.parametrize("provider", ["exa", "searxng"]) +def test_census_includes_generic_and_inline_provider_coercion(provider): + assert _local_coercer_names(_module_ast(provider)), f"{provider}'s existing max_results normalization escaped the census" + + +@pytest.mark.parametrize( + "source", + [ + "def _coerce_max_results(value):\n return int(value)\n", + 'def _coerce_positive_int(value, default, option):\n return int(value)\n\ndef search(config):\n return _coerce_positive_int(config.get("max_results"), 5, "max_results")\n', + 'def _coerce_positive_int(value, *, option):\n return int(value)\n\ndef search(config):\n return _coerce_positive_int(config["max_results"], option="max_results")\n', + ( + "async def search(config):\n" + ' raw = config.get("max_results", 5)\n' + " try:\n" + " max_results = int(raw)\n" + " except ValueError:\n" + ' logger.warning("Invalid Search max_results=%r; using default %s", raw, 5)\n' + " return max_results\n" + ), + "def search(raw):\n max_results: int = int(raw)\n return max_results\n", + ], + ids=["named-helper", "generic-positional", "generic-keyword", "inline-warning", "inline-without-warning"], +) +def test_census_rejects_undeclared_coercion_patterns(tmp_path, monkeypatch, source): + provider_dir = tmp_path / "copycat" + provider_dir.mkdir() + (provider_dir / "tools.py").write_text(source, encoding="utf-8") + monkeypatch.setattr(sys.modules[__name__], "COMMUNITY_ROOT", tmp_path) + monkeypatch.setattr(sys.modules[__name__], "DEFERRED_PROVIDERS", set()) + + with pytest.raises(AssertionError, match="newly copied:.*copycat"): + test_deferral_list_matches_the_providers_that_still_copy() + + +@pytest.mark.parametrize( + "source", + [ + 'def _coerce_timeout(value):\n """max_results is normalized elsewhere."""\n return int(value)\n', + 'def _coerce_timeout(value):\n """max_results"""\n return int(value)\n\ndef search(config):\n return _coerce_timeout(config.get("timeout", 30))\n', + 'def search(config):\n max_results = config.get("max_results", 5)\n return client.search(max_results=max_results)\n', + ], + ids=["docstring-mention", "exact-docstring", "pass-through"], +) +def test_census_ignores_documentation_and_pass_through(source): + assert not _local_coercer_names(ast.parse(source))