Repository navigation
feat(ingest): crawl discovery source (#586) - #588
bhushan4work wants to merge 3 commits into
Conversation
|
@anurag629 could u please review this pr |
anurag629
left a comment
There was a problem hiding this comment.
Thanks @bhushan4work, this mostly works. The count + inFlight check keeps a crawl at maxPages, links go through normalizeUrl() and seen before they're queued, exclude and robots.txt are checked before a fetch, and crawled pages keep their HTML so ingest() doesn't fetch them again (ingest.test.ts:443 checks one request per page).
A few things need to change before it can go in:
-
The crawl ignores where a redirect ends up. It fetches with
redirect: 'follow'(packages/ingest/src/discover.ts:222-226), then stores the page under the URL it asked for and resolves its links against that URL, neverresponse.url. Ifhttps://example.comredirects tohttps://www.example.com/(orhttp://tohttps://), a site that writes absolute links, as WordPress menus do, crawls one page, because everyhttps://www.example.com/...link fails the origin check at line 291. If/docsredirects to/docs/, a relative<a href="install">there resolves to/installinstead of/docs/install. And a same-origin link that redirects off the site (/githubtohttps://github.com/acme) gets that site's HTML indexed ashttps://example.com/github. Please take the crawl's origin, and its robots.txt, from where the start page ended up, skip any later page whose final URL is on another origin, and useresponse.url || urlas the page's URL, the base for its links and the key inseen(response.urlis empty for aResponsebuilt in a test). -
One failed response body fails the whole run.
await response.text()atdiscover.ts:240is inside atrythat only has afinally, so a page that sends its headers and then stalls pasttimeoutMs, or drops the connection mid-body, rejects the worker,Promise.all,discover()andingest().chatcops ingestexits with 2 and indexes nothing. Catch it and push the message toerrors, the same as the fetch failure just above it. While you're there, the!response.okand non-HTML branches at lines 232-238continuewithout touching the body. Callawait response.body?.cancel()first, asrobots.ts:104and #553 do, since a crawl meets far more PDFs and 404s than a sitemap. -
The page caps.
IngestConfig.maxPages(--max-pages) never reaches the crawl, sochatcops ingest --crawl https://example.com --max-pages 20 --dry-runcan still fetch 500 pages during discovery beforeingest.ts:117keeps 20. Pass it intodiscover()and stop the crawl at the smaller of the two. And when the crawl stops at its cap with links still queued, nothing marks the discovery as partial, so--pruneremoves indexed pages the crawl just didn't reach this time.ingest.ts:117-121pushesstopped at --max-pagesfor that reason; push the same kind of entry when the crawl stops with URLs left incurrentLevelornextLevel. Also,nextLevelis filled as each page finishes (line 251), so withconcurrencyabove 1 its order, and so which pages make the cut, depends on which response came back first. Collect each page's links by its position incurrentLeveland join them in that order. The exact-order assertions atingest.test.ts:90and:93rely on/crawl/bfs/aanswering before/crawl/bfs/btoday. -
A crawl that can't start says nothing. When the start URL doesn't match
include/exclude(discover.ts:141), is disallowed by robots.txt (line 180) or isn't HTML (line 238), the crawl returns no pages and no error. The CLI printsFound 0 pages.and exits 0, and with--prunethat empty discovery removes every page in the index. Push an error for the start URL in each case, such asblocked by robots.txtornot HTML (application/pdf). -
Some of the tests can't catch what they're named for. The cross-origin link at
ingest.test.ts:122ishttps://other.example.com/other, which never reaches the fixture server, sonot.toContain('/other')at line 140 passes even with the origin check atdiscover.ts:291deleted: the fetch would fail intoerrors, which the test doesn't look at. Start a secondfixtureSite()(another port is another origin), link to it, and assert itsrequestsstays empty andres.errorsis[]. ThemaxPagestest at line 112 counts the pages returned, not the ones fetched, so also assert/crawl/mp/band/crawl/mp/cwere never requested. Then add tests for points 1, 2 and 4.site.set()takes a status and headers, sosite.set('/crawl/r/docs', '', 'text/html', 301, { location: '/crawl/r/docs/' })gives the trailing slash case, and a redirect from one fixture site to the other gives both the start URL case and a link that leaves the site. For point 2, afetchwrapper in the test that returns this for one URL is enough:new Response(new ReadableStream({ start: (c) => c.error(new Error('reset')) }), { headers: { 'content-type': 'text/html' } })
-
Docs.
packages/cli/README.md:28lists the ingest options without--crawl, and the error atpackages/cli/src/main.ts:167still only sayspass --source. The new row inwebsite/src/content/docs/core/knowledge-base.mdxshould say thatmaxDepthcounts link hops fromurl(0 isurlalone), and that for a crawlincludeandexcludealso decide which links are followed,urlincluded, soinclude: ['https://example.com/docs/**']withurl: 'https://example.com/'finds nothing.
Push when these are in and I'll review it again.
|
@anurag629 requested fixes are pushed and ci is green too . could u run a final review before merge |
anurag629
left a comment
There was a problem hiding this comment.
Thanks @bhushan4work, nearly there. All six points are in: the redirect and cross-origin tests use a second fixture site, a body that errors lands in errors instead of failing the run, a capped or empty crawl now blocks --prune, and the level order no longer depends on which response comes back first. All checks pass, including e2e.
A few things need to change before it can go in:
-
The start page is now fetched before robots.txt is read.
crawlSite()fetchesstartUrlatpackages/ingest/src/discover.ts:194and only checks robots oncanonicalStartUrlat line 235. So withUser-agent: ChatCopsBot/Disallow: /,chatcops ingest --crawl https://example.comstill requests the homepage. The first version checked before the fetch, andurlsandsitemappages are checked before theirs (ingest.ts:158-159). Please checkstartUrlagainst its robots.txt before the fetch, and keep the check oncanonicalStartUrlfor the redirect case. In the robots part of the startup test (ingest.test.ts:308-315), also assertsite.requestsdoesn't contain/crawl/startfail/blocked. -
The cap message names
--max-pageseven when the crawl's own limit stopped it.discover.ts:149,:271and:416all pushstopped at --max-pages ${effectiveMaxPages}. On a 700-page site,chatcops ingest --crawl https://example.com --max-pages 1000stops at the default 500 and reportsstopped at --max-pages 500, a number the user never passed. Keep that text whenoptions.maxPagesis the smaller cap, and use something likehttps://example.com/: crawl stopped at maxPages 500when it's the source's. The test atingest.test.ts:112-116setsmaxPages: 2on the source and expects that message, so it's the one to update. -
DiscoverOptions.maxPagessays "Limit total pages discovered." (discover.ts:21), but onlycrawlSite()reads it, sodiscover([{ type: 'sitemap', url }], { ...options, maxPages: 10 })still returns the whole sitemap. Say it caps each crawl source.
Push when these are in and I'll review it again.
anurag629
left a comment
There was a problem hiding this comment.
Thanks @bhushan4work. robots.txt is now read before the start page is fetched (packages/ingest/src/discover.ts:196-207), the check on where a redirect lands is still there at line 246, and ingest.test.ts:339 and :352 cover both. The cap message only names --max-pages when that's the limit that stopped the crawl, and the tests at lines 116-146 check each side. All checks pass, including e2e.
what does this change?
adds sitemap-independent website crawling to
@chatcops/ingest, allowing ingestion directly from a homepage. supports breadth-first same-origin crawling with configurable depth and page limits, url normalization, deduplication, robots.txt, include/exclude rules, concurrency limits, and fetch timeouts.adds
--crawl <url>andcrawl:<url>source support to the cli. reuses fetched html during extraction to avoid duplicate requests. updates tests, api snapshots, documentation, and changesets.closes #586
how i tested this
pnpm test:coveragepassed (85/80 thresholds met).pnpm lintpassed.pnpm apipassed (all 10 package api snapshots verified).pnpm docs:checkpassed (205 typescript samples checked).--crawlandcrawl:source support.ai assistance
none
checklist
devpnpm -r build,pnpm testandpnpm -r typecheckpass locallypnpm changeset) if this changescore,widgetorserverpnpm apirun andapi/committed if an exported type or option changed