fix(agent/tools): port Crawler to ToolBase so it can load and run - #16415
Merged
yuzhichang merged 8 commits intoJul 3, 2026
Conversation
dosubot
Bot
added
size:M
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
immuhammadfurqan
force-pushed
the
fix/crawler-toolbase-port
branch
from
June 27, 2026 20:09
a5965d9 to
7a3e430
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit_test/agent/component/test_crawler.py (1)
25-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd one regression test for
_invokeoutput wiring.These tests cover
CrawlerParam, but the PR also fixes the missing_invokepath and removedbe_outputcall. A mocked crawl test should assertCrawler._invoke(url=...)returns content and writesformalized_content, so that regression is actually locked down.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit_test/agent/component/test_crawler.py` around lines 25 - 49, Add a regression test for the Crawler._invoke path so the output wiring is covered, not just CrawlerParam. Create a mocked crawl case that calls Crawler._invoke(url=...) and assert it returns the crawled content while also writing formalized_content through the output mechanism, confirming the removed be_output call is still properly replaced in Crawler._invoke.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent/tools/crawler.py`:
- Around line 31-42: The web_crawler tool metadata is missing required ToolMeta
fields, so update the ToolMeta definition in the crawler tool’s initialization
to include both displayName and displayDescription alongside name, description,
and parameters. Make the values consistent with the existing web_crawler
metadata so any descriptor or UI consumer of ToolMeta sees the full declared
shape.
- Around line 69-72: The crawler tool’s required url handling in the crawl entry
path is silently returning success with an empty result when url is missing,
which masks invalid invocations. Update the url check in the crawler method to
treat missing url as an invocation error by logging the bad call and raising or
returning an error state instead of setting formalized_content to empty and
returning a successful string; keep the fix localized around the url extraction
path in the crawler tool.
- Around line 62-85: The timeout is applied to the synchronous Crawler._invoke
wrapper, which can let the daemon worker keep running and mutate outputs after
ToolBase.invoke has already failed. Move timeout enforcement into the async
crawl path instead, so get_web/async crawl is bounded directly and _invoke only
coordinates cancellation, URL validation, pin_dns_global, and set_output without
wrapping the whole method in `@timeout`.
---
Nitpick comments:
In `@test/unit_test/agent/component/test_crawler.py`:
- Around line 25-49: Add a regression test for the Crawler._invoke path so the
output wiring is covered, not just CrawlerParam. Create a mocked crawl case that
calls Crawler._invoke(url=...) and assert it returns the crawled content while
also writing formalized_content through the output mechanism, confirming the
removed be_output call is still properly replaced in Crawler._invoke.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7c4dc7e-8381-4279-96d7-4a10d64a7749
📒 Files selected for processing (2)
agent/tools/crawler.pytest/unit_test/agent/component/test_crawler.py
Comment on lines
+31
to
+42
| self.meta: ToolMeta = { | ||
| "name": "web_crawler", | ||
| "description": "This tool can be used to crawl a web page and return its content as HTML, Markdown, or the extracted main text.", | ||
| "parameters": { | ||
| "url": { | ||
| "type": "string", | ||
| "description": "The absolute URL (including the http:// or https:// scheme) of the web page to crawl.", | ||
| "default": "{sys.query}", | ||
| "required": True, | ||
| } | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Complete the ToolMeta contract.
ToolMeta requires displayName and displayDescription; this metadata omits both, so descriptor/UI consumers expecting the declared shape can break.
Proposed fix
self.meta: ToolMeta = {
"name": "web_crawler",
+ "displayName": "Web Crawler",
"description": "This tool can be used to crawl a web page and return its content as HTML, Markdown, or the extracted main text.",
+ "displayDescription": "Crawl a web page and return HTML, Markdown, or extracted text.",
"parameters": {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| self.meta: ToolMeta = { | |
| "name": "web_crawler", | |
| "description": "This tool can be used to crawl a web page and return its content as HTML, Markdown, or the extracted main text.", | |
| "parameters": { | |
| "url": { | |
| "type": "string", | |
| "description": "The absolute URL (including the http:// or https:// scheme) of the web page to crawl.", | |
| "default": "{sys.query}", | |
| "required": True, | |
| } | |
| }, | |
| } | |
| self.meta: ToolMeta = { | |
| "name": "web_crawler", | |
| "displayName": "Web Crawler", | |
| "description": "This tool can be used to crawl a web page and return its content as HTML, Markdown, or the extracted main text.", | |
| "displayDescription": "Crawl a web page and return HTML, Markdown, or extracted text.", | |
| "parameters": { | |
| "url": { | |
| "type": "string", | |
| "description": "The absolute URL (including the http:// or https:// scheme) of the web page to crawl.", | |
| "default": "{sys.query}", | |
| "required": True, | |
| } | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/tools/crawler.py` around lines 31 - 42, The web_crawler tool metadata
is missing required ToolMeta fields, so update the ToolMeta definition in the
crawler tool’s initialization to include both displayName and displayDescription
alongside name, description, and parameters. Make the values consistent with the
existing web_crawler metadata so any descriptor or UI consumer of ToolMeta sees
the full declared shape.
Comment on lines
+62
to
+85
| @timeout(int(os.environ.get("COMPONENT_EXEC_TIMEOUT", 10 * 60))) | ||
| def _invoke(self, **kwargs): | ||
| from common.ssrf_guard import assert_url_is_safe, pin_dns_global | ||
|
|
||
| ans = self.get_input() | ||
| ans = " - ".join(ans["content"]) if "content" in ans else "" | ||
| if self.check_if_canceled("Crawler processing"): | ||
| return | ||
|
|
||
| url = kwargs.get("url") | ||
| if not url: | ||
| self.set_output("formalized_content", "") | ||
| return "" | ||
|
|
||
| try: | ||
| _ssrf_hostname, _ssrf_ip = assert_url_is_safe(ans) | ||
| _ssrf_hostname, _ssrf_ip = assert_url_is_safe(url) | ||
| except ValueError: | ||
| return Crawler.be_output("URL not valid") | ||
| msg = "URL not valid" | ||
| self.set_output("_ERROR", msg) | ||
| return msg | ||
|
|
||
| try: | ||
| # pin_dns_global is used (not thread-local) because crawl4ai resolves | ||
| # DNS in asyncio executor threads that don't share thread-local state. | ||
| with pin_dns_global(_ssrf_hostname, _ssrf_ip): | ||
| result = asyncio.run(self.get_web(ans)) | ||
| result = asyncio.run(self.get_web(url)) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid the threaded timeout wrapper around mutable component output.
timeout() runs this sync _invoke body in a daemon thread; if timeout enforcement is enabled, the worker can continue and call set_output() after ToolBase.invoke() has already returned an error. Prefer timing out the async crawl itself.
Proposed fix
- `@timeout`(int(os.environ.get("COMPONENT_EXEC_TIMEOUT", 10 * 60)))
def _invoke(self, **kwargs):
@@
# pin_dns_global is used (not thread-local) because crawl4ai resolves
# DNS in asyncio executor threads that don't share thread-local state.
with pin_dns_global(_ssrf_hostname, _ssrf_ip):
- result = asyncio.run(self.get_web(url))
+ result = asyncio.run(
+ asyncio.wait_for(
+ self.get_web(url),
+ timeout=int(os.environ.get("COMPONENT_EXEC_TIMEOUT", 10 * 60)),
+ )
+ )-from common.connection_utils import timeout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @timeout(int(os.environ.get("COMPONENT_EXEC_TIMEOUT", 10 * 60))) | |
| def _invoke(self, **kwargs): | |
| from common.ssrf_guard import assert_url_is_safe, pin_dns_global | |
| ans = self.get_input() | |
| ans = " - ".join(ans["content"]) if "content" in ans else "" | |
| if self.check_if_canceled("Crawler processing"): | |
| return | |
| url = kwargs.get("url") | |
| if not url: | |
| self.set_output("formalized_content", "") | |
| return "" | |
| try: | |
| _ssrf_hostname, _ssrf_ip = assert_url_is_safe(ans) | |
| _ssrf_hostname, _ssrf_ip = assert_url_is_safe(url) | |
| except ValueError: | |
| return Crawler.be_output("URL not valid") | |
| msg = "URL not valid" | |
| self.set_output("_ERROR", msg) | |
| return msg | |
| try: | |
| # pin_dns_global is used (not thread-local) because crawl4ai resolves | |
| # DNS in asyncio executor threads that don't share thread-local state. | |
| with pin_dns_global(_ssrf_hostname, _ssrf_ip): | |
| result = asyncio.run(self.get_web(ans)) | |
| result = asyncio.run(self.get_web(url)) | |
| def _invoke(self, **kwargs): | |
| from common.ssrf_guard import assert_url_is_safe, pin_dns_global | |
| if self.check_if_canceled("Crawler processing"): | |
| return | |
| url = kwargs.get("url") | |
| if not url: | |
| self.set_output("formalized_content", "") | |
| return "" | |
| try: | |
| _ssrf_hostname, _ssrf_ip = assert_url_is_safe(url) | |
| except ValueError: | |
| msg = "URL not valid" | |
| self.set_output("_ERROR", msg) | |
| return msg | |
| try: | |
| # pin_dns_global is used (not thread-local) because crawl4ai resolves | |
| # DNS in asyncio executor threads that don't share thread-local state. | |
| with pin_dns_global(_ssrf_hostname, _ssrf_ip): | |
| result = asyncio.run( | |
| asyncio.wait_for( | |
| self.get_web(url), | |
| timeout=int(os.environ.get("COMPONENT_EXEC_TIMEOUT", 10 * 60)), | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/tools/crawler.py` around lines 62 - 85, The timeout is applied to the
synchronous Crawler._invoke wrapper, which can let the daemon worker keep
running and mutate outputs after ToolBase.invoke has already failed. Move
timeout enforcement into the async crawl path instead, so get_web/async crawl is
bounded directly and _invoke only coordinates cancellation, URL validation,
pin_dns_global, and set_output without wrapping the whole method in `@timeout`.
Comment on lines
+69
to
+72
| url = kwargs.get("url") | ||
| if not url: | ||
| self.set_output("formalized_content", "") | ||
| return "" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Treat a missing required url as an invocation error.
Line 39 marks url as required, but this path returns a successful empty result. That masks malformed tool calls and makes the agent think crawling succeeded.
Proposed fix
url = kwargs.get("url")
if not url:
- self.set_output("formalized_content", "")
- return ""
+ msg = "URL is required"
+ logging.warning("Crawler invoked without required url")
+ self.set_output("_ERROR", msg)
+ return msgAs per coding guidelines, **/*.py: Add logging for new flows.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| url = kwargs.get("url") | |
| if not url: | |
| self.set_output("formalized_content", "") | |
| return "" | |
| url = kwargs.get("url") | |
| if not url: | |
| msg = "URL is required" | |
| logging.warning("Crawler invoked without required url") | |
| self.set_output("_ERROR", msg) | |
| return msg |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agent/tools/crawler.py` around lines 69 - 72, The crawler tool’s required url
handling in the crawl entry path is silently returning success with an empty
result when url is missing, which masks invalid invocations. Update the url
check in the crawler method to treat missing url as an invocation error by
logging the bad call and raising or returning an error state instead of setting
formalized_content to empty and returning a successful string; keep the fix
localized around the url extraction path in the crawler tool.
Source: Coding guidelines
immuhammadfurqan
force-pushed
the
fix/crawler-toolbase-port
branch
from
June 27, 2026 20:54
7a3e430 to
b04f54c
Compare
yuzhichang
self-requested a review
June 28, 2026 00:44
yuzhichang
reviewed
Jun 28, 2026
yuzhichang
left a comment
yuzhichang
left a comment
Member
There was a problem hiding this comment.
Thanks for porting Crawler onto ToolBase. I re-reviewed the current head and the core load-time/runtime breakage is fixed: the tool now has meta, exposes a callable _invoke, and no longer depends on removed be_output behavior.
I did not find a blocking correctness issue in the implementation itself. The main thing still missing for merge confidence is one direct regression test for the _invoke execution path, not just CrawlerParam. A mocked crawl test that asserts Crawler._invoke(query=...) returns content and writes formalized_content would lock down the exact runtime path this PR is restoring.
yuzhichang
reviewed
Jun 28, 2026
yuzhichang
left a comment
yuzhichang
left a comment
Member
There was a problem hiding this comment.
I re-reviewed the branch locally. The core runtime regression is fixed: the tool now has ToolMeta, a working _invoke entrypoint, and no longer depends on removed be_output behavior. I did not find a merge-blocking implementation bug in the current code.
The remaining gap is test coverage for the restored execution path. Please add one focused regression test that mocks the crawl result, calls Crawler._invoke(query=...), and asserts both the returned content and the formalized_content output. That will lock down the exact runtime path this PR is restoring.
immuhammadfurqan
added a commit
to immuhammadfurqan/ragflow
that referenced
this pull request
Jun 28, 2026
Address review feedback on infiniflow#16415: add direct regression tests for the restored execution path (not just CrawlerParam). Using a mocked crawl result and stubbed SSRF guard: - _invoke(query=...) returns the page content and writes it to formalized_content. - an empty query short-circuits without crawling. - an unsafe URL is rejected ("URL not valid" / _ERROR) before any crawl.
Contributor
Author
|
Thanks @yuzhichang — added direct With a mocked
Full file (7 tests) passes locally. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit_test/agent/component/test_crawler.py`:
- Around line 72-85: The happy-path crawler test currently mocks pin_dns_global
to a no-op but never verifies that _invoke() actually uses it, so add an
assertion in the test around Crawler._invoke to confirm the DNS pinning context
manager is entered when fetching the URL. Keep the existing
ssrf.assert_url_is_safe and get_web setup, and tighten the test by checking the
pin_dns_global mock was called/entered during the successful crawl path so
regressions that skip DNS pinning fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abbf589c-9718-4272-9aad-395cdde28677
📒 Files selected for processing (1)
test/unit_test/agent/component/test_crawler.py
yuzhichang
force-pushed
the
main
branch
2 times, most recently
from
June 29, 2026 01:47
78db4e9 to
f86a0e7
Compare
The Crawler tool was never ported to the modern ToolBase/_invoke
interface during the agent module redesign, leaving it broken three ways:
- CrawlerParam (a ToolParamBase) defined no `meta`, but
ToolParamBase.__init__ reads self.meta["parameters"], so constructing
it raised AttributeError. Because canvas loading instantiates
`<Component>Param()`, any agent containing a Crawler node failed to load.
- It extended ToolBase (whose invoke() dispatches to _invoke) but only
implemented the legacy `_run`, so _invoke fell through to
ComponentBase._invoke -> NotImplementedError.
- `_run` called the removed `be_output`, which no longer exists.
Add a ToolMeta with a required `query` parameter (the URL to crawl,
default {sys.query}) matching the convention used by the other tools
(e.g. ArXiv). Replace `_run`/`be_output` with
`_invoke`/`set_output("formalized_content", ...)`, and keep the existing
SSRF guard. Add regression tests covering param construction, validation,
and the tool descriptor.
Closes infiniflow#16414
Address review feedback on infiniflow#16415: add direct regression tests for the restored execution path (not just CrawlerParam). Using a mocked crawl result and stubbed SSRF guard: - _invoke(query=...) returns the page content and writes it to formalized_content. - an empty query short-circuits without crawling. - an unsafe URL is rejected ("URL not valid" / _ERROR) before any crawl.
yuzhichang
force-pushed
the
fix/crawler-toolbase-port
branch
from
June 29, 2026 05:56
ecf2cc0 to
a5d0db9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit_test/agent/component/test_crawler.py`:
- Around line 72-85: The test for crawler._invoke only verifies the returned
content and misses whether the SSRF safety check actually ran. Tighten the test
by asserting the monkeypatched ssrf.assert_url_is_safe is invoked when
crawler._invoke(query=...) is executed, alongside the existing get_web behavior,
so the success path explicitly covers the SSRF validation step.
- Around line 50-57: The CrawlerParam metadata test only verifies that query is
present and required; tighten test_meta_exposes_query_parameter so it also
asserts the default value for query is {sys.query}. Use CrawlerParam.get_meta()
and the function.parameters structure to confirm the descriptor contract doesn’t
regress if the default is removed or changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e47d85c-c399-4202-8df4-ac044b1d4e74
📒 Files selected for processing (2)
agent/tools/crawler.pytest/unit_test/agent/component/test_crawler.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agent/tools/crawler.py
Comment on lines
+50
to
+57
| def test_meta_exposes_query_parameter(): | ||
| # The tool descriptor must advertise a required `query` parameter (the URL | ||
| # to crawl) so an Agent's LLM can call it. `query` matches the frontend | ||
| # form field and the {sys.query} convention shared by the other tools. | ||
| meta = CrawlerParam().get_meta() | ||
| params = meta["function"]["parameters"] | ||
| assert "query" in params["properties"] | ||
| assert "query" in params["required"] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the {sys.query} default too.
This only checks that query exists and is required. The PR contract also depends on the descriptor defaulting query to {sys.query}, so a regression there would still pass this test.
Suggested test tightening
def test_meta_exposes_query_parameter():
# The tool descriptor must advertise a required `query` parameter (the URL
# to crawl) so an Agent's LLM can call it. `query` matches the frontend
# form field and the {sys.query} convention shared by the other tools.
meta = CrawlerParam().get_meta()
params = meta["function"]["parameters"]
assert "query" in params["properties"]
assert "query" in params["required"]
+ assert params["properties"]["query"]["default"] == "{sys.query}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_meta_exposes_query_parameter(): | |
| # The tool descriptor must advertise a required `query` parameter (the URL | |
| # to crawl) so an Agent's LLM can call it. `query` matches the frontend | |
| # form field and the {sys.query} convention shared by the other tools. | |
| meta = CrawlerParam().get_meta() | |
| params = meta["function"]["parameters"] | |
| assert "query" in params["properties"] | |
| assert "query" in params["required"] | |
| def test_meta_exposes_query_parameter(): | |
| # The tool descriptor must advertise a required `query` parameter (the URL | |
| # to crawl) so an Agent's LLM can call it. `query` matches the frontend | |
| # form field and the {sys.query} convention shared by the other tools. | |
| meta = CrawlerParam().get_meta() | |
| params = meta["function"]["parameters"] | |
| assert "query" in params["properties"] | |
| assert "query" in params["required"] | |
| assert params["properties"]["query"]["default"] == "{sys.query}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit_test/agent/component/test_crawler.py` around lines 50 - 57, The
CrawlerParam metadata test only verifies that query is present and required;
tighten test_meta_exposes_query_parameter so it also asserts the default value
for query is {sys.query}. Use CrawlerParam.get_meta() and the
function.parameters structure to confirm the descriptor contract doesn’t regress
if the default is removed or changed.
Comment on lines
+72
to
+85
| monkeypatch.setattr(ssrf, "assert_url_is_safe", lambda url: ("example.com", "93.184.216.34")) | ||
| monkeypatch.setattr(ssrf, "pin_dns_global", lambda *a, **k: contextlib.nullcontext()) | ||
|
|
||
| crawler, out = _make_tool() | ||
|
|
||
| async def fake_get_web(url): | ||
| return "PAGE CONTENT for " + url | ||
|
|
||
| crawler.get_web = fake_get_web | ||
|
|
||
| result = crawler._invoke(query="http://example.com") | ||
|
|
||
| assert result == "PAGE CONTENT for http://example.com" | ||
| assert out["formalized_content"] == "PAGE CONTENT for http://example.com" |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Prove the success path still runs the SSRF check.
The monkeypatch at Line 72 returns a safe host/IP pair, but this test never asserts _invoke() actually called assert_url_is_safe(). If that check were accidentally removed, the test would still pass as long as get_web() returns content.
Suggested test tightening
- monkeypatch.setattr(ssrf, "assert_url_is_safe", lambda url: ("example.com", "93.184.216.34"))
+ seen = []
+ monkeypatch.setattr(
+ ssrf,
+ "assert_url_is_safe",
+ lambda url: (seen.append(url) or ("example.com", "93.184.216.34")),
+ )
monkeypatch.setattr(ssrf, "pin_dns_global", lambda *a, **k: contextlib.nullcontext())
@@
result = crawler._invoke(query="http://example.com")
assert result == "PAGE CONTENT for http://example.com"
assert out["formalized_content"] == "PAGE CONTENT for http://example.com"
+ assert seen == ["http://example.com"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| monkeypatch.setattr(ssrf, "assert_url_is_safe", lambda url: ("example.com", "93.184.216.34")) | |
| monkeypatch.setattr(ssrf, "pin_dns_global", lambda *a, **k: contextlib.nullcontext()) | |
| crawler, out = _make_tool() | |
| async def fake_get_web(url): | |
| return "PAGE CONTENT for " + url | |
| crawler.get_web = fake_get_web | |
| result = crawler._invoke(query="http://example.com") | |
| assert result == "PAGE CONTENT for http://example.com" | |
| assert out["formalized_content"] == "PAGE CONTENT for http://example.com" | |
| seen = [] | |
| monkeypatch.setattr( | |
| ssrf, | |
| "assert_url_is_safe", | |
| lambda url: (seen.append(url) or ("example.com", "93.184.216.34")), | |
| ) | |
| monkeypatch.setattr(ssrf, "pin_dns_global", lambda *a, **k: contextlib.nullcontext()) | |
| crawler, out = _make_tool() | |
| async def fake_get_web(url): | |
| return "PAGE CONTENT for " + url | |
| crawler.get_web = fake_get_web | |
| result = crawler._invoke(query="http://example.com") | |
| assert result == "PAGE CONTENT for http://example.com" | |
| assert out["formalized_content"] == "PAGE CONTENT for http://example.com" | |
| assert seen == ["http://example.com"] |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 81-81: Do not make http calls without encryption
Context: "http://example.com"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit_test/agent/component/test_crawler.py` around lines 72 - 85, The
test for crawler._invoke only verifies the returned content and misses whether
the SSRF safety check actually ran. Tighten the test by asserting the
monkeypatched ssrf.assert_url_is_safe is invoked when crawler._invoke(query=...)
is executed, alongside the existing get_web behavior, so the success path
explicitly covers the SSRF validation step.
dosubot
Bot
added
size:L
dosubot
Bot
added
size:M
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16415 +/- ##
==========================================
- Coverage 94.56% 93.16% -1.40%
==========================================
Files 10 10
Lines 717 717
Branches 118 118
==========================================
- Hits 678 668 -10
- Misses 25 29 +4
- Partials 14 20 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yuzhichang
pushed a commit
that referenced
this pull request
Jul 3, 2026
xugangqiang
pushed a commit
to xugangqiang/ragflow
that referenced
this pull request
Jul 3, 2026
…finiflow#16415) ### What problem does this PR solve? Closes infiniflow#16414. The **Crawler** agent tool (`agent/tools/crawler.py`) was never ported to the modern `ToolBase`/`_invoke` interface during the agent module redesign, so it was broken in three independent ways: 1. **Crashed on construction.** `CrawlerParam` extends `ToolParamBase`, whose `__init__` reads `self.meta["parameters"]`, but `CrawlerParam` defined no `meta`. Constructing it raised `AttributeError: 'CrawlerParam' object has no attribute 'meta'`. Because `agent/canvas.py` instantiates `component_class(component_name + "Param")()` while loading a canvas, **any agent containing a Crawler node failed to load.** 2. **`_invoke` missing.** It extends `ToolBase` (whose `invoke()` dispatches to `self._invoke`) but only implemented the legacy `_run`, so `_invoke` resolved to `ComponentBase._invoke` → `NotImplementedError`. 3. **`be_output` removed.** `_run` called `Crawler.be_output(...)`, which no longer exists on the base classes. ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) ### Changes - Add a `ToolMeta` to `CrawlerParam` (defined before `super().__init__()`, matching every other ported tool such as `ArXivParam`/`TavilyExtractParam`) advertising a required `query` parameter — the URL to crawl, default `{sys.query}`, consistent with the `{sys.query}` convention shared by the other tools. - Replace the legacy `_run`/`be_output` with `_invoke`/`set_output`, writing the extracted page content to `formalized_content` (errors surfaced via `_ERROR`), consistent with the other tools. - Preserve the existing SSRF guard (`assert_url_is_safe` + `pin_dns_global`). - Add regression tests (`test/unit_test/agent/component/test_crawler.py`) covering param construction, validation, and the tool descriptor. Same class of defect as infiniflow#16329 (DeepL). Backend-only; no frontend changes. --------- Co-authored-by: Zhichang Yu <yuzhichang@gmail.com>
xugangqiang
pushed a commit
to xugangqiang/ragflow
that referenced
this pull request
Jul 3, 2026
xugangqiang
pushed a commit
to xugangqiang/ragflow
that referenced
this pull request
Jul 6, 2026
…finiflow#16415) ### What problem does this PR solve? Closes infiniflow#16414. The **Crawler** agent tool (`agent/tools/crawler.py`) was never ported to the modern `ToolBase`/`_invoke` interface during the agent module redesign, so it was broken in three independent ways: 1. **Crashed on construction.** `CrawlerParam` extends `ToolParamBase`, whose `__init__` reads `self.meta["parameters"]`, but `CrawlerParam` defined no `meta`. Constructing it raised `AttributeError: 'CrawlerParam' object has no attribute 'meta'`. Because `agent/canvas.py` instantiates `component_class(component_name + "Param")()` while loading a canvas, **any agent containing a Crawler node failed to load.** 2. **`_invoke` missing.** It extends `ToolBase` (whose `invoke()` dispatches to `self._invoke`) but only implemented the legacy `_run`, so `_invoke` resolved to `ComponentBase._invoke` → `NotImplementedError`. 3. **`be_output` removed.** `_run` called `Crawler.be_output(...)`, which no longer exists on the base classes. ### Type of change - [x] Bug Fix (non-breaking change which fixes an issue) ### Changes - Add a `ToolMeta` to `CrawlerParam` (defined before `super().__init__()`, matching every other ported tool such as `ArXivParam`/`TavilyExtractParam`) advertising a required `query` parameter — the URL to crawl, default `{sys.query}`, consistent with the `{sys.query}` convention shared by the other tools. - Replace the legacy `_run`/`be_output` with `_invoke`/`set_output`, writing the extracted page content to `formalized_content` (errors surfaced via `_ERROR`), consistent with the other tools. - Preserve the existing SSRF guard (`assert_url_is_safe` + `pin_dns_global`). - Add regression tests (`test/unit_test/agent/component/test_crawler.py`) covering param construction, validation, and the tool descriptor. Same class of defect as infiniflow#16329 (DeepL). Backend-only; no frontend changes. --------- Co-authored-by: Zhichang Yu <yuzhichang@gmail.com>
xugangqiang
pushed a commit
to xugangqiang/ragflow
that referenced
this pull request
Jul 6, 2026
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Closes #16414.
The Crawler agent tool (
agent/tools/crawler.py) was never ported to the modernToolBase/_invokeinterface during the agent module redesign, so it was broken in three independent ways:CrawlerParamextendsToolParamBase, whose__init__readsself.meta["parameters"], butCrawlerParamdefined nometa. Constructing it raisedAttributeError: 'CrawlerParam' object has no attribute 'meta'. Becauseagent/canvas.pyinstantiatescomponent_class(component_name + "Param")()while loading a canvas, any agent containing a Crawler node failed to load._invokemissing. It extendsToolBase(whoseinvoke()dispatches toself._invoke) but only implemented the legacy_run, so_invokeresolved toComponentBase._invoke→NotImplementedError.be_outputremoved._runcalledCrawler.be_output(...), which no longer exists on the base classes.Type of change
Changes
ToolMetatoCrawlerParam(defined beforesuper().__init__(), matching every other ported tool such asArXivParam/TavilyExtractParam) advertising a requiredqueryparameter — the URL to crawl, default{sys.query}, consistent with the{sys.query}convention shared by the other tools._run/be_outputwith_invoke/set_output, writing the extracted page content toformalized_content(errors surfaced via_ERROR), consistent with the other tools.assert_url_is_safe+pin_dns_global).test/unit_test/agent/component/test_crawler.py) covering param construction, validation, and the tool descriptor.Same class of defect as #16329 (DeepL). Backend-only; no frontend changes.