fix(docker): apply crawler_configs to single-URL and streaming requests - #2290
Open
talelboussetta wants to merge 3 commits into
Open
talelboussetta wants to merge 3 commits into
talelboussetta wants to merge 3 commits into
Conversation
The per-URL config list (unclecode#1837) was only honoured when a non-streaming request carried two or more URLs. With one URL the handler called arun(), which takes a single config, and /crawl/stream (and /crawl with stream=true) never passed the list to handle_stream_crawl_request. Both requests ran with crawler_config alone and still answered 200. A request that carries a list now goes to arun_many whatever its URL count, and the streaming handler takes and applies the list. Loading moves into _load_crawler_configs(), shared by both handlers the way _normalize_and_validate_seeds() is, so both apply the same trust boundary and wire the PDF URL validator into every entry. On the streaming path each entry gets stream=True, since arun_many reads the stream flag from the first config of a list. The legacy source-grep test that pinned the single-URL path to arun() is updated to the new routing; behavioural tests are in deploy/docker/tests/test_crawler_configs_routing.py.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The streaming path mishandles per-URL PDF crawler selection and silently ignores per-URL deep-crawl strategies.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR updates Docker crawl routing so crawler_configs applies to single-URL and streaming requests.
Changes:
- Centralizes config-list loading and validation.
- Routes single-URL config lists through
arun_many. - Forwards config lists through streaming handlers.
- Adds and updates routing regression tests.
| File | Description |
|---|---|
tests/test_issue_1837_config_list.py |
Updates config-list routing expectations. |
deploy/docker/tests/test_crawler_configs_routing.py |
Adds routing behavior tests. |
deploy/docker/server.py |
Forwards config lists to streaming requests. |
deploy/docker/api.py |
Loads config lists and routes crawl requests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
With crawler_configs applied, the crawler was still chosen from the top-level crawler_config alone, so a list entry using PDFContentScrapingStrategy ran on the pooled browser crawler and failed before the PDF scraper could run. _needs_pdf_crawler() decides from the list when there is one: a list of PDF entries runs on PDFCrawlerStrategy, and since one crawler serves every URL of a request, a list mixing PDF and browser entries is refused with 400 instead of failing mid-crawl. The non-streaming handler now loads the list before it picks a crawler, as the streaming one already did.
Wraps the new lines over black's limit. The source assertion in test_issue_1837_config_list.py now matches the call rather than the whole one-line conditional, so it no longer depends on how the line is wrapped. No behaviour change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Fixes #2287
crawler_configs(#1852) was only honoured when a non-streaming/crawlcarried two or more URLs. With one URL the handler calledarun(), which takes a single config, and/crawl/stream(and/crawlwithstream: true) never passed the list down. Both answered 200 having run withcrawler_configalone.arun_manywhatever its URL count;arun_manyalready matches each URL against the list.handle_stream_crawl_requesttakescrawler_configsand applies it;stream_processpasses it. Each entry getsstream=True, sincearun_manyreads the stream flag from the first config of a list._load_crawler_configs(), shared by both handlers the way_normalize_and_validate_seeds()is, so both apply the same untrusted boundary and wire the PDF URL validator into every entry._needs_pdf_crawler()): a list ofPDFContentScrapingStrategyentries runs onPDFCrawlerStrategy, and since one crawler serves every URL of a request, a list mixing PDF and browser entries is refused with 400 instead of running its PDF entries on the browser crawler. The non-streaming handler now loads the list before it picks a crawler.@hafezparast: this reverses what
test_single_url_ignores_crawler_configspinned. I read the rationale there ("arun only takes one config") as the reason for the branch rather than a wish to drop the list, so the test is rewritten to assert the new routing. If ignoring it for one URL was deliberate for another reason, I'd like to know.Not changed: on the streaming path
base_configis still not applied (it isn't applied tocrawler_configthere either), and a streaming deep crawl still runs fromcrawler_config, asarun_manyitself does. A per-URLdeep_crawl_strategycan't reach either path: it is on the untrusted forbidden list, so such a body is rejected when it is loaded.List of files changed and why
deploy/docker/api.py:_load_crawler_configs()and_needs_pdf_crawler(); single-URL routing througharun_manywhen a list is present; the streaming handler's new parameter.deploy/docker/server.py:stream_processforwardscrawler_configs.deploy/docker/tests/test_crawler_configs_routing.py(new): behavioural tests against the real handlers with a recording crawler: one URL with a list, one URL without, streaming with a list, the endpoint forwarding it, a list of PDF entries running on the PDF crawler, and a mixed list refused on both paths.tests/test_issue_1837_config_list.py: the two source assertions that pinned the old branch are updated to the new one.How Has This Been Tested?
The offline suites (
deploy/docker/testswithout the live-servertest_1–test_7scripts,tests/unit, the pool tests) show no new failures againstdevelop.End to end, on an image built from
develop@ 1f68e5b (docker build -t c4ai-dev .), against a local test site, soCRAWL4AI_ALLOW_INTERNAL_URLS=true, before and with this branch'sapi.pyandserver.pymounted over/app. The page has two sections,.only-aand.only-b; the list setscss_selector: ".only-a"withurl_matcher: "*":cc @ntohidi
Checklist: