feat: add Cloudflare /crawl API client (Phase 1 of Cloudflare migration)#54
feat: add Cloudflare /crawl API client (Phase 1 of Cloudflare migration)#54mfafore22 wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Cloudflare crawl client support, updates crawler configuration and site defaults, rewrites crawling around synchronous Markdown results, simplifies runner execution, and enables postponed type-annotation evaluation. ChangesCloudflare crawl migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant crawl_site
participant start_crawl
participant wait_for_crawl
participant CloudflareAPI
Caller->>crawl_site: crawl_site(account_id, api_token, url)
crawl_site->>start_crawl: start_crawl(...)
start_crawl->>CloudflareAPI: POST crawl request
CloudflareAPI-->>start_crawl: job_id
start_crawl-->>crawl_site: job_id
crawl_site->>wait_for_crawl: wait_for_crawl(job_id)
loop poll until not "running" or timeout
wait_for_crawl->>CloudflareAPI: GET crawl status
CloudflareAPI-->>wait_for_crawl: status/result
end
wait_for_crawl-->>crawl_site: crawl result
crawl_site-->>Caller: crawl result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
👋 Hi @mfafore22, thanks for the pull request! Before this can be merged, please address: Lint (prek), Type Checking (mypy), Type Checking (pyrefly). Details and local reproduction commands are below, with links to the relevant CONTRIBUTING.md section for each. CI Failures❌ prekRun ❌ mypyRun ❌ pyreflyRun Test Coverage Report✅ 87% coverage (required: ≥80%) Coverage meets the project threshold. 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tapio/crawler/client.py`:
- Around line 31-35: The Cloudflare crawl start/status requests in
wait_for_crawl and the HTTP call block around httpx.post/httpx.get currently let
timeout, network, and status exceptions escape, which can abort the polling loop
on transient failures. Add explicit timeout handling and wrap the external calls
in retry logic that catches httpx.TimeoutException, httpx.NetworkError, and
HTTPStatusError, then continues polling until the overall wait window expires.
Use the wait_for_crawl flow and the request/response parsing around
response.raise_for_status() and job_id/status checks as the main points to
update.
- Around line 34-35: The Cloudflare response parsing assumes `data["result"]`
always exists, so unexpected or empty JSON can raise a `KeyError`; update the
parsing in the relevant `tapio/crawler/client.py` request handling paths that
use `response.json()`, `job_id`, and `data["result"]["status"]` to validate the
payload shape first. Use safe access or explicit checks for `result` and
`status`, and raise a clear, domain-specific error when the response schema is
missing or invalid instead of indexing directly.
- Around line 6-93: Add Google-style docstrings to start_crawl, wait_for_crawl,
and crawl_site, since these public functions currently have none. Document each
function with a brief summary plus :param entries for every argument, :return
for the returned job_id/result, and :raises for the HTTP/timeout failures
surfaced by httpx.raise_for_status and TimeoutError in wait_for_crawl. Keep the
docstrings aligned with the existing function behavior and use the function
names to locate the right spots.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba741b06-3281-411b-99f0-3c9df95b0030
📒 Files selected for processing (2)
tapio/config/config_manager.pytapio/crawler/client.py
| def start_crawl( | ||
| account_id: str, | ||
| api_token: str, | ||
| url: str, | ||
| depth: int = 2, | ||
| limit: int = 100, | ||
| render: bool = True, | ||
| source: str = "all", | ||
| ) -> str: | ||
| endpoint = f"https://api.cloudflare.com/client/v4/accounts/{account_id}/browser-rendering/crawl" | ||
|
|
||
| headers = { | ||
| "Authorization": f"Bearer {api_token}", | ||
| "Content-Type": "application/json", | ||
| } | ||
|
|
||
| payload = { | ||
| "url": url, | ||
| "depth": depth, | ||
| "limit": limit, | ||
| "formats": ["markdown"], | ||
| "render": render, | ||
| "source": source, | ||
| } | ||
|
|
||
| response = httpx.post(endpoint, json=payload, headers=headers) | ||
| response.raise_for_status() | ||
|
|
||
| data = response.json() | ||
| job_id = data["result"] | ||
|
|
||
| return job_id | ||
|
|
||
|
|
||
| def wait_for_crawl(account_id: str, job_id: str, api_token: str) -> dict: | ||
| max_attempts = 60 | ||
| delay_seconds = 5 | ||
|
|
||
| for _ in range(max_attempts): | ||
| endpoint = ( | ||
| f"https://api.cloudflare.com/client/v4/accounts/{account_id}" | ||
| f"/browser-rendering/crawl/{job_id}?limit=1" | ||
| ) | ||
|
|
||
| headers = { | ||
| "Authorization": f"Bearer {api_token}", | ||
| } | ||
|
|
||
| response = httpx.get(endpoint, headers=headers) | ||
| response.raise_for_status() | ||
|
|
||
| data = response.json() | ||
| status = data["result"]["status"] | ||
|
|
||
| if status != "running": | ||
| return data["result"] | ||
|
|
||
| time.sleep(delay_seconds) | ||
|
|
||
| raise TimeoutError("Crawl job did not complete within timeout") | ||
|
|
||
|
|
||
| def crawl_site( | ||
| account_id: str, | ||
| api_token: str, | ||
| url: str, | ||
| depth: int = 2, | ||
| limit: int = 100, | ||
| render: bool = True, | ||
| source: str = "all", | ||
| ) -> dict: | ||
| job_id = start_crawl( | ||
| account_id=account_id, | ||
| api_token=api_token, | ||
| url=url, | ||
| depth=depth, | ||
| limit=limit, | ||
| render=render, | ||
| source=source, | ||
| ) | ||
|
|
||
| result = wait_for_crawl( | ||
| account_id=account_id, | ||
| job_id=job_id, | ||
| api_token=api_token, | ||
| ) | ||
|
|
||
| return result No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing docstrings on all three public functions.
start_crawl, wait_for_crawl, and crawl_site have no docstrings at all. Coding guidelines require Google-style docstrings with :param, :return, and :raises documentation for every function.
As per coding guidelines, "Use Google-style docstrings for all Python functions and classes," "Include a docstring summary, parameter documentation, and return-type documentation for Python functions," and "Document Python functions with :param for parameters and :return for return values" / "Document Python exceptions with :raises in docstrings."
📝 Example docstring for start_crawl
def start_crawl(
account_id: str,
api_token: str,
url: str,
depth: int = 2,
limit: int = 100,
render: bool = True,
source: str = "all",
) -> str:
+ """Start a Cloudflare Browser Rendering crawl job.
+
+ :param account_id: Cloudflare account identifier.
+ :param api_token: Cloudflare API token with Browser Rendering edit permission.
+ :param url: Seed URL to crawl.
+ :param depth: Maximum crawl depth.
+ :param limit: Maximum number of pages to crawl.
+ :param render: Whether to render pages via a headless browser.
+ :param source: Crawl source filter (e.g. "all").
+ :return: The Cloudflare-assigned crawl job id.
+ :raises httpx.HTTPStatusError: If the Cloudflare API returns an error status.
+ """
endpoint = f"https://api.cloudflare.com/client/v4/accounts/{account_id}/browser-rendering/crawl"🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 30-30: Request-controlled URL passed to httpx; validate against an allowlist to prevent SSRF.
Context: httpx.post(endpoint, json=payload, headers=headers)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(avoid-ssrf)
[warning] 53-53: Request-controlled URL passed to httpx; validate against an allowlist to prevent SSRF.
Context: httpx.get(endpoint, headers=headers)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(avoid-ssrf)
🤖 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 `@tapio/crawler/client.py` around lines 6 - 93, Add Google-style docstrings to
start_crawl, wait_for_crawl, and crawl_site, since these public functions
currently have none. Document each function with a brief summary plus :param
entries for every argument, :return for the returned job_id/result, and :raises
for the HTTP/timeout failures surfaced by httpx.raise_for_status and
TimeoutError in wait_for_crawl. Keep the docstrings aligned with the existing
function behavior and use the function names to locate the right spots.
Source: Coding guidelines
| response = httpx.post(endpoint, json=payload, headers=headers) | ||
| response.raise_for_status() | ||
|
|
||
| data = response.json() | ||
| job_id = data["result"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No timeout errors handled inside the polling loop; unhandled network exceptions from external calls.
httpx.post/httpx.get here rely on httpx's default 5s timeout (confirmed via httpx docs). Cloudflare crawl start/status responses may legitimately take longer than 5s, and any httpx.TimeoutException, httpx.NetworkError, or HTTPStatusError raised will propagate unhandled — inside wait_for_crawl's loop this means a single transient blip aborts the entire up-to-5-minute polling window instead of being retried, defeating the purpose of the loop.
🔁 Add explicit timeout + retry-on-transient-error handling
- response = httpx.get(endpoint, headers=headers)
- response.raise_for_status()
+ try:
+ response = httpx.get(endpoint, headers=headers, timeout=30.0)
+ response.raise_for_status()
+ except (httpx.TimeoutException, httpx.NetworkError, httpx.HTTPStatusError):
+ time.sleep(delay_seconds)
+ continueAlso applies to: 54-58
🤖 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 `@tapio/crawler/client.py` around lines 31 - 35, The Cloudflare crawl
start/status requests in wait_for_crawl and the HTTP call block around
httpx.post/httpx.get currently let timeout, network, and status exceptions
escape, which can abort the polling loop on transient failures. Add explicit
timeout handling and wrap the external calls in retry logic that catches
httpx.TimeoutException, httpx.NetworkError, and HTTPStatusError, then continues
polling until the overall wait window expires. Use the wait_for_crawl flow and
the request/response parsing around response.raise_for_status() and
job_id/status checks as the main points to update.
| data = response.json() | ||
| job_id = data["result"] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled KeyError if the API response shape differs from expectations.
data["result"] and data["result"]["status"] assume a fixed response shape without validation. If Cloudflare returns an unexpected/empty payload (e.g. a 2xx with a different schema), this raises an unhandled KeyError instead of a meaningful error.
🛡️ Defensive access with clearer error
- data = response.json()
- job_id = data["result"]
+ data = response.json()
+ if "result" not in data:
+ raise RuntimeError(f"Unexpected Cloudflare response: {data}")
+ job_id = data["result"]Also applies to: 57-58
🤖 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 `@tapio/crawler/client.py` around lines 34 - 35, The Cloudflare response
parsing assumes `data["result"]` always exists, so unexpected or empty JSON can
raise a `KeyError`; update the parsing in the relevant `tapio/crawler/client.py`
request handling paths that use `response.json()`, `job_id`, and
`data["result"]["status"]` to validate the payload shape first. Use safe access
or explicit checks for `result` and `status`, and raise a clear, domain-specific
error when the response schema is missing or invalid instead of indexing
directly.
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 `@tapio/config/config_models.py`:
- Around line 49-51: The config model fields have a lint issue from an overly
long annotation line and a formatting typo. Update the field declarations in
config_models.py around max_concurrent, max_depth, and limit so the long
Field(...) definition is wrapped to satisfy ruff E501, and change the stray
"limit :" spacing to match the existing class attribute style. Keep the fix
localized to the config model field definitions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 41638003-f6a6-4769-911d-60737af3ae12
📒 Files selected for processing (2)
tapio/config/config_models.pytapio/config/site_configs.yaml
| max_concurrent: Annotated[int, Field(ge=1, le=50, description="Maximum number of concurrent requests (legacy crawler only)")] = 5 | ||
| max_depth: Annotated[int, Field(ge=1, le=10, description="Maximum crawling depth from starting URLs")] = 1 | ||
| limit : Annotated[ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix line-length lint failure and stray formatting.
Line 49 exceeds 120 chars (ruff E501). Line 51 also has an extraneous space before the colon (limit :), inconsistent with the rest of the file.
🔧 Proposed fix
- max_concurrent: Annotated[int, Field(ge=1, le=50, description="Maximum number of concurrent requests (legacy crawler only)")] = 5
+ max_concurrent: Annotated[
+ int,
+ Field(ge=1, le=50, description="Maximum number of concurrent requests (legacy crawler only)"),
+ ] = 5
max_depth: Annotated[int, Field(ge=1, le=10, description="Maximum crawling depth from starting URLs")] = 1
- limit : Annotated[
+ limit: Annotated[
int,
Field(ge=1, le=100_000, description="Max pages to crawl (Cloudflare)"),
] = 100📝 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.
| max_concurrent: Annotated[int, Field(ge=1, le=50, description="Maximum number of concurrent requests (legacy crawler only)")] = 5 | |
| max_depth: Annotated[int, Field(ge=1, le=10, description="Maximum crawling depth from starting URLs")] = 1 | |
| limit : Annotated[ | |
| max_concurrent: Annotated[ | |
| int, | |
| Field(ge=1, le=50, description="Maximum number of concurrent requests (legacy crawler only)"), | |
| ] = 5 | |
| max_depth: Annotated[int, Field(ge=1, le=10, description="Maximum crawling depth from starting URLs")] = 1 | |
| limit: Annotated[ | |
| int, | |
| Field(ge=1, le=100_000, description="Max pages to crawl (Cloudflare)"), | |
| ] = 100 |
🧰 Tools
🪛 GitHub Check: Lint (prek)
[failure] 49-49: ruff (E501)
tapio/config/config_models.py:49:121: E501 Line too long (133 > 120)
🤖 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 `@tapio/config/config_models.py` around lines 49 - 51, The config model fields
have a lint issue from an overly long annotation line and a formatting typo.
Update the field declarations in config_models.py around max_concurrent,
max_depth, and limit so the long Field(...) definition is wrapped to satisfy
ruff E501, and change the stray "limit :" spacing to match the existing class
attribute style. Keep the fix localized to the config model field definitions.
Source: Linters/SAST tools
mfafore22
left a comment
mfafore22
left a comment
There was a problem hiding this comment.
I tested it on my local it was fine
mfafore22
left a comment
mfafore22
left a comment
There was a problem hiding this comment.
Phase 3 refactored crawler.py
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tapio/crawler/crawler.py (2)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand the new docstrings to match the repository standard.
These one-line docstrings omit the required parameter, return, and applicable exception/example/note documentation for the new class and functions. As per coding guidelines,
**/*.py: “Use Google-style docstrings for all Python functions and classes” and “Document Python functions with:paramfor parameters and:returnfor return values.”Also applies to: 53-54, 61-62, 70-71, 75-76, 90-91, 100-101, 109-110, 124-125, 128-129, 153-154, 162-163, 171-172, 179-180, 188-189, 196-197
🤖 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 `@tapio/crawler/crawler.py` at line 41, The new class and functions in crawler.py use one-line docstrings that do not meet the repository’s Python docstring standard. Update the docstrings for the class and each affected function (including the ones around the listed symbols such as the crawler constructor/methods and helper functions) to Google-style format, adding parameter descriptions with :param, return documentation with :return:, and any applicable notes/examples/exceptions where relevant so the public API is fully documented.Source: Coding guidelines
55-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssign the Cloudflare credentials to typed attributes after validation.
os.getenv()returnsstr | None, so storing it directly onselfleavesself.account_idandself.api_tokenoptional at the latercrawl_site()call site. Validate locals first, then assignself.account_id: strandself.api_token: str.tapio/crawler/crawler.py:55-59🤖 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 `@tapio/crawler/crawler.py` around lines 55 - 59, The Cloudflare credentials are being stored on self directly from os.getenv(), which leaves self.account_id and self.api_token typed as optional later in crawl_site(). Validate the environment values in local variables first, raise the same ValueError if either is missing, then assign the validated values to typed instance attributes in Crawler.__init__ so self.account_id and self.api_token are guaranteed to be str.Source: Coding guidelines
tapio/crawler/runner.py (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand runner docstrings to the required format.
The new class and methods have only summary docstrings;
run()in particular should documentsite_name,site_config, and the returned crawl results. As per coding guidelines,**/*.py: “Use Google-style docstrings for all Python functions and classes” and “Document Python functions with:paramfor parameters and:returnfor return values.”Also applies to: 17-18, 25-26
🤖 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 `@tapio/crawler/runner.py` at line 11, The Runner class and its run() method only have summary docstrings and need to be expanded to the required Google-style format. Update the docstrings on Runner and run() to include :param entries for site_name and site_config, and a :return entry describing the crawl results. Make sure the documentation follows the Python docstring standard used elsewhere in the codebase and covers all newly added class/method docstrings in runner.py.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 `@tapio/crawler/crawler.py`:
- Around line 167-168: The path construction in the crawler still only validates
the top-level output directory, so URL path/query components can escape or
collide within the host subdirectory. Update the filename generation logic in
the crawler method that builds `full_path` to normalize and sanitize the
URL-derived path and query text, and ensure `_assert_within_output_dir` is
applied against the final `output_dir/<host>` target rather than just
`output_dir`. Apply the same host-directory confinement and sanitization
consistently in the other path-building blocks referenced by the crawler code.
- Around line 198-203: The mapping persistence in Crawler.save_url_mappings()
logs the failure but then swallows it, allowing the crawl to report success even
when url_mappings.json was not written. Update the except block to re-raise the
exception after logger.exception so callers can detect the failure and surface
the crawl artifact error.
- Around line 83-88: The URL mappings load in crawler.py is swallowing
decode/read failures and continuing with an empty self.url_mappings, which can
later overwrite valid data on save. Update the mapping load path in the
crawler’s initialization logic to fail fast when Path(self.mapping_file) cannot
be opened or json.load() fails, rather than defaulting to an empty mapping; keep
the existing logger.exception("Error loading URL mappings") and ensure the
crawler does not proceed with a fresh mapping after a bad existing file.
- Around line 104-105: The mapping persistence in crawler.Crawler around
_process_record() and _save_url_mappings() is skipped when a later record
raises, leaving earlier outputs untracked. Refactor the record-processing flow
to avoid a single list comprehension that can abort early, and ensure
_save_url_mappings() is invoked from a finally block (or equivalent cleanup
path) so accumulated URL mappings are always persisted even if one record fails.
- Around line 102-104: The crawl flow in Crawler.crawl is only processing
raw["records"], so it drops any additional Cloudflare pages returned via cursor
pagination. Update _fetch_from_cloudflare and/or crawl to keep requesting
subsequent pages while a cursor is present, merge all records before calling
_filter_completed and _process_record, and ensure the persisted crawl results
include every page from the job, not just the first response.
In `@tapio/crawler/runner.py`:
- Around line 10-25: The CrawlerRunner API was renamed, but the test still calls
the removed async entrypoint. Update the affected call site in
tests/crawler/test_runner.py to use CrawlerRunner.run() instead of run_async(),
or restore a compatible run_async method on CrawlerRunner if async behavior must
remain. Use the CrawlerRunner class and its run method as the main reference
point so the test and implementation stay aligned.
---
Nitpick comments:
In `@tapio/crawler/crawler.py`:
- Line 41: The new class and functions in crawler.py use one-line docstrings
that do not meet the repository’s Python docstring standard. Update the
docstrings for the class and each affected function (including the ones around
the listed symbols such as the crawler constructor/methods and helper functions)
to Google-style format, adding parameter descriptions with :param, return
documentation with :return:, and any applicable notes/examples/exceptions where
relevant so the public API is fully documented.
- Around line 55-59: The Cloudflare credentials are being stored on self
directly from os.getenv(), which leaves self.account_id and self.api_token typed
as optional later in crawl_site(). Validate the environment values in local
variables first, raise the same ValueError if either is missing, then assign the
validated values to typed instance attributes in Crawler.__init__ so
self.account_id and self.api_token are guaranteed to be str.
In `@tapio/crawler/runner.py`:
- Line 11: The Runner class and its run() method only have summary docstrings
and need to be expanded to the required Google-style format. Update the
docstrings on Runner and run() to include :param entries for site_name and
site_config, and a :return entry describing the crawl results. Make sure the
documentation follows the Python docstring standard used elsewhere in the
codebase and covers all newly added class/method docstrings in runner.py.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 56667a2c-7e02-4a6c-bf90-f9288fa2690d
📒 Files selected for processing (2)
tapio/crawler/crawler.pytapio/crawler/runner.py
| try: | ||
| with Path(self.mapping_file).open(encoding="utf-8") as f: | ||
| self.url_mappings = json.load(f) | ||
| logger.info("Loaded %d existing URL mappings", len(self.url_mappings)) | ||
| except Exception: | ||
| logger.exception("Error loading URL mappings") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not overwrite mappings after a failed load.
If url_mappings.json exists but cannot be decoded/read, this logs and continues with an empty mapping; the end-of-crawl save can then replace the previous mappings.
Fail fast on invalid existing mappings
- except Exception:
- logger.exception("Error loading URL mappings")
+ except (OSError, json.JSONDecodeError) as exc:
+ logger.exception("Error loading URL mappings")
+ msg = f"Unable to load existing URL mappings from {self.mapping_file}"
+ raise ValueError(msg) from exc📝 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.
| try: | |
| with Path(self.mapping_file).open(encoding="utf-8") as f: | |
| self.url_mappings = json.load(f) | |
| logger.info("Loaded %d existing URL mappings", len(self.url_mappings)) | |
| except Exception: | |
| logger.exception("Error loading URL mappings") | |
| try: | |
| with Path(self.mapping_file).open(encoding="utf-8") as f: | |
| self.url_mappings = json.load(f) | |
| logger.info("Loaded %d existing URL mappings", len(self.url_mappings)) | |
| except (OSError, json.JSONDecodeError) as exc: | |
| logger.exception("Error loading URL mappings") | |
| msg = f"Unable to load existing URL mappings from {self.mapping_file}" | |
| raise ValueError(msg) from exc |
🤖 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 `@tapio/crawler/crawler.py` around lines 83 - 88, The URL mappings load in
crawler.py is swallowing decode/read failures and continuing with an empty
self.url_mappings, which can later overwrite valid data on save. Update the
mapping load path in the crawler’s initialization logic to fail fast when
Path(self.mapping_file) cannot be opened or json.load() fails, rather than
defaulting to an empty mapping; keep the existing logger.exception("Error
loading URL mappings") and ensure the crawler does not proceed with a fresh
mapping after a bad existing file.
| raw = self._fetch_from_cloudflare() | ||
| records = self._filter_completed(raw.get("records", [])) | ||
| results = [self._process_record(r) for r in records] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether crawl-result pagination is handled anywhere in the crawler client.
rg -n 'limit=1|cursor|raw\.get\("records"|records' tapio/crawlerRepository: Finntegrate/tapio
Length of output: 950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the crawler client and crawler implementation around the relevant calls.
sed -n '1,220p' tapio/crawler/client.py
printf '\n---\n'
sed -n '1,220p' tapio/crawler/crawler.pyRepository: Finntegrate/tapio
Length of output: 10060
🌐 Web query:
Cloudflare browser rendering crawl result limit cursor pagination /browser-rendering/crawl/{job_id} records cursor
💡 Result:
To paginate through results for a Cloudflare Browser Rendering crawl job using the GET /accounts/{account_id}/browser-rendering/crawl/{job_id} endpoint, you can utilize the cursor and limit query parameters [1][2][3]. Key details for pagination: - limit: An optional integer that specifies the maximum number of records to return in a single response [1][3]. - cursor: An optional string or number used to navigate through paginated result sets [1][2][4]. If a crawl response exceeds 10 MB, the API will include a cursor value in the response body [5][3]. You should pass this value as a query parameter in your subsequent request to retrieve the next page of results [3]. Example usage: You can request a specific page of results by appending the cursor and limit parameters to your API call [3]: curl -X GET 'https://api.cloudflare.com/client/v4/accounts/{account_id}/browser-rendering/crawl/{job_id}?cursor={cursor_value}&limit={limit_value}&status=completed' \ -H 'Authorization: Bearer YOUR_API_TOKEN' When a job reaches a terminal status, you can also fetch all results by omitting the limit parameter, though using pagination is recommended for large datasets [3]. You may also filter the returned records by using the status parameter, which accepts values such as queued, errored, completed, disallowed, skipped, or cancelled [2][3].
Citations:
- 1: https://developers.cloudflare.com/api/resources/browser_rendering/subresources/crawl/methods/get/
- 2: https://developers.cloudflare.com/api/resources/browser_rendering/subresources/crawl/
- 3: https://github.com/cloudflare/cloudflare-docs/blob/production/src/content/docs/browser-rendering/rest-api/crawl-endpoint.mdx
- 4: https://developers.cloudflare.com/api/node/resources/browser_rendering/subresources/crawl/methods/get/
- 5: https://developers.cloudflare.com/browser-run/quick-actions/crawl-endpoint/
Follow Cloudflare cursor pagination before persisting crawl results. The /crawl/{job_id} GET path uses limit=1, but Cloudflare paginates crawl records with cursor; crawl() only consumes raw["records"], so later pages are dropped and multi-page crawls are truncated.
🤖 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 `@tapio/crawler/crawler.py` around lines 102 - 104, The crawl flow in
Crawler.crawl is only processing raw["records"], so it drops any additional
Cloudflare pages returned via cursor pagination. Update _fetch_from_cloudflare
and/or crawl to keep requesting subsequent pages while a cursor is present,
merge all records before calling _filter_completed and _process_record, and
ensure the persisted crawl results include every page from the job, not just the
first response.
| results = [self._process_record(r) for r in records] | ||
| self._save_url_mappings() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Persist mappings even if a later record fails.
The list comprehension aborts on the first _process_record() exception, and _save_url_mappings() is skipped, leaving already-written Markdown files without persisted URL mappings.
Save accumulated mappings in a finally block
- results = [self._process_record(r) for r in records]
- self._save_url_mappings()
+ results: list[CrawlResult] = []
+ try:
+ for record in records:
+ results.append(self._process_record(record))
+ finally:
+ self._save_url_mappings()📝 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.
| results = [self._process_record(r) for r in records] | |
| self._save_url_mappings() | |
| results: list[CrawlResult] = [] | |
| try: | |
| for record in records: | |
| results.append(self._process_record(record)) | |
| finally: | |
| self._save_url_mappings() |
🤖 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 `@tapio/crawler/crawler.py` around lines 104 - 105, The mapping persistence in
crawler.Crawler around _process_record() and _save_url_mappings() is skipped
when a later record raises, leaving earlier outputs untracked. Refactor the
record-processing flow to avoid a single list comprehension that can abort
early, and ensure _save_url_mappings() is invoked from a finally block (or
equivalent cleanup path) so accumulated URL mappings are always persisted even
if one record fails.
| full_path = Path(self.output_dir) / parsed.netloc / filename.lstrip("/") | ||
| self._assert_within_output_dir(full_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Constrain generated paths to the per-host directory.
The guard only checks output_dir, so .. segments or path separators from the URL path/query can resolve outside output_dir/<host> while still staying under output_dir, risking cross-page file collisions.
Constrain to the host directory and sanitize query text
+import re
+
@@
- full_path = Path(self.output_dir) / parsed.netloc / filename.lstrip("/")
- self._assert_within_output_dir(full_path)
+ domain_dir = Path(self.output_dir) / parsed.netloc
+ full_path = domain_dir / filename.lstrip("/")
+ self._assert_within_directory(full_path, domain_dir)
return str(full_path)
@@
- safe = query.replace("=", "_").replace("&", "_")
+ safe = re.sub(r"[^A-Za-z0-9._-]+", "_", query).strip("._")
+ if not safe:
+ return filename
@@
- def _assert_within_output_dir(self, path: Path) -> None:
- """Raise if the resolved path escapes the output directory."""
+ def _assert_within_directory(self, path: Path, directory: Path) -> None:
+ """Raise if the resolved path escapes the target directory."""
abs_path = path.resolve()
- abs_out = Path(self.output_dir).resolve()
- if not abs_path.is_relative_to(abs_out):
- msg = f"Invalid URL results in path outside output directory: {path}"
+ abs_directory = directory.resolve()
+ if not abs_path.is_relative_to(abs_directory):
+ msg = f"Invalid URL results in path outside target directory: {path}"
raise ValueError(msg)Also applies to: 179-186, 188-194
🤖 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 `@tapio/crawler/crawler.py` around lines 167 - 168, The path construction in
the crawler still only validates the top-level output directory, so URL
path/query components can escape or collide within the host subdirectory. Update
the filename generation logic in the crawler method that builds `full_path` to
normalize and sanitize the URL-derived path and query text, and ensure
`_assert_within_output_dir` is applied against the final `output_dir/<host>`
target rather than just `output_dir`. Apply the same host-directory confinement
and sanitization consistently in the other path-building blocks referenced by
the crawler code.
| try: | ||
| with Path(self.mapping_file).open("w", encoding="utf-8") as f: | ||
| json.dump(self.url_mappings, f, indent=2, ensure_ascii=False) | ||
| logger.debug( | ||
| "Saved %d URL mappings to %s", | ||
| len(self.url_mappings), | ||
| self.mapping_file, | ||
| ) | ||
| logger.debug("Saved %d URL mappings to %s", len(self.url_mappings), self.mapping_file) | ||
| except Exception: | ||
| logger.exception("Error saving URL mappings") | ||
|
|
||
| def _extract_links(self, soup: BeautifulSoup, base_url: str) -> list[str]: | ||
| """Extract valid links to follow from a BeautifulSoup object. | ||
|
|
||
| Args: | ||
| soup: BeautifulSoup object of the parsed HTML. | ||
| base_url: Base URL for resolving relative links. | ||
|
|
||
| Returns: | ||
| A list of absolute URLs to follow. | ||
| """ | ||
| links = [] | ||
|
|
||
| # Extract all href attributes from anchor tags | ||
| for anchor in soup.find_all("a", href=True): | ||
| # Get href attribute - BeautifulSoup guarantees it exists due to href=True filter | ||
| href = anchor["href"] # type: ignore[index] | ||
|
|
||
| # Skip if href is None or empty | ||
| if not href: | ||
| continue | ||
|
|
||
| # Convert to string (BeautifulSoup can return different types) | ||
| href = str(href) | ||
|
|
||
| # Convert relative URLs to absolute URLs | ||
| absolute_url = urljoin(base_url, href) | ||
|
|
||
| # Filter out non-http(s) schemes and fragments, and check if the domain is allowed | ||
| if ( | ||
| absolute_url.startswith(("http://", "https://")) | ||
| and "#" not in absolute_url | ||
| and self._is_allowed_domain(absolute_url) | ||
| ): | ||
| links.append(absolute_url) | ||
|
|
||
| return links | ||
| logger.exception("Error saving URL mappings") No newline at end of file |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail the crawl when mappings cannot be saved.
The runner will report success even if url_mappings.json was not written. Re-raise after logging so callers can surface the failed crawl artifact.
Propagate mapping persistence failures
- except Exception:
+ except Exception:
logger.exception("Error saving URL mappings")
+ raise📝 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.
| try: | |
| with Path(self.mapping_file).open("w", encoding="utf-8") as f: | |
| json.dump(self.url_mappings, f, indent=2, ensure_ascii=False) | |
| logger.debug( | |
| "Saved %d URL mappings to %s", | |
| len(self.url_mappings), | |
| self.mapping_file, | |
| ) | |
| logger.debug("Saved %d URL mappings to %s", len(self.url_mappings), self.mapping_file) | |
| except Exception: | |
| logger.exception("Error saving URL mappings") | |
| def _extract_links(self, soup: BeautifulSoup, base_url: str) -> list[str]: | |
| """Extract valid links to follow from a BeautifulSoup object. | |
| Args: | |
| soup: BeautifulSoup object of the parsed HTML. | |
| base_url: Base URL for resolving relative links. | |
| Returns: | |
| A list of absolute URLs to follow. | |
| """ | |
| links = [] | |
| # Extract all href attributes from anchor tags | |
| for anchor in soup.find_all("a", href=True): | |
| # Get href attribute - BeautifulSoup guarantees it exists due to href=True filter | |
| href = anchor["href"] # type: ignore[index] | |
| # Skip if href is None or empty | |
| if not href: | |
| continue | |
| # Convert to string (BeautifulSoup can return different types) | |
| href = str(href) | |
| # Convert relative URLs to absolute URLs | |
| absolute_url = urljoin(base_url, href) | |
| # Filter out non-http(s) schemes and fragments, and check if the domain is allowed | |
| if ( | |
| absolute_url.startswith(("http://", "https://")) | |
| and "#" not in absolute_url | |
| and self._is_allowed_domain(absolute_url) | |
| ): | |
| links.append(absolute_url) | |
| return links | |
| logger.exception("Error saving URL mappings") | |
| try: | |
| with Path(self.mapping_file).open("w", encoding="utf-8") as f: | |
| json.dump(self.url_mappings, f, indent=2, ensure_ascii=False) | |
| logger.debug("Saved %d URL mappings to %s", len(self.url_mappings), self.mapping_file) | |
| except Exception: | |
| logger.exception("Error saving URL mappings") | |
| raise |
🤖 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 `@tapio/crawler/crawler.py` around lines 198 - 203, The mapping persistence in
Crawler.save_url_mappings() logs the failure but then swallows it, allowing the
crawl to report success even when url_mappings.json was not written. Update the
except block to re-raise the exception after logger.exception so callers can
detect the failure and surface the crawl artifact error.
mfafore22
left a comment
mfafore22
left a comment
There was a problem hiding this comment.
Cloudlfare is returning 0 results
|
1 participant
Part of #2 — migrating custom crawler/parser to Cloudflare Browser Rendering.
What this PR does
Adds
tapio/crawler/client.pywith three functions for the Cloudflare/crawlAPI:start_crawl(account_id, api_token, url, ...)— POST to initiate a crawl job, returns job_idwait_for_crawl(account_id, job_id, api_token)— polls the job until it completes (max 5 min)crawl_site(...)— convenience wrapper that combines bothAlso fixes a pre-existing issue in
tapio/config/config_manager.pywhere thefrom_filemethod usedConfigManageras a return type before the class was fully defined. Addedfrom __future__ import annotations.Verified working
Tested against real Cloudflare API:
"status":"active"crawl_site("https://example.com")returnsStatus: completedwith recordsRequires
Environment variables:
CLOUDFLARE_ACCOUNT_IDCLOUDFLARE_API_TOKEN(withAccount > Browser Rendering > Editpermission)Not included in this PR (follow-ups)
CrawlerConfigandsite_configs.yamlBaseCrawler/CrawlerRunnerto use this clientI'll open follow-up PRs for each phase to keep review manageable.
Summary by CodeRabbit
limit,render(JavaScript rendering), andsource(all,sitemaps,links).