fix(tools): address review feedback on URLReadTool

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>

Author: joaomdmoura

lib/crewai-tools/src/crewai_tools/rag/loaders/pdf_loader.py

@@ -7,7 +7,12 @@
 
 from crewai_tools.rag.base_loader import BaseLoader, LoaderResult
 from crewai_tools.rag.source_content import SourceContent
-from crewai_tools.security.safe_requests import safe_get
+from crewai_tools.security.safe_requests import safe_get_bounded
+
+
+# Remote PDFs are held in memory for the whole extraction, so the download needs
+# a ceiling. Override per call with the ``max_bytes`` kwarg to ``load``.
+DEFAULT_MAX_PDF_BYTES = 50 * 1024 * 1024
 
 
 class PDFLoader(BaseLoader):
@@ -28,17 +33,19 @@ def _fetch_from_url(url: str, kwargs: dict[str, Any]) -> bytes:
 
         The content stays in memory rather than going to a temporary file: the
         whole body has to be buffered either way, and a temp file would need
-        unlinking on every error path to avoid leaving files behind.
+        unlinking on every error path to avoid leaving files behind. Because it
+        is held in memory, the download is capped.
 
         Args:
             url: The URL to download from.
-            kwargs: Optional dict that may contain custom headers.
+            kwargs: Optional dict that may contain custom ``headers`` and a
+                ``max_bytes`` ceiling for the download.
 
         Returns:
             The raw PDF content.
 
         Raises:
-            ValueError: If the download fails.
+            ValueError: If the download fails or exceeds the size ceiling.
         """
         headers = kwargs.get(
             "headers",
@@ -49,9 +56,13 @@ def _fetch_from_url(url: str, kwargs: dict[str, Any]) -> bytes:
         )
 
         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=kwargs.get("max_bytes", DEFAULT_MAX_PDF_BYTES),
+                headers=headers,
+                timeout=30,
+            )
+            return body
         except Exception as e:
             raise ValueError(f"Failed to download PDF from {url}: {e!s}") from e
 

lib/crewai-tools/src/crewai_tools/tools/url_read_tool/url_read_tool.py

@@ -81,10 +81,11 @@ class URLReadToolSchema(BaseModel):
         ),
     )
     start_line: int | None = Field(
-        1, description="Line number to start reading from (1-indexed)"
+        1, ge=1, description="Line number to start reading from (1-indexed)"
     )
     line_count: int | None = Field(
         None,
+        ge=1,
         description="Number of lines to read. If None, reads the entire content",
     )
 
@@ -311,12 +312,16 @@ def _window(text: str, start_line: int, line_count: int | None) -> str:
 
         The whole body has already been fetched by this point, so unlike the
         filesystem equivalent this only trims output -- it saves no transfer.
+
+        The bounds are clamped rather than trusted: the args schema rejects
+        anything below 1 before it gets here, but islice raises on a negative
+        stop index, and this runs outside the caller's error handling.
         """
         if start_line == 1 and line_count is None:
             return text
 
         start_index = max(start_line - 1, 0)
-        stop_index = None if line_count is None else start_index + line_count
+        stop_index = None if line_count is None else start_index + max(line_count, 0)
         selected = list(islice(text.splitlines(keepends=True), start_index, stop_index))
 
         if not selected and start_index > 0:

lib/crewai-tools/tests/rag/test_pdf_loader.py

@@ -1,5 +1,5 @@
 import tempfile
-from unittest.mock import Mock, patch
+from unittest.mock import patch
 
 from crewai_tools.rag.base_loader import LoaderResult
 from crewai_tools.rag.loaders.pdf_loader import PDFLoader
@@ -9,6 +9,11 @@
 
 pymupdf = pytest.importorskip("pymupdf")
 
+# Patched at the loader's seam rather than at requests.get: safe_get_bounded
+# resolves the hostname before issuing a request, which would make these tests
+# depend on DNS for example.com.
+FETCH = "crewai_tools.rag.loaders.pdf_loader.safe_get_bounded"
+
 
 def build_pdf(text: str = "Quarterly revenue was 42") -> bytes:
     """Return the bytes of a one-page PDF containing *text*."""
@@ -20,8 +25,14 @@ def build_pdf(text: str = "Quarterly revenue was 42") -> bytes:
         document.close()
 
 
+def fetch_result(body: bytes, url: str = "https://example.com/report.pdf"):
+    """Build the (body, content_type, final_url) tuple safe_get_bounded returns."""
+    return body, "application/pdf", url
+
+
 class TestPDFLoader:
     def test_load_pdf_from_file(self):
+        """A PDF on disk has its text extracted with page markers."""
         with tempfile.NamedTemporaryFile(suffix=".pdf") as f:
             f.write(build_pdf())
             f.flush()
@@ -35,20 +46,16 @@ def test_load_pdf_from_file(self):
         assert result.metadata["file_type"] == "pdf"
 
     def test_load_pdf_from_url(self):
-        with patch("requests.get") as mock_get:
-            mock_get.return_value = Mock(
-                content=build_pdf("Content from URL"),
-                raise_for_status=Mock(),
-                status_code=200,
-                headers={},
-            )
+        """A PDF fetched from a URL is extracted and attributed to that URL."""
+        with patch(FETCH) as fetch:
+            fetch.return_value = fetch_result(build_pdf("Content from URL"))
             result = PDFLoader().load(SourceContent("https://example.com/report.pdf"))
 
         assert "Content from URL" in result.content
         assert result.source == "https://example.com/report.pdf"
         assert result.metadata["file_name"] == "report.pdf"
 
-        headers = mock_get.call_args[1]["headers"]
+        headers = fetch.call_args.kwargs["headers"]
         assert headers["Accept"] == "application/pdf"
         assert "crewai-tools PDFLoader" in headers["User-Agent"]
 
@@ -59,45 +66,63 @@ def test_load_pdf_from_url_leaves_no_temp_file(self):
         so every PDF ingested from a URL left a file behind.
         """
         with (
-            patch("requests.get") as mock_get,
+            patch(FETCH) as fetch,
             patch("tempfile.NamedTemporaryFile") as mock_tempfile,
         ):
-            mock_get.return_value = Mock(
-                content=build_pdf(),
-                raise_for_status=Mock(),
-                status_code=200,
-                headers={},
-            )
+            fetch.return_value = fetch_result(build_pdf())
             PDFLoader().load(SourceContent("https://example.com/report.pdf"))
 
         mock_tempfile.assert_not_called()
 
+    def test_load_pdf_from_url_is_size_bounded(self):
+        """The download is capped, since the body is held in memory."""
+        with patch(FETCH) as fetch:
+            fetch.return_value = fetch_result(build_pdf())
+            PDFLoader().load(SourceContent("https://example.com/report.pdf"))
+
+        assert fetch.call_args.kwargs["max_bytes"] == 50 * 1024 * 1024
+
+    def test_load_pdf_from_url_accepts_a_custom_size_limit(self):
+        """Callers can lower or raise the ceiling per load."""
+        with patch(FETCH) as fetch:
+            fetch.return_value = fetch_result(build_pdf())
+            PDFLoader().load(
+                SourceContent("https://example.com/report.pdf"), max_bytes=1024
+            )
+
+        assert fetch.call_args.kwargs["max_bytes"] == 1024
+
     def test_load_pdf_from_url_with_custom_headers(self):
+        """Caller-supplied headers replace the loader's defaults."""
         custom_headers = {"Authorization": "Bearer token"}
 
-        with patch("requests.get") as mock_get:
-            mock_get.return_value = Mock(
-                content=build_pdf(),
-                raise_for_status=Mock(),
-                status_code=200,
-                headers={},
-            )
+        with patch(FETCH) as fetch:
+            fetch.return_value = fetch_result(build_pdf())
             PDFLoader().load(
                 SourceContent("https://example.com/report.pdf"), headers=custom_headers
             )
 
-        assert mock_get.call_args[1]["headers"] == custom_headers
+        assert fetch.call_args.kwargs["headers"] == custom_headers
 
     def test_load_pdf_url_download_error(self):
-        with patch("requests.get", side_effect=Exception("Network error")):
+        """A failed download surfaces as a ValueError naming the URL."""
+        with patch(FETCH, side_effect=Exception("Network error")):
             with pytest.raises(ValueError, match="Failed to download PDF"):
                 PDFLoader().load(SourceContent("https://example.com/report.pdf"))
 
+    def test_load_pdf_url_over_size_limit(self):
+        """An oversized body is reported rather than partially parsed."""
+        with patch(FETCH, side_effect=ValueError("exceeds the 1024 byte limit")):
+            with pytest.raises(ValueError, match="Failed to download PDF"):
+                PDFLoader().load(SourceContent("https://example.com/huge.pdf"))
+
     def test_load_pdf_missing_file(self):
+        """A missing local path raises FileNotFoundError, not ValueError."""
         with pytest.raises(FileNotFoundError, match="PDF file not found"):
             PDFLoader().load(SourceContent("/nonexistent/report.pdf"))
 
     def test_load_corrupt_pdf_raises_value_error(self):
+        """Bytes that are not a parseable PDF produce a read error."""
         with tempfile.NamedTemporaryFile(suffix=".pdf") as f:
             f.write(b"%PDF-1.4 not really a pdf")
             f.flush()
@@ -106,6 +131,7 @@ def test_load_corrupt_pdf_raises_value_error(self):
                 PDFLoader().load(SourceContent(f.name))
 
     def test_pdf_with_no_extractable_text(self):
+        """A PDF whose pages hold no text says so instead of returning empty."""
         document = pymupdf.open()
         document.new_page()
         blank = document.tobytes()
@@ -120,6 +146,7 @@ def test_pdf_with_no_extractable_text(self):
         assert "no extractable text" in result.content
 
     def test_pdf_doc_id_is_stable(self):
+        """The same source yields the same doc_id across loads."""
         with tempfile.NamedTemporaryFile(suffix=".pdf") as f:
             f.write(build_pdf())
             f.flush()