Skip to content

Add Firecrawl Search tool to the Atomic Forge - #298

Merged
Eigenwise merged 15 commits into
Eigenwise:mainfrom
JuampiHernandez:feat/firecrawl-search-tool
Oct 10, 2026
Merged

Eigenwise merged 15 commits into
Eigenwise:mainfrom
JuampiHernandez:feat/firecrawl-search-tool

Conversation

@JuampiHernandez

@JuampiHernandez JuampiHernandez commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Hey, I'm Juampi from the Firecrawl team, the provider of the search API this tool calls.

Summary

Adds atomic-forge/tools/firecrawl_search, a Forge tool that runs web and news searches through the Firecrawl Search API. The input schema carries queries, search_type (web or news), and max_results_per_query, plus an include_content flag. When include_content is set, each result also comes back with the main content of the page as Markdown in the same call, so an agent can read the pages without a separate fetch and clean-up step.

The implementation follows the same layout as the other Forge tools (hackernews_search, arxiv_search): input schema, output schemas, config, a tool class with an async request per query gathered with return_exceptions=True, and a sync run wrapper. It calls the REST API with aiohttp directly, so it adds no new dependency. Optional config fields cover location, time_range (day, week, month, year), and max_content_chars to cap page content length.

How it handles failures:

  • Only a 200 whose body says success: true counts as a successful response. A data that is null, missing, or not a list for the requested source raises instead of returning an empty result. A missing source key is treated as no results, which is what the API returns in that case.
  • Results without a well-formed http(s) URL (a valid host name, checked in its IDNA-encoded form, an IPv4 or IPv6 address, a valid port, no whitespace or control characters, and no backslash or extra @ before the path) are skipped, and badly typed optional fields are left empty, so one odd result does not discard the rest.
  • If no query returns results and at least one failed, run_async raises, so a wrong key, exhausted credits, or a malformed response never looks like an empty search. If only some queries fail, the others are returned and the failed ones are listed in failures with the reason.
  • Redirects are not followed.
  • api_key is a SecretStr (falling back to FIRECRAWL_API_KEY) and is only sent in the Authorization header. Provider and transport error text is redacted where it enters the tool, error messages are redacted again after the query is interpolated, and errors are raised from frames that hold neither the response, an aiohttp error nor a rejected body, so the key does not reach reprs, raised errors, error context, traceback locals, logs, or failures. This covers the configured key (8 characters or more) and anything in Firecrawl's fc- key format. Inputs the caller passes in, such as a query, are not altered and appear in captured locals as given.

Changes

  • atomic-forge/tools/firecrawl_search/tool/firecrawl_search.py: the tool
  • atomic-forge/tools/firecrawl_search/tests/test_firecrawl_search.py: 159 tests, most of them against a local aiohttp server (aiohttp.test_utils.TestServer) covering round trips, malformed and over-deep response bodies, status and body edge cases, redirects, batch results, connection, DNS and timeout failures, invalid base URLs, result URL validation, and a dummy key echoed back by the server or contained in a query. An autouse guard keeps the suite offline by blocking DNS lookups and connections to anything but loopback, and the DNS-failure tests only use localhost
  • atomic-forge/tools/firecrawl_search/README.md, pyproject.toml, requirements.txt, .coveragerc: packaging files in the same shape as the other tools
  • atomic-forge/README.md, AGENTS.md, docs/guides/tools.md, README.md: one listing line each, between Fía Signals and Hacker News Search
  • uv.lock: registers the new workspace member (additive only)

Verification

  • black --check and flake8 (repo config) pass, and uv lock --locked is clean
  • pytest --cov --cov-branch in the tool directory: 159 passed offline, 100% line and branch coverage of tool/
  • radon cc 6.0.1: every function in tool/firecrawl_search.py is at complexity 5 or below, and every test function at 4 or below
  • mypy --strict reports nothing in the tool or test files beyond the untyped __init_subclass__ inherited from BaseTool
  • Live run of the example against the API for web, news, and web with include_content=True returned the expected fields; a wrong key raises the API's 401 error
  • An AtomicAgent whose output schema is FirecrawlSearchToolInputSchema generated the tool input from a user question, and a second agent answered from the returned results

Notes

  • time_range maps to the API's tbs filter, which applies to web results only, so the tool sends it only for web searches.
  • Defaults are 5 results per query and a 75 second timeout, because a request with include_content fetches every result page.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical source review of 16010089.

The selectable web/news search and same-call Markdown have useful value. Your affiliation is disclosed and the README links are neutral.

One admission blocker: tool/firecrawl_search.py:122 hardcodes origin='atomic-agents', and line138 sends it on every query. test_build_body_defaults explicitly requires it. This is undocumented integration attribution to the vendor. Please remove the field and its test expectation. I withheld candidate execution at this static check.

Native Radon6.0.1 finds production CC8 for _to_item (line102), CC12 for _fetch (line132), and CC10 for run_async (line153). Each exceeds our strict per-function CRAP <6 threshold even at full coverage. Please simplify at meaningful result-validation, request and batch-result boundaries. Keep the useful assertions.

The static review also found risks needing tests: lines148-151 can turn data: null or a non-list source value into empty success, and the plain config key plus verbatim provider error/log text have no secret-redaction boundary. These are source findings, not executed reproductions. Please cover malformed successful responses and dummy-key exclusion from repr/errors/logs using actual aiohttp transport boundaries.

I verified immutable source and measured complexity, but did not run the 24 tests, confirm the claimed 100% coverage, install vendor code or call the API.

The later 7a1bcff0 review confirms the attribution field was removed and all 16 production functions measured CRAP 1 through 5. Its response-body/URL/diagnostic findings were subsequently repaired in source at ab23d310. At 24b09860, the completed source review also credits the malformed-label fixes and the explicitly narrowed caller-input privacy contract; its remaining IDNA finding is unexecuted. No test pass is claimed for this original head or for the current head here.

@JuampiHernandez

JuampiHernandez commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for the review. All points are addressed in the commits up to 7a1bcff:

  • Removed the origin field from the request body and its expectation in test_build_body_defaults.
  • Split the logic at the request (_build_body, _exchange, _post), result-validation (_failure, _extract_hits, _to_item) and batch-result (_collect) boundaries. radon cc 6.0.1 now reports 5 or below for every function, with 100% line and branch coverage.
  • Malformed successful responses now raise: data: null, a missing data, a non-list source value, success that is anything other than true, and a page where no result has an http(s) URL. Badly typed fields inside an otherwise valid result are left empty instead of failing the query. When no query returns results and one failed, run_async raises; partial failures are listed in a new failures output field with the reason.
  • api_key is a SecretStr. Provider and transport error text is redacted where it enters the tool, transport errors (including invalid base URLs and timeouts) are converted to plain errors with no chained aiohttp exception, the failed response body is dropped before raising, and redirects are not followed.
  • Tests run against a local aiohttp server (aiohttp.test_utils.TestServer) and check that a dummy key echoed back by the server (plain, Bearer-prefixed, upper-cased, nested in JSON) stays out of repr, raised errors, error context, traceback locals, logs and failures.

76 tests pass locally.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical review of 7a1bcff0.

Rechecked 7a1bcff0 with actual aiohttp loopback calls behind an independently reviewed no-egress guard. The request attribution is gone, the links are neutral, and affiliation is disclosed. All 16 production functions have genuine measured native CRAP scores from 1 through 5; the previous complexity blockers are repaired.

Three reproduced defects remain in tool/firecrawl_search.py:

  • Malformed success retains keys in traceback locals, lines217/227/230. A loopback HTTP200 response {success: true, data: null, error: {token: DUMMY_KEY}} raises with the key still in _extract_hits and _fetch frame locals. An all-invalid hit list carrying the key does too. Both the canonical fc- key and a custom configured key reproduce this. Drop or sanitize rejected payloads before raising, including malformed-success paths, and assert the actual exception-frame surfaces.
  • Invalid URLs become result items, lines115/134-141. https://@/a and https://:443/a are accepted without a hostname; whitespace/newline URLs survive too. Validate the actual usable HTTP(S) host and normalize or reject invalid whitespace before returning an item.
  • Query text bypasses diagnostic redaction, lines165-166/242-243. With a query containing the configured dummy key, an unauthorized response puts the exact key into failures[0].error and the warning log. Apply the same protection after interpolating query text.

The independent probes ran 53 cases: 42 passed and 11 failed. Ten failures reproduce those three defects; the eleventh assumed concurrent request arrival order and is a probe mistake, not a product bug. Session/connector closure, cancellation, timeout, redirect refusal and request-option checks passed. Short/encoded/split-key echoes and verbatim success fields remain explicitly characterized privacy limits.

The author suite ran once: 74 passed, 2 failed, exit1. Both failures attempted external-host resolution and were stopped by the guard before traffic; they are not privacy passes. Keep that suite genuinely offline and preserve the real failing outcomes. Production coverage is 142/142 statements and 26/26 branches, with 21 existing example exclusions. Focused Black127 and Flake8 passed.

No live Firecrawl calls, real credentials, installs or candidate source changes occurred.

The ab23d310 source review supersedes these three reproduced findings: malformed/all-invalid response-body retention, the listed hostless/whitespace/newline URLs and diagnostic interpolation are source-repaired. At 24b09860, the completed source review credits the remaining malformed-label fixes and accepts the disclosed caller-input privacy narrowing. Its remaining IDNA/BIDI/encoded-label finding is source-only. The execution counts and coverage above belong to 7a1bcff0; current-head tests remain unverified.

…-or-reason from the request boundary, block non-loopback connects in tests
@JuampiHernandez

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the second pass, and for running it against real loopback calls behind a no-egress guard. That made all three issues easy to reproduce. They are fixed in the commits after 7a1bcff, up to ab23d31:

  • Rejected bodies in traceback locals. Parsing a successful response (_parse) now returns either the items or a reason and never raises, and the request boundary (_exchange) returns either the successful body or a redacted reason. The only raises left are in _post and _fetch, whose frames hold just that reason. JSON that is too deeply nested for the parser is handled the same way. The tests check the captured locals of every tool frame for data: null with an echoed key and for an all-invalid hit list, with both a custom key and an fc- key.
  • Invalid result URLs. A URL is kept only if it is printable, fully matches http(s) with a host that starts with a letter or digit (or an IPv6 literal), has no whitespace, and urlsplit accepts its port and host. https://@/a, https://:443/a, https://[:]/, out-of-range ports, a backslash before the path, and URLs with spaces, tabs, newlines or control characters are skipped. Encoded paths, IDN hosts, IPv6 and ports are still kept.
  • Query text in diagnostics. Error messages are redacted after the query is interpolated, failures[*].query is redacted too, and the warning log reuses the redacted message.

On the other points:

  • The suite is offline. An autouse guard blocks DNS lookups and connections to anything but loopback and fails any test that tries. The two DNS cases are back as their own test: the guard answers the lookup, and the test asserts the ClientConnectorDNSError and the exact host that was blocked.
  • Any is gone. Provider data is typed as object or Dict[str, object], the cast is gone, and request records in the tests use a TypedDict. mypy --strict on the tool now only reports the untyped __init_subclass__ inherited from BaseTool.
  • The tests have no lambdas left, and every test function is at radon CC 4 or below.

122 tests pass locally with 100% line and branch coverage of tool/.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historical source review of ab23d310.

Source-only recheck of ab23d310: the previous malformed/all-invalid response-body traceback retention, concrete hostless/whitespace/newline URL cases and query interpolation into errors/logs are repaired in the source. The typed named responders preserve useful assertions; no tracking or referral attribution has returned. I haven't run this head's tests.

Two remaining source-backed findings in tool/firecrawl_search.py:

  • Credential-bearing queries remain in traceback locals, lines227/259/279. With either the canonical configured key or a custom configured key, queries=[f'why does {KEY} fail'] followed by a 401 Unauthorized response leaves the original query/params/queries in the raising candidate frames. _error protects the message, but diagnostics that capture locals can still expose the key. The new query regression at tests635-647 checks text/failures/logs without the error_surfaces(error) check used by the response-body regression. Cover the same claimed candidate-frame boundary for query inputs, or explicitly narrow the traceback-privacy contract. This is a source-derived finding, not a new-head runtime reproduction.
  • Malformed hostnames still become result URLs, lines119/139-143. https://example..com/a and https://a%.com/a pass the hostname suffix pattern and .port check, then become items. Empty interior DNS labels and invalid host percent escapes contradict the usable-host contract. Validate the hostname's syntax too; no DNS lookup or proof that a domain exists is needed. These are unexecuted source counterexamples.

Native Radon 6.0.1 measures all 18 production bodies at complexity 1 through 5. Current-head coverage, per-function CRAP and style were not verified; the previous 142/142 coverage and author-reported test count do not establish a pass for these changed bytes.

The new fake-DNS tests preserve their assertions. Our existing guard rejects their external hosts at the connector before the fake resolver, so that runner cannot establish their pass unchanged. No candidate execution, remote traffic, real credentials, installation or source edits occurred in this recheck.

Both requests above are superseded at 24b09860: the completed source review accepts the README/_post disclosure of original caller query/parameter retention, and confirms the example..com/a and a%.com/a repairs. The remaining IDNA hostname-syntax finding is source-only and unexecuted. No new runtime or coverage result is claimed.

@JuampiHernandez

Copy link
Copy Markdown
Contributor Author

Thanks again for the careful recheck. Both findings are addressed in the commits up to 24b0986:

  • Credential-bearing queries in traceback locals. I went with your second option and narrowed the contract explicitly. The tool has to send the query as given, and the caller's own frames hold it too, so it can't keep a caller-supplied input out of every frame. What it does guarantee is now written in the README and in the _post docstring: the key never appears in the tool's own state or in anything derived from a response or a transport error. Inputs such as the query appear in captured locals as given. test_key_in_query_text_is_redacted_in_diagnostics now checks that boundary with capture_locals: every tool-frame local except query, queries and params is key-free, alongside the existing message, failure and log checks.
  • Malformed host names. Host names are now validated in one place (_valid_host). The host must be dot-separated, non-empty labels of letters, digits, hyphens and underscores that start and end with a letter or digit, with up to 63 characters per label, 253 in total and one optional trailing dot. A host whose last label is all digits must be a real IPv4 address, and IPv6 literals and ports are left to urlsplit. https://example..com/a, https://a%.com/a, https://a%41b.com/a, labels with a leading or trailing hyphen, 64-character labels, over-long names, 256.0.0.1 and 1.2.3 are skipped. IDN, punycode, underscore, IPv4 and IPv6 hosts are still kept. The URL pattern now only checks the overall shape and keeps @ and backslashes out of the port as well as the userinfo, so a.com:1\@b.com can't be read two ways.

Also:

  • The DNS-failure tests no longer use external host names. They resolve localhost through a test-local getaddrinfo stub that fails, and assert the ClientConnectorDNSError and that the lookup was for localhost, so a connector-level guard that only allows loopback should run them unchanged.
  • All test functions, fixtures and helpers are typed. The request helpers take explicit parameters, and mypy --strict on the tests reports nothing in the tool or test files.

145 tests pass locally on Python 3.12 and 3.13 with 100% line and branch coverage of tool/.

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked 24b09860. The earlier example..com and a%.com hostname cases are repaired. The README and _post now consistently disclose caller query/parameter retention, so that earlier privacy-contract finding is resolved too. The response-body and diagnostic-message protections remain in the source.

One URL-validation gap remains at tool/firecrawl_search.py:125-154: the Unicode hostname check counts characters before IDNA encoding. It accepts https://aא.com/a and a URL whose first label is 63 copies of é. The first violates IDNA's bidirectional-label rules; the second exceeds the 63-byte label limit after encoding. Both reach result items despite the valid-host contract. Please validate IDNA encoding and encoded-label limits before accepting Unicode hostnames, while keeping valid IDNs supported. No DNS lookup is needed.

This finding comes from source inspection, including the installed URL library and Python IDNA rejection paths. I haven't executed these cases or run this head's tests, coverage, type checking or style checks. No remote requests were made.

@JuampiHernandez

Copy link
Copy Markdown
Contributor Author

Thanks for catching this one. Fixed in the commits up to c8bcade.

Host names are now checked in their IDNA (ASCII) form, which is what DNS sees. _valid_host encodes the name first with Python's idna codec (IDNA 2003), and the label, length and IPv4 rules run on the encoded result. The numeric-last-label rule also looks at the Unicode form, so example.١٢٣ is still skipped, and since encoding turns dots such as 。 and . into ., 256.0.0.1 and 1.2。3 are skipped too. The codec rejects labels that are empty or longer than 63 bytes once encoded, and Unicode labels that break the IDNA rules, such as mixing left-to-right and right-to-left scripts. The 253-character limit is applied to the encoded form too.

  • https://aא.com/a, a first label of 63 é, a name that is under 253 characters in Unicode but over 253 once encoded, and a name whose encoded form ends in an empty label (a⒈. encodes to a1..) are now skipped.
  • Valid IDNs are still kept: müller.de, straße.de, Hebrew and Chinese hosts, a 20-character é label, and names that use 。 as a separator or trailing dot. Two runs of real international searches (German, Japanese, Hebrew, Russian, Greek, Chinese) kept all 100 results.
  • The label pattern is now ASCII-only, since it always runs on the encoded name.

159 tests pass locally on Python 3.12 and 3.13 with 100% line and branch coverage of tool/.

@Eigenwise Eigenwise left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked c8bcadecd3b399b988947117e104c51cb9563b31. The IDNA fix and its valid-host regression cases passed in one current-head run. The full existing suite returned 157 passed, 2 failed, no skips. Both failures were the localhost DNS-stub cases: our numeric-loopback-only network guard rejected the host before the stub resolver ran. Those two cases remain unverified. The exercised privacy, redirect, timeout, deduplication and result-cap cases passed too. No further URL changes requested.

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked c8bcadecd3b399b988947117e104c51cb9563b31. The IDNA repair and valid international-host regressions passed in the guarded run: 157 tests passed, 2 DNS-stub cases were blocked by our numeric-only network guard, no skips. Those two DNS paths remain unverified; the full guarded suite wasn't green. The exercised privacy, redirect, timeout, batching and result-limit cases passed, and normal repository core/style CI is green. No remaining product defect was found in the reviewed source and exercised cases.

@Eigenwise
Eigenwise merged commit 045afc1 into Eigenwise:main Oct 10, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants