Add Phase 2 crawler rendering: manifest-driven, resumable, per-document refresh - #75
Conversation
…nt refresh Replaces crawl's whole-site BFS deep crawl and crawl_state.json with manifest-driven rendering: only due records are rendered, progress is persisted after each URL for resumability, and per-document cache mode follows the spec's refresh policy (initial backfill, trusted-lastmod changes, stale extractor version, and scheduled audits). Artifacts are now keyed by canonical_url with redirect-driven rekeying, and the render browser sends the configured User-Agent instead of spoofing Chrome. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe crawler now separates URL discovery from manifest-driven rendering. It selects due records, applies refresh and retry policies, renders URLs with bounded concurrency, persists documents and failures, and reports coverage and run status. ChangesManifest contracts, storage, and scheduling
Manifest record rendering
Runner, CLI, and site workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MiseTask
participant ManifestStore
participant CrawlerRunner
participant Crawl4AICrawler
MiseTask->>ManifestStore: discover site URLs
MiseTask->>CrawlerRunner: start crawl with limits
CrawlerRunner->>Crawl4AICrawler: render due manifest records
Crawl4AICrawler->>ManifestStore: persist records and documents
CrawlerRunner-->>MiseTask: return RenderRunSummary
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
CI Results
All checks passed. 🎉 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crawler/tapio_crawler/discovery/rate_limiter.py (1)
111-122: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix the unparenthesized
exceptclause inparse_retry_after.
crawler/tapio_crawler/discovery/rate_limiter.pyhas the sameexcept TypeError, ValueError, OverflowError:pattern, so the module cannot compile with Python 3. Use a tuple for the exception types before the colon.🤖 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 `@crawler/tapio_crawler/discovery/rate_limiter.py` around lines 111 - 122, Update the exception handler in parse_retry_after to use a parenthesized tuple for TypeError, ValueError, and OverflowError, preserving the existing fallback behavior while making the module valid Python 3 syntax.
🧹 Nitpick comments (3)
crawler/tests/crawler/test_policy.py (1)
18-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Google-style docstring to
_record.Document the helper summary,
overrides, and returnedManifestRecord. This helper builds the policy test fixture and is part of the test module’s local API.As per coding guidelines, “Use Google-style docstrings for all Python functions and classes, documenting summaries, parameters, return types, exceptions, examples, notes, TODOs, deprecations, references, and warnings where applicable.”
🤖 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 `@crawler/tests/crawler/test_policy.py` around lines 18 - 28, Add a Google-style docstring to the `_record` test helper describing that it builds a policy-test `ManifestRecord` fixture, documenting the `overrides` keyword arguments and the returned `ManifestRecord`.Source: Coding guidelines
crawler/tapio_crawler/config/config_models.py (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
RefreshConfigfields in the class docstring.Add a Google-style
Attributessection forunchanged_audit_days,inactive_grace_cycles, andcoverage_target_percent. This makes the public configuration contract clear.As per coding guidelines, “Use Google-style docstrings for all Python functions and classes, documenting summaries, parameters, return types, exceptions, examples, notes, TODOs, deprecations, references, and warnings where applicable.”
🤖 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 `@crawler/tapio_crawler/config/config_models.py` around lines 65 - 70, Expand the RefreshConfig class docstring with a Google-style Attributes section documenting unchanged_audit_days, inactive_grace_cycles, and coverage_target_percent, including each field’s purpose and expected value semantics. Keep the existing class summary and field definitions unchanged.Source: Coding guidelines
crawler/tests/test_cli.py (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the status assertion.
"complete" in result.stdoutalso matches the stringincomplete. The assertion passes for either run state. Assert the rendered phrase instead.♻️ Proposed change
assert result.exit_code == 0 - assert "complete" in result.stdout + assert "for example: complete." in result.stdout assert "saved 1" in result.stdout🤖 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 `@crawler/tests/test_cli.py` around lines 49 - 53, Update the status assertion in the CLI test to match the complete rendered phrase rather than checking for the substring "complete", while preserving the existing expectations for saved output, warnings, and manifest store closure.
🤖 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 `@crawler/tapio_crawler/crawler/crawler.py`:
- Around line 384-399: Update _next_retry in
crawler/tapio_crawler/crawler/crawler.py:384-399 to park records after
MAX_RETRY_COUNT instead of returning retry_after=None; use a persistent long
retry_after or a terminal state excluded by list_eligible_page/decide_render.
Update crawler/tests/crawler/test_crawler.py:232-243 to perform a second render
pass and assert the parked record is not rendered again.
In `@crawler/tapio_crawler/manifest/store.py`:
- Around line 232-236: Update the redirect/rekey flow around _write and the
manifest DELETE to first load any existing record for new_record.canonical_url,
merge its accumulated fields (including discovery_source and earliest
first_seen_at) with the old record, then delete the old identity and write the
merged record within one transaction. Preserve the target record’s persisted
state when both identities exist, and add a test covering that case.
- Around line 43-47: Update manifest database initialization to inspect PRAGMA
table_info(manifest) and add the missing retry_count column with ALTER TABLE ...
ADD COLUMN retry_count INTEGER NOT NULL DEFAULT 0 before writes occur. Preserve
existing schemas and initialization behavior, and add a regression test that
opens a pre-change manifest schema and successfully saves a record.
---
Outside diff comments:
In `@crawler/tapio_crawler/discovery/rate_limiter.py`:
- Around line 111-122: Update the exception handler in parse_retry_after to use
a parenthesized tuple for TypeError, ValueError, and OverflowError, preserving
the existing fallback behavior while making the module valid Python 3 syntax.
---
Nitpick comments:
In `@crawler/tapio_crawler/config/config_models.py`:
- Around line 65-70: Expand the RefreshConfig class docstring with a
Google-style Attributes section documenting unchanged_audit_days,
inactive_grace_cycles, and coverage_target_percent, including each field’s
purpose and expected value semantics. Keep the existing class summary and field
definitions unchanged.
In `@crawler/tests/crawler/test_policy.py`:
- Around line 18-28: Add a Google-style docstring to the `_record` test helper
describing that it builds a policy-test `ManifestRecord` fixture, documenting
the `overrides` keyword arguments and the returned `ManifestRecord`.
In `@crawler/tests/test_cli.py`:
- Around line 49-53: Update the status assertion in the CLI test to match the
complete rendered phrase rather than checking for the substring "complete",
while preserving the existing expectations for saved output, warnings, and
manifest store closure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 369407ef-4cc8-43fd-888e-0da7bd13ab80
📒 Files selected for processing (20)
README.mdcrawler/README.mdcrawler/tapio_crawler/cli.pycrawler/tapio_crawler/config/config_models.pycrawler/tapio_crawler/config/site_configs.yamlcrawler/tapio_crawler/crawler/crawler.pycrawler/tapio_crawler/crawler/policy.pycrawler/tapio_crawler/crawler/runner.pycrawler/tapio_crawler/discovery/rate_limiter.pycrawler/tapio_crawler/manifest/models.pycrawler/tapio_crawler/manifest/store.pycrawler/tests/config/test_config_manager.pycrawler/tests/config/test_config_models.pycrawler/tests/conftest.pycrawler/tests/crawler/test_crawler.pycrawler/tests/crawler/test_policy.pycrawler/tests/crawler/test_runner.pycrawler/tests/manifest/test_store.pycrawler/tests/test_cli.pymise.toml
💤 Files with no reviewable changes (2)
- crawler/tests/conftest.py
- crawler/tapio_crawler/config/site_configs.yaml
| @staticmethod | ||
| def _next_retry( | ||
| record: ManifestRecord, | ||
| now: datetime, | ||
| summary: RenderRunSummary, | ||
| ) -> tuple[int, datetime | None]: | ||
| """Return the next retry count/timestamp for a failed or unconfirmed attempt. | ||
|
|
||
| Counts it in ``summary.retried`` only when a future retry is actually | ||
| scheduled (the cap isn't exceeded). | ||
| """ | ||
| retry_count = record.retry_count + 1 | ||
| if retry_count > MAX_RETRY_COUNT: | ||
| return retry_count, None | ||
| summary.retried += 1 | ||
| return retry_count, now + timedelta(seconds=retry_backoff_seconds(retry_count)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
The retry cap clears the backoff instead of parking the URL, and the test encodes that behavior. _next_retry returns retry_after=None once retry_count exceeds MAX_RETRY_COUNT. decide_render skips a record only while retry_after > now, so the record becomes due on the very next run with no backoff at all, and retry_count grows without bound.
crawler/tapio_crawler/crawler/crawler.py#L384-L399: past the cap, park the record. Either keep a longretry_afterin place or move the record to a terminal state thatlist_eligible_pageordecide_renderexcludes.crawler/tests/crawler/test_crawler.py#L232-L243: run a second render pass in the test and assert the parked record is not rendered again, instead of asserting onlyretry_countandretry_after is None.
📍 Affects 2 files
crawler/tapio_crawler/crawler/crawler.py#L384-L399(this comment)crawler/tests/crawler/test_crawler.py#L232-L243
🤖 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 `@crawler/tapio_crawler/crawler/crawler.py` around lines 384 - 399, Update
_next_retry in crawler/tapio_crawler/crawler/crawler.py:384-399 to park records
after MAX_RETRY_COUNT instead of returning retry_after=None; use a persistent
long retry_after or a terminal state excluded by
list_eligible_page/decide_render. Update
crawler/tests/crawler/test_crawler.py:232-243 to perform a second render pass
and assert the parked record is not rendered again.
…ema migration - decide_render now parks a record once its retry cap is exhausted without ever rendering successfully, instead of falling through to "initial_backfill" and being retried forever. - ManifestStore.rekey merges discovery provenance with an existing record at the target canonical URL instead of silently overwriting it. - ManifestStore adds any manifest column missing from an older database file on open, so a pre-Phase-2 manifest.db doesn't fail on the first retry_count read/write. - Fixed a CLI test whose "complete" substring check also matched "incomplete". The rate_limiter.py `except TypeError, ValueError, OverflowError:` review comment was a false alarm: confirmed valid PEP 758 syntax under the pinned Python 3.14.2, left unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@coderabbitai this project uses Python 3.14, so this is valid syntax:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
1 participant
Description
Implements Phase 2 ("controlled backfill") of docs/specs/crawler-improvements.md, building on Phase 1's manifest and discovery (#74). Covers Requirement 3 (manifest-driven collection and resumability), Requirement 4 (content selection and document quality), Requirement 5 (per-document cache and refresh policy), and the render-side counts for Requirement 6.
Phase 1 audit
Before starting, I audited PR #74 against the spec's Phase 1 acceptance criteria: all 83 original tests, ruff, and mypy passed, and Requirements 1, 2, and the discovery-side half of Requirement 6 are correctly implemented. Two minor, non-blocking gaps were noted for a future follow-up (no
discovery.cache_ttl_hourssitemap caching yet; robots/sitemap URLs aren't persisted as run metadata) — not fixed in this PR.What's included
crawl <site>no longer runs its own whole-site BFS deep crawl frombase_urltracked by a site-widecrawl_state.json. It now renders only manifest records thatdiscoveralready populated and that are due, paging through them via a newManifestStore.list_eligible_page, and persists each result to the manifest immediately after it completes — so a stopped run resumes from where it left off rather than restarting.crawler/policy.py, pure and independently unit-tested):decide_render()implements the spec's Requirement 5 rules — initial backfill and a trustedsitemap_lastmodchange useCacheMode.WRITE_ONLY; a stale extractor version or an elapsedunchanged_audit_dayswindow useCacheMode.ENABLEDwithcheck_cache_freshness=True. Crawl4AI'shit_validated/hit_fallback/failure cache outcomes are each handled per their specific acceptance criteria, including exponential backoff with a per-URL retry cap, and a render-time 429/503 now extends the same shared per-hostHostRateLimitersuspension discovery already respects.canonical_urlinstead of rawsource_url(fixing the spec-flagged bug where two URLs redirecting to the same page produced two files), with automatic rekeying via a newManifestStore.rekey()when a render's redirect resolves to a different canonical URL. Frontmatter gainscanonical_url,content_hash,language, andextractor_version. Also fixes a real gap the spec's Design Principle 7 called out: the render browser now sends the configuredUser-Agentinstead of Crawl4AI's default Chrome-spoofing string.crawl's output reports considered/rendered/saved/low-quality/failed/retried counts and a coverage ratio (current/eligible) computed from the manifest, warning when it's below the site's configuredrefresh.coverage_target_percent.DiscoveryConfig.trust_lastmod,RefreshConfig(unchanged_audit_days,inactive_grace_cycles,coverage_target_percent), andCrawlerConfig.word_count_threshold; removed the now-deadmax_depth/max_pages/recrawl_interval_hours(the BFS-from-base-url path andcrawl_state.jsonare gone —GapCrawlConfigkeeps its ownmax_depth/max_pagesfor the P1 gap-crawl supplement, untouched).crawldrops--depth(no longer meaningful) and adds--max-urls(default 5000) and--batch-size(default 500) per the spec's CLI-flag contract;--forceis re-scoped to mean "ignore each record's due-schedule."crawler/README.mdand the rootREADME.mdfor the new two-stepdiscover→crawlworkflow, andmise.toml'scrawltask now runsdiscoverbeforecrawlfor each configured site.Explicitly out of scope for this PR
Gap-crawl as a supplement for sitemap sources (P1), retrieval evaluation (P1), operator controls — pause/cancel/concurrent site jobs (P1), all P2 items, and any change to
ingest/(it already readssource_url/frontmatter independently and needs no crawler-side coupling change).Checklist
uv run ruff check .incrawler/and addressed any issuesCONTRIBUTING.mdand addressed any issuescrawler/tapio_crawler,uv run pytest --cov=tapio_crawler)README.md(both root andcrawler/) since this PR changes user-facing behaviorRelated issue
Continues the "Phase 2 — controlled backfill" requirements in docs/specs/crawler-improvements.md, following on from #74. Canonical tracking issue: #72.
Test plan
uv run pytest --cov=tapio_crawler— 108 passed, 97% coverageuv run ruff check .— cleanuv run mypy tapio_crawleranduv run pyrefly check tapio_crawler— cleanuv run tapio-crawler discover migri && uv run tapio-crawler crawl migri --max-urls 5) if a reviewer wants end-to-end confirmation before merging🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation