Skip to content

feat(ingest): crawl discovery source (#586) - #588

Open
bhushan4work wants to merge 3 commits into
codercops:devfrom
bhushan4work:586
Open

bhushan4work wants to merge 3 commits into
codercops:devfrom
bhushan4work:586

Conversation

@bhushan4work

Copy link
Copy Markdown
Collaborator

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> and crawl:<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:coverage passed (85/80 thresholds met).
  • pnpm lint passed.
  • pnpm api passed (all 10 package api snapshots verified).
  • pnpm docs:check passed (205 typescript samples checked).
  • added ingest tests covering traversal, depth and page limits, url filtering, robots.txt, exclude rules, and duplicate-fetch prevention.
  • added cli tests covering --crawl and crawl: source support.

ai assistance

none

checklist

  • this pr targets dev
  • i was assigned the linked issue
  • pnpm -r build, pnpm test and pnpm -r typecheck pass locally
  • tests added or updated where it makes sense
  • for a bug fix: the new test fails without my change
  • changeset added (pnpm changeset) if this changes core, widget or server
  • pnpm api run and api/ committed if an exported type or option changed
  • docs updated if behavior or options changed

@bhushan4work

Copy link
Copy Markdown
Collaborator Author

@anurag629 could u please review this pr

@anurag629 anurag629 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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, never response.url. If https://example.com redirects to https://www.example.com/ (or http:// to https://), a site that writes absolute links, as WordPress menus do, crawls one page, because every https://www.example.com/... link fails the origin check at line 291. If /docs redirects to /docs/, a relative <a href="install"> there resolves to /install instead of /docs/install. And a same-origin link that redirects off the site (/github to https://github.com/acme) gets that site's HTML indexed as https://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 use response.url || url as the page's URL, the base for its links and the key in seen (response.url is empty for a Response built in a test).

  2. One failed response body fails the whole run. await response.text() at discover.ts:240 is inside a try that only has a finally, so a page that sends its headers and then stalls past timeoutMs, or drops the connection mid-body, rejects the worker, Promise.all, discover() and ingest(). chatcops ingest exits with 2 and indexes nothing. Catch it and push the message to errors, the same as the fetch failure just above it. While you're there, the !response.ok and non-HTML branches at lines 232-238 continue without touching the body. Call await response.body?.cancel() first, as robots.ts:104 and #553 do, since a crawl meets far more PDFs and 404s than a sitemap.

  3. The page caps. IngestConfig.maxPages (--max-pages) never reaches the crawl, so chatcops ingest --crawl https://example.com --max-pages 20 --dry-run can still fetch 500 pages during discovery before ingest.ts:117 keeps 20. Pass it into discover() 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 --prune removes indexed pages the crawl just didn't reach this time. ingest.ts:117-121 pushes stopped at --max-pages for that reason; push the same kind of entry when the crawl stops with URLs left in currentLevel or nextLevel. Also, nextLevel is filled as each page finishes (line 251), so with concurrency above 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 in currentLevel and join them in that order. The exact-order assertions at ingest.test.ts:90 and :93 rely on /crawl/bfs/a answering before /crawl/bfs/b today.

  4. 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 prints Found 0 pages. and exits 0, and with --prune that empty discovery removes every page in the index. Push an error for the start URL in each case, such as blocked by robots.txt or not HTML (application/pdf).

  5. Some of the tests can't catch what they're named for. The cross-origin link at ingest.test.ts:122 is https://other.example.com/other, which never reaches the fixture server, so not.toContain('/other') at line 140 passes even with the origin check at discover.ts:291 deleted: the fetch would fail into errors, which the test doesn't look at. Start a second fixtureSite() (another port is another origin), link to it, and assert its requests stays empty and res.errors is []. The maxPages test at line 112 counts the pages returned, not the ones fetched, so also assert /crawl/mp/b and /crawl/mp/c were never requested. Then add tests for points 1, 2 and 4. site.set() takes a status and headers, so site.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, a fetch wrapper 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' } })
  6. Docs. packages/cli/README.md:28 lists the ingest options without --crawl, and the error at packages/cli/src/main.ts:167 still only says pass --source. The new row in website/src/content/docs/core/knowledge-base.mdx should say that maxDepth counts link hops from url (0 is url alone), and that for a crawl include and exclude also decide which links are followed, url included, so include: ['https://example.com/docs/**'] with url: 'https://example.com/' finds nothing.

Push when these are in and I'll review it again.

@bhushan4work

Copy link
Copy Markdown
Collaborator Author

@anurag629 requested fixes are pushed and ci is green too . could u run a final review before merge

@anurag629 anurag629 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The start page is now fetched before robots.txt is read. crawlSite() fetches startUrl at packages/ingest/src/discover.ts:194 and only checks robots on canonicalStartUrl at line 235. So with User-agent: ChatCopsBot / Disallow: /, chatcops ingest --crawl https://example.com still requests the homepage. The first version checked before the fetch, and urls and sitemap pages are checked before theirs (ingest.ts:158-159). Please check startUrl against its robots.txt before the fetch, and keep the check on canonicalStartUrl for the redirect case. In the robots part of the startup test (ingest.test.ts:308-315), also assert site.requests doesn't contain /crawl/startfail/blocked.

  2. The cap message names --max-pages even when the crawl's own limit stopped it. discover.ts:149, :271 and :416 all push stopped at --max-pages ${effectiveMaxPages}. On a 700-page site, chatcops ingest --crawl https://example.com --max-pages 1000 stops at the default 500 and reports stopped at --max-pages 500, a number the user never passed. Keep that text when options.maxPages is the smaller cap, and use something like https://example.com/: crawl stopped at maxPages 500 when it's the source's. The test at ingest.test.ts:112-116 sets maxPages: 2 on the source and expects that message, so it's the one to update.

  3. DiscoverOptions.maxPages says "Limit total pages discovered." (discover.ts:21), but only crawlSite() reads it, so discover([{ 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 anurag629 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ingest: a crawl source for sites without a sitemap

2 participants