fix(kb): add traceable web ingest and owners-compatible metadata#179
fix(kb): add traceable web ingest and owners-compatible metadata#179sqhyz55 wants to merge 7 commits into
Conversation
07fe27b to
84d5e7d
Compare
1feac31 to
97e7177
Compare
rogercloud
left a comment
There was a problem hiding this comment.
Review — PR #179
MAJOR
Dead code: _fetch_html + _fetch_html_static — crawler.py:695-721
The _crawl_page method calls either _fetch_html_rendered(url) (line 575) or _fetch_with_fallback(sessions, url) (line 582) directly. The _fetch_html method and its _fetch_html_static helper are defined but never wired in — zero callers across the entire codebase. ~27 lines that should be removed or wired up.
MINOR
No API-level validation for render_wait_until — kb.py:2314
render_wait_until is an Optional[str] Form field. An invalid value like "invalid" passes through to WebCrawlConfig(render_wait_until=...), where Pydantic's Literal check raises a ValidationError → HTTP 500. Should validate at the API boundary and return a 422 with a clear message.
Unused HTTP sessions when render_js=True — crawler.py:273-301
When render_js=True, _open_sessions at line 297 still creates httpx/curl_cffi sessions, but _crawl_page never uses them in the Playwright path — it calls _fetch_html_rendered directly. The sessions are properly cleaned up by AsyncExitStack, so this is only wasteful, not a leak.
Other
- Playwright lifecycle (
_start_playwright/_stop_playwright) withtry/finally— correct ✓ start_urlvalidation withurlparse— handles whitespace, missing scheme/netloc ✓asyncio.gather(return_exceptions=True)correctly catches PlaywrightRuntimeError✓page.goto()returningNonehandled gracefully withstatus_code=None✓bool(render_js)wrapper — redundant (FastAPI already coerces), but harmless ✓
Tests: 10/10 passed
rogercloud
left a comment
There was a problem hiding this comment.
Additional non-duplicate review comments from static review.
- Validate HTTP status code in Playwright rendered path to reject non-2xx pages before content cleaning (data correctness bug) - Mock run_web_ingestion in test_ingest_web_accepts_stripped_url to avoid network/storage dependency and assert stripped URL - Add render_js checkbox to knowledge-base-creation-dialog for feature parity with the detail-page add-source flow - Replace hardcoded Chinese label with i18n key in detail page
- Validate HTTP status code in Playwright rendered path to reject non-2xx pages before content cleaning (data correctness bug) - Mock run_web_ingestion in test_ingest_web_accepts_stripped_url to avoid network/storage dependency and assert stripped URL - Add render_js checkbox to knowledge-base-creation-dialog for feature parity with the detail-page add-source flow - Replace hardcoded Chinese label with i18n key in detail page
7c7b2a2 to
de0bb29
Compare
- Validate HTTP status code in Playwright rendered path to reject non-2xx pages before content cleaning (data correctness bug) - Mock run_web_ingestion in test_ingest_web_accepts_stripped_url to avoid network/storage dependency and assert stripped URL - Add render_js checkbox to knowledge-base-creation-dialog for feature parity with the detail-page add-source flow - Replace hardcoded Chinese label with i18n key in detail page
de0bb29 to
2e8925b
Compare
rogercloud
left a comment
There was a problem hiding this comment.
Re-reviewed the latest update. The previous inline issues for rendered HTTP status handling, the creation-dialog JS toggle, i18n, and the stripped URL test path look addressed.
Remaining items are non-blocking cleanup / polish:
src/xagent/core/tools/core/RAG_tools/web_crawler/crawler.py:_fetch_html()/_fetch_html_static()still appear to be dead code;_crawl_page()continues to call_fetch_html_rendered()or_fetch_with_fallback()directly.src/xagent/core/tools/core/RAG_tools/web_crawler/crawler.py:render_js=Truestill opens the normal HTTP session stack even though the Playwright path does not use it. This is wasteful but not a leak.src/xagent/web/api/kb.py: invalidrender_wait_untilvalues still rely onWebCrawlConfigvalidation rather than explicit API-boundary validation, so hand-written API calls may get a less clear error than a targeted 422.
I do not consider these production blockers.
Fixes xorbitsai#162 by adding end-to-end trace_id logging, stricter start_url validation, fail-fast crawl errors, and optional JS-rendered crawling (Playwright) with clearer user guidance. Also adds compatibility for collections requiring an owners field in LanceDB metadata (aligned with xorbitsai#166).
Some FastAPI/Starlette versions treat empty form fields as missing ("Field required"), while others pass an empty string through to our custom start_url validation.
This test now accepts either 422 detail shape for blank inputs.
Made-with: Cursor
- Remove duplicate `owners: List[str]` field in CollectionInfo (mypy no-redef) - Fix datetime naive/aware mismatch in web_ingestion fail-fast path - Add get_db override in test_kb_ingest_web_invalid_start_url fixture - Accept extra kwargs in test_kb_web_ingestion_uploaded_file stubs (trace_id) - Update test assertion to match main's "Website crawling failed:" message format
…indings Critical fixes: - Pass render_js/render_wait_until/render_timeout_ms to WebCrawlConfig in kb.py - Route _crawl_page through _fetch_html_rendered when render_js=True - Remove duplicate owners column from PyArrow metadata schema - Remove owners migration leftover (conflicts with derived-only design) Major fixes: - Pass trace_id to run_document_ingestion in web_ingestion pipeline - Clean up Playwright process on _start_playwright failure - Replace defensive getattr(config, "render_js", False) with direct access Performance: - Guard html_snippet construction with logger.isEnabledFor(DEBUG) - Downgrade is_valid_content rejection logs from INFO to DEBUG
- Validate HTTP status code in Playwright rendered path to reject non-2xx pages before content cleaning (data correctness bug) - Mock run_web_ingestion in test_ingest_web_accepts_stripped_url to avoid network/storage dependency and assert stripped URL - Add render_js checkbox to knowledge-base-creation-dialog for feature parity with the detail-page add-source flow - Replace hardcoded Chinese label with i18n key in detail page
- Remove unused _fetch_html/_fetch_html_static dead code in web crawler - Skip HTTP session creation when render_js uses Playwright only - Validate render_wait_until at API boundary with 422 on invalid values
Use any blocking failure (not all) to return the friendly anti-bot message during fail-fast, matching post-rebase message behavior.
2e8925b to
9d30031
Compare
Fixes #162 by adding end-to-end trace_id logging, stricter start_url validation, fail-fast crawl errors, and optional JS-rendered crawling (Playwright) with clearer user guidance.
Also adds compatibility for collections requiring an owners field in LanceDB metadata (aligned with #166).