feat(tools): add URLReadTool for reading arbitrary URLs - #6834
Conversation
📝 WalkthroughWalkthroughThe pull request adds bounded secure URL retrieval, ChangesURL content access
Sequence Diagram(s)sequenceDiagram
participant Caller
participant URLReadTool
participant safe_get_bounded
participant HTTPResource
participant PyMuPDF
Caller->>URLReadTool: provide URL and optional line window
URLReadTool->>safe_get_bounded: request bounded secure fetch
safe_get_bounded->>HTTPResource: stream validated HTTP GET
HTTPResource-->>safe_get_bounded: response chunks and metadata
safe_get_bounded-->>URLReadTool: body, content type, and final URL
URLReadTool->>PyMuPDF: extract PDF text when content is PDF
PyMuPDF-->>URLReadTool: extracted text
URLReadTool-->>Caller: return text or formatted error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
lib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.py (1)
51-54: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding this download with
safe_get_bounded.
_fetch_from_urlreadsresponse.contentwith no size cap. The body now stays in memory for the whole extraction, so a large remote PDF costs RAM instead of a temp file. This PR addssafe_get_boundedinlib/crewai-tools/src/crewai_tools/security/safe_requests.py, which abandons the stream once the body crosses a limit. Reusing it here gives the loader the same protection the newURLReadToolhas.This is not a regression: the previous temp-file version also read the full body. Treat it as a follow-up if the test churn is unwelcome, because
test_load_pdf_from_urland its siblings mockrequests.getand readcontentfrom the mock.♻️ Sketch
try: - response = safe_get(url, headers=headers, timeout=30) - response.raise_for_status() - return response.content + body, _content_type, _final_url = safe_get_bounded( + url, max_bytes=_MAX_PDF_BYTES, timeout=30, headers=headers + ) + return body except Exception as e: raise ValueError(f"Failed to download PDF from {url}: {e!s}") from e🤖 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 `@lib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.py` around lines 51 - 54, Update _fetch_from_url to use safe_get_bounded instead of safe_get when downloading the PDF, preserving the existing headers, timeout, status validation, and content return behavior while enforcing the shared response-size limit from safe_requests.lib/crewai-tools/tests/rag/test_pdf_loader.py (1)
37-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPatch
safe_getinstead ofrequests.getto remove the DNS dependency.
PDFLoader._fetch_from_urlcallssafe_get, andsafe_getcallsvalidate_urlbefore it issues the request.validate_urlresolves the hostname. These tests therefore need working DNS resolution forexample.com, and they fail in a network-isolated runner for a reason unrelated to the PDF loader. Patchingcrewai_tools.rag.loaders.pdf_loader.safe_getremoves the dependency and stops the tests from asserting onsafe_getinternals.The same change applies to
test_load_pdf_from_url_leaves_no_temp_file(Lines 55-73),test_load_pdf_from_url_with_custom_headers(Lines 75-89), andtest_load_pdf_url_download_error(Lines 91-94).♻️ Proposed change for `test_load_pdf_from_url`
def test_load_pdf_from_url(self): - with patch("requests.get") as mock_get: + with patch( + "crewai_tools.rag.loaders.pdf_loader.safe_get" + ) as mock_get: mock_get.return_value = Mock( content=build_pdf("Content from URL"), raise_for_status=Mock(), status_code=200, headers={}, ) result = PDFLoader().load(SourceContent("https://example.com/report.pdf"))🤖 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 `@lib/crewai-tools/tests/rag/test_pdf_loader.py` around lines 37 - 45, Update the URL-based PDF loader tests—test_load_pdf_from_url, test_load_pdf_from_url_leaves_no_temp_file, test_load_pdf_from_url_with_custom_headers, and test_load_pdf_url_download_error—to patch crewai_tools.rag.loaders.pdf_loader.safe_get instead of requests.get, configuring the mocked safe_get response or exception as needed while preserving each test’s existing assertions.lib/crewai-tools/tests/url_read_tool_test.py (1)
144-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the extension fallback through
runinstead of_resolve_kind.These two tests assert on the private method
_resolve_kind. The same cases are observable throughtool.runwith a patchedsafe_get_bounded, as the surrounding tests do. Behavior-focused tests survive a refactor of the classification internals.Based on coding guidelines: "Write unit tests for new functionality that focus on behavior rather than implementation details."
♻️ Example replacement
-def test_octet_stream_falls_back_to_url_extension(): - tool = URLReadTool() - assert ( - tool._resolve_kind("application/octet-stream", "https://example.com/a/b.pdf") - == "pdf" - ) - assert tool._resolve_kind("", "https://example.com/a/b.csv") == "text" - assert tool._resolve_kind("", "https://example.com/a/b.bin") is None +def test_octet_stream_csv_falls_back_to_url_extension(): + tool = URLReadTool() + with patch(f"{TOOL_MODULE}.safe_get_bounded") as fetch: + fetch.return_value = fetch_result( + b"a,b\n1,2\n", "application/octet-stream", "https://example.com/a/b.csv" + ) + assert tool.run(url="https://example.com/a/b.csv") == "a,b\n1,2\n" + + +def test_octet_stream_unknown_extension_is_rejected(): + tool = URLReadTool() + with patch(f"{TOOL_MODULE}.safe_get_bounded") as fetch: + fetch.return_value = fetch_result( + b"\x00\x01", "application/octet-stream", "https://example.com/a/b.bin" + ) + assert "Unsupported content type" in tool.run(url="https://example.com/a/b.bin")🤖 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 `@lib/crewai-tools/tests/url_read_tool_test.py` around lines 144 - 159, Update test_octet_stream_falls_back_to_url_extension and test_query_string_does_not_break_extension_fallback to exercise the extension fallback through URLReadTool.run rather than the private _resolve_kind method. Patch safe_get_bounded as in the surrounding tests, provide responses for the PDF, CSV, and binary URLs, and assert the observable run results while preserving the existing expected classifications.Source: Coding guidelines
🤖 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 `@lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py`:
- Around line 83-89: Constrain the start_line and line_count fields in the
URLReadTool schema to non-negative values using their Field definitions,
preserving start_line’s existing 1-indexed default and line_count’s optional
None behavior. Ensure invalid negative inputs are rejected during model
validation before _window is reached.
In `@lib/crewai-tools/tool.specs.json`:
- Line 26985: Update the URLReadTool entry in tool.specs.json so
package_dependencies lists the format-specific dependencies pymupdf,
python-docx, and beautifulsoup4 instead of remaining empty.
---
Nitpick comments:
In `@lib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.py`:
- Around line 51-54: Update _fetch_from_url to use safe_get_bounded instead of
safe_get when downloading the PDF, preserving the existing headers, timeout,
status validation, and content return behavior while enforcing the shared
response-size limit from safe_requests.
In `@lib/crewai-tools/tests/rag/test_pdf_loader.py`:
- Around line 37-45: Update the URL-based PDF loader
tests—test_load_pdf_from_url, test_load_pdf_from_url_leaves_no_temp_file,
test_load_pdf_from_url_with_custom_headers, and
test_load_pdf_url_download_error—to patch
crewai_tools.rag.loaders.pdf_loader.safe_get instead of requests.get,
configuring the mocked safe_get response or exception as needed while preserving
each test’s existing assertions.
In `@lib/crewai-tools/tests/url_read_tool_test.py`:
- Around line 144-159: Update test_octet_stream_falls_back_to_url_extension and
test_query_string_does_not_break_extension_fallback to exercise the extension
fallback through URLReadTool.run rather than the private _resolve_kind method.
Patch safe_get_bounded as in the surrounding tests, provide responses for the
PDF, CSV, and binary URLs, and assert the observable run results while
preserving the existing expected classifications.
🪄 Autofix
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 Plus
Run ID: 6ed62d54-65fe-4999-a11d-85bd5b116fef
📒 Files selected for processing (9)
lib/crewai-tools/src/crewai_tools/__init__.pylib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.pylib/crewai-tools/src/crewai_tools/security/safe_requests.pylib/crewai-tools/src/crewai_tools/tools/__init__.pylib/crewai-tools/src/crewai_tools/tools/url_read_tool/__init__.pylib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.pylib/crewai-tools/tests/rag/test_pdf_loader.pylib/crewai-tools/tests/url_read_tool_test.pylib/crewai-tools/tool.specs.json
There was a problem hiding this comment.
Pull request overview
Adds a new network-egress tool to crewai-tools for fetching arbitrary http(s) URLs and converting common document formats to text, while reusing existing SSRF protections and adding bounded streaming to limit large responses. Also removes a temp-file leak in PDFLoader by switching URL PDF ingestion to in-memory bytes and adds regression tests.
Changes:
- Introduces
URLReadToolto fetch URLs and extract text from PDF/DOCX/HTML and “text-shaped” content types, with optional line windowing. - Adds
safe_get_boundedto stream responses via existingsafe_getvalidation and abort once the decoded body exceedsmax_bytes. - Updates
PDFLoaderURL handling to avoid writing temp files and adds new test coverage; updates tool exports/specs accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/crewai-tools/tool.specs.json | Registers URLReadTool in generated tool specs (init params + run params). |
| lib/crewai-tools/tests/url_read_tool_test.py | Adds unit tests for URLReadTool behavior and safe_get_bounded. |
| lib/crewai-tools/tests/rag/test_pdf_loader.py | Adds regression and behavior tests for PDFLoader (URL + no temp file). |
| lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py | Implements the new URLReadTool including content-type dispatch and extraction. |
| lib/crewai-tools/src/crewai_tools/tools/url_read_tool/init.py | Exposes URLReadTool from its package. |
| lib/crewai-tools/src/crewai_tools/tools/init.py | Exports URLReadTool from the tools module. |
| lib/crewai-tools/src/crewai_tools/security/safe_requests.py | Adds safe_get_bounded bounded streaming helper. |
| lib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.py | Switches URL PDF ingestion to in-memory bytes and ensures pymupdf doc closes in finally. |
| lib/crewai-tools/src/crewai_tools/init.py | Exports URLReadTool at the package root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 26ff36a. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py:342
BaseTool.run()only validatesargs_schemawhen called with keyword args. In_run(),start_line = start_line or 1/line_count = line_count or Nonemeans positional calls liketool.run(url, 0, 0)ortool.run(url, -5, -5)bypass schema validation and get silently coerced (or return an empty window) instead of being rejected as the schema/doc/tests imply. Add explicit validation/coercion here so invalid values are refused regardless of how the tool is called.
"""Fetch a URL and return its content, or a window of it, as text."""
start_line = start_line or 1
line_count = line_count or None
lib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.py:65
kwargs.get("max_bytes", DEFAULT_MAX_PDF_BYTES)will pass through an explicitmax_bytes=None(or other non-int) from a caller, which then causes aTypeErrorinsidesafe_get_boundedwhen comparingtotal > max_bytes. Normalize/validatemax_bytesbefore callingsafe_get_boundedsoNonereliably falls back to the default ceiling.
body, _content_type, _final_url = safe_get_bounded(
url,
max_bytes=kwargs.get("max_bytes", DEFAULT_MAX_PDF_BYTES),
headers=headers,
timeout=30,
)
|
Review feedback addressed across two commits — Fixed (6):
Test changes (nits): loader tests now patch the loader's own seam instead of One declined: Two notes on the majors, since both changed my mental model of the fix:
Still open, by design: neither DNS rebinding nor magic-byte content sniffing is in this PR. Both are noted in the description — the first changes behavior for every Verification: 236 tests pass across |
FileReadTool is confined to the local filesystem, so there was no way for an agent to read a document that lives behind an http(s) URL. Rather than adding a flag to FileReadTool, this adds a separate tool: granting it grants network egress to addresses an LLM picks at runtime, and that should be a deliberate choice rather than a toggle on a filesystem tool. URLReadTool fetches a URL and returns its content as text. PDF and DOCX bodies have their text extracted, HTML is stripped to visible text, and text-shaped types (plain text, Markdown, JSON, XML, YAML, CSV) are decoded using the charset the server declares. Any other content type is refused rather than returned as base64, keeping the output text-only. Requests reuse the existing SSRF protections in security/safe_requests: validate_url resolves every hostname and rejects private, loopback, link-local and reserved addresses (covering cloud metadata endpoints), and safe_get never auto-follows redirects, revalidating each hop and dropping credentials on cross-origin ones. Resolving before validating also normalizes encoded forms, so http://2130706433/ is rejected as 127.0.0.1 without needing a string blocklist. Adds safe_get_bounded on top of that, which streams the body and abandons it once it crosses max_bytes. The cap counts decoded bytes, which is what a compressed response expands into -- Content-Length describes the wire size and cannot bound that. It also closes the redirect hops, which stream=True would otherwise leave holding their connections. Two risks are documented rather than closed. Validation resolves the hostname and requests resolves it again to connect, so DNS rebinding remains possible; closing it needs the connection pinned to the validated address, which would change behavior for all existing safe_get callers. And the returned text is untrusted remote content entering an agent's context, which input validation cannot address. Also fixes a temp file leak in PDFLoader, which reached the same pymupdf-from-URL path. It wrote downloads to NamedTemporaryFile with delete=False and never unlinked them, so every PDF ingested from a URL left a file behind. It now opens from memory, the way URLReadTool does, which removes the leak by construction instead of relying on cleanup on each error path; its doc.close() also moves into a finally so a failure mid-extraction still releases the handle. PDFLoader had no test file, so this adds one covering both paths plus a regression test asserting no temp file is created. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bound start_line and line_count with ge=1 in the args schema. Both were unbounded, and _window computes stop as start + line_count, so a negative line_count reached islice, which rejects a negative stop. The windowing runs outside the tool's error handling, so that escaped _run as a raw ValueError instead of an error string. BaseTool.run validates kwargs against args_schema, so the constraint refuses the value before any request is made. _window also clamps its own bounds now, so it cannot raise if called directly. Bound the PDFLoader download with safe_get_bounded. The body is held in memory for the whole extraction, so it needed a ceiling; it defaults to 50 MiB and takes a max_bytes kwarg to load() for callers ingesting larger documents. Patch the loader's own seam in its tests rather than requests.get. Both safe_get and safe_get_bounded resolve the hostname before requesting, so the previous mocks made the tests depend on DNS for example.com and fail in a network-isolated runner for reasons unrelated to the loader. Exercise the content-type fallback through run() rather than asserting on _resolve_kind, so the tests survive a refactor of the classification internals, and cover the octet-stream PDF, missing-type, query-string and unknown-extension cases as observable behavior. Add docstrings to the new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four findings from the Copilot and Cursor reviews. safe_get leaked its accumulated hops on every failure path. It closed the response it was about to abandon but not the ones already in history, and a caller handed an exception has no handle on them -- under stream=True each holds its connection until its body is read or closed. The loop now closes history before re-raising. Hops are still the caller's on success, where they arrive via response.history. safe_get_bounded rejected a non-positive max_bytes only after issuing the request, and then reported it as an oversized body. It now fails before the request. Its oversized-body error also named the requested URL rather than the one that served the body, which differ after a redirect. The content-type fallback consulted only the final URL for an extension, so a .pdf link redirecting to an extensionless CDN or presigned path was refused even though the requested URL identified the type. It now checks the final URL first, then the requested one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
97c86a3 to
fa38898
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py:346
start_line = start_line or 1/line_count = line_count or Nonesilently normalizes falsy values (e.g., 0) instead of rejecting them. BecauseBaseTool.runskipsargs_schemavalidation when positional args are used, callers can bypass thege=1constraint and get surprising behavior (e.g.,line_count=0reads the whole content). Add explicit runtime validation here and only default when the value is actuallyNone.
start_line = start_line or 1
line_count = line_count or None
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py:347
start_line = start_line or 1/line_count = line_count or Noneconflates0withNoneand doesn’t guard negative values (e.g.start_line=-5stays negative). BecauseBaseTool.runonly validatesargs_schemawhen called with keyword args, positional callers can bypass thege=1constraints and get surprising behavior (likeline_count=0returning the full content). Consider explicit validation/normalization here so runtime behavior matches the schema constraints even when validation is bypassed.
start_line = start_line or 1
line_count = line_count or None
Why
FileReadToolis confined to the local filesystem, so an agent had no way to read a document that lives behind an http(s) URL.ScrapeWebsiteToolcovers HTML pages, but not "read the content of this PDF/CSV/JSON."This adds a separate tool rather than a flag on
FileReadTool. Granting it grants network egress to addresses an LLM chooses at runtime; that should be a deliberate choice, not a toggle on a filesystem tool. Buryingrequests.getinsidefile_read_tool.pywould also silently invalidate anyone's audit of "FileReadTool is sandboxed tobase_dir."What
URLReadTool— fetch a URL, return its content as text.application/pdftext/html,application/xhtml+xml+json/+xml/+yamlSupports
start_line/line_countwindowing, andmax_bytes,timeout,headers,encodingat construction.safe_get_boundedinsecurity/safe_requests.py— streams the body and abandons it once it crossesmax_bytes. The cap counts decoded bytes, sinceContent-Lengthdescribes the wire size and can't bound what a compressed response expands into. Also closes the redirect hops, whichstream=Truewould otherwise leave holding their connections.Security
Reuses the SSRF protections already in this repo rather than adding a parallel path:
validate_urlresolves every hostname and rejects private, loopback, link-local and reserved addresses — cloud metadata endpoints included.safe_getnever auto-follows redirects: each hop is revalidated and credentials are dropped on cross-origin hops.Verified live:
Two risks are documented in the docstring, not closed:
HTTPAdapter— that would change behavior for all 14 existingsafe_getcallers, so it belongs in its own PR.Drive-by fix: temp file leak in
PDFLoaderrag/loaders/pdf_loader.pyreached the same pymupdf-from-URL path and wrote downloads toNamedTemporaryFile(delete=False)without ever unlinking them — every PDF ingested from a URL left a file behind. (DOCXLoaderdoes this correctly.)It now opens from memory the way
URLReadTooldoes, removing the leak by construction rather than relying on cleanup along each error path. Itsdoc.close()also moves into afinally, so a failure mid-extraction still releases the handle. The old code already buffered the whole body intoresponse.content, so there's no memory regression.PDFLoaderhad no test file; this adds one.Testing
tests/url_read_tool_test.py— 23 tests: charset handling, line windowing, content-type dispatch and refusal, extension fallback, validation/request failures, header merging, a real PDF round-trip built with pymupdf, and the bounded-fetch helper (size cap, early abandon, hop closing,stream=True).tests/rag/test_pdf_loader.py— 9 tests over real PDF bytes, includingtest_load_pdf_from_url_leaves_no_temp_fileas the regression guard.tests/rag/,tests/url_read_tool_test.py,tests/utilities/; plusfile_read_tool,test_generate_tool_specs,test_optional_dependenciesgreen.ruff check,ruff format --check,mypyclean.tool.specs.jsonis included (generated bygenerate_tool_specs.py, which CI would regenerate anyway) so the PR is self-contained. The spec exposesurl/start_line/line_countas run params andmax_bytes/timeout/headers/encodingas init params, with no required env vars.Known gap worth a follow-up
Presigned URLs from integration layers (Gmail attachments, S3/R2 fetch endpoints) often serve
application/octet-streamwith an opaque hash for a path — no content type and no extension, so both fallbacks miss and the read is refused. Content sniffing on magic bytes (%PDF-,PK\x03\x04, UTF-8 decodability) would fix that class of URL. Happy to add here or separately.🤖 Generated with Claude Code
Note
Medium Risk
Introduces deliberate network egress for LLM-chosen URLs (SSRF mitigations documented but DNS rebinding and prompt injection remain); changes shared safe_get/PDFLoader behavior for all redirect and remote-PDF callers.
Overview
Adds
URLReadToolso agents can fetch http(s) URLs and get text-only output (PDF/DOCX/HTML/JSON/CSV/etc.), with optionalstart_line/line_count, viasafe_get_bounded(SSRF checks, redirect revalidation, streamed max_bytes cap).safe_requests: newsafe_get_bounded;safe_getnow closes redirect hops on failure (important withstream=True).PDFLoader: remote PDFs load from in-memory bytes (no leaky temp files), use bounded download (default 50 MiB), anddoc.close()infinally.Exports
URLReadTooland updatestool.specs.json; adds tests for the tool, bounded fetch,safe_getcleanup, and PDF loader.Reviewed by Cursor Bugbot for commit fa38898. Bugbot is set up for automated code reviews on this repo. Configure here.