Repository navigation
Add Firecrawl Search tool to the Atomic Forge - #298
Conversation
There was a problem hiding this comment.
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.
|
Thanks for the review. All points are addressed in the commits up to 7a1bcff:
76 tests pass locally. |
… the aiohttp transport
…s, dedupe queries
There was a problem hiding this comment.
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_hitsand_fetchframe locals. An all-invalid hit list carrying the key does too. Both the canonicalfc-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://@/aandhttps://:443/aare 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].errorand 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.
…sult URLs, redact query text in diagnostics
…-or-reason from the request boundary, block non-loopback connects in tests
…s; guard connect_ex in tests
|
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:
On the other points:
122 tests pass locally with 100% line and branch coverage of |
There was a problem hiding this comment.
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._errorprotects the message, but diagnostics that capture locals can still expose the key. The new query regression at tests635-647 checks text/failures/logs without theerror_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/aandhttps://a%.com/apass the hostname suffix pattern and.portcheck, 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.
…nputs; type the tests
…v4; cap host names at 253 characters
|
Thanks again for the careful recheck. Both findings are addressed in the commits up to 24b0986:
Also:
145 tests pass locally on Python 3.12 and 3.13 with 100% line and branch coverage of |
Eigenwise
left a comment
There was a problem hiding this comment.
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.
|
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.
159 tests pass locally on Python 3.12 and 3.13 with 100% line and branch coverage of |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
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 carriesqueries,search_type(webornews), andmax_results_per_query, plus aninclude_contentflag. Wheninclude_contentis 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 withreturn_exceptions=True, and a syncrunwrapper. It calls the REST API withaiohttpdirectly, so it adds no new dependency. Optional config fields coverlocation,time_range(day, week, month, year), andmax_content_charsto cap page content length.How it handles failures:
success: truecounts as a successful response. Adatathat 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.@before the path) are skipped, and badly typed optional fields are left empty, so one odd result does not discard the rest.run_asyncraises, 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 infailureswith the reason.api_keyis aSecretStr(falling back toFIRECRAWL_API_KEY) and is only sent in theAuthorizationheader. 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, orfailures. This covers the configured key (8 characters or more) and anything in Firecrawl'sfc-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 toolatomic-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 uselocalhostatomic-forge/tools/firecrawl_search/README.md,pyproject.toml,requirements.txt,.coveragerc: packaging files in the same shape as the other toolsatomic-forge/README.md,AGENTS.md,docs/guides/tools.md,README.md: one listing line each, between Fía Signals and Hacker News Searchuv.lock: registers the new workspace member (additive only)Verification
black --checkandflake8(repo config) pass, anduv lock --lockedis cleanpytest --cov --cov-branchin the tool directory: 159 passed offline, 100% line and branch coverage oftool/radon cc6.0.1: every function intool/firecrawl_search.pyis at complexity 5 or below, and every test function at 4 or belowmypy --strictreports nothing in the tool or test files beyond the untyped__init_subclass__inherited fromBaseToolweb,news, andwebwithinclude_content=Truereturned the expected fields; a wrong key raises the API's 401 errorAtomicAgentwhose output schema isFirecrawlSearchToolInputSchemagenerated the tool input from a user question, and a second agent answered from the returned resultsNotes
time_rangemaps to the API'stbsfilter, which applies to web results only, so the tool sends it only for web searches.include_contentfetches every result page.