feat(scraper): content hash provenance and enrichment cache - #46
Conversation
|
Hey Nelson, took a careful read through. Strong PR, alignment with the What stood out as good:
What I need before I merge:
A couple of things I noticed but I am leaving to your judgment. If you
On the follow-ups (ADR-0013 + audit, ADR-0014 + --update), splitting Let me know once the end-to-end is done and I will move on the merge. |
|
What beautiful solutions! |
|
Hey @deandevz I will address your comments, thanks! |
|
Merged. End to end is green and the PR delivers what the description I ran the e2e against What shipped, verified on disk:
What stays solid: cache writes during enrichment, full cache hit on The two side issues you filed (#47 non transient enrich error Looking forward to ADR-0013 + `audit` and ADR-0014 + `--update` on |
* fix(db): make insert_documentation an upsert (closes #48) * fix(scraper): one chunk failure no longer aborts enrich batch (#47) * refactor(scraper): promote URL canonicalisation to shared helper New module ``king_context.scraper.url_utils`` exposes ``canonicalize_url``, lifted verbatim from ``audit._canonicalize``. ``audit.py`` re-exports the private name for compatibility with its own callsites. Pure refactor, no behaviour change. Sets up shared use from ``update.py`` in a follow-up commit where URL diff parity between corpus_urls and fresh_urls needs the same normalisation. * fix(scraper): chunk_pages reads real URL from page sidecar chunk_pages used md_file.stem as source_url, so every chunk produced by the scraper carried slug-form URLs like docs-example-com-quickstart instead of the real https://docs.example.com/quickstart. Pre-existing since the chunk module landed; the consequence surfaced in #51 as noisy added_urls / removed_urls diffs when update compared corpus_urls (slug-form, written by this path) against fresh_urls (real URLs, from the provider). Fix: read the real URL from the page sidecar fetch.py writes alongside each <slug>.md as <slug>.meta.json. When the sidecar is absent or unreadable (legacy _temp/ from pre-#46 scrapes), fall back to the slug so existing work directories continue to chunk. Backward compat at the corpus boundary: committed data/*.json files keep their slug-form URLs until the next refresh writes through the new path. The audit command's URL canonicalisation absorbs the transient mismatch in the meantime. Four new tests cover the sidecar-present, sidecar-missing, sidecar-malformed, and url-key-missing branches. * feat(scraper): king-scrape update (ADR-0014) + cache bypass + 6 fixes * fix(scraper): _resolve_source_url handles explicit null _meta
Summary
Adds a
content_hashto chunks and a small file-per-hash cache for enrichment results, so a re-run on unchanged content doesn't pay for a new LLM call. Also threads optional_meta(content hash, scrape timestamp, scraper version) through the exported corpus and per-page sidecars. This is the foundation for drift detection and incremental refresh later, by itself it's mostly invisible to users on a fresh scrape, but it makes those follow-ups possible without a redesign.Type of change
How it was tested
pytest -q700 passed, 1 skipped
Covered with new tests:
tests/test_scraper/test_enrich_cache.py— key derivation, atomic writes, no tmp leak on serializable failure, roundtrip, overwrite, IO error handlingtests/test_scraper/test_chunk.py—Chunk.content_hashis populated, deterministic, and changes with contenttests/test_scraper/test_export.py— section_metacarries the hash, top-level_metahas the right shapetests/test_scraper/test_fetch.py— per-page<slug>.meta.jsonsidecar is writtenThe pre existing scraper tests share chunk content across calls, so I added an autouse fixture in
tests/conftest.pythat points the cache at a tmp dir per test (otherwise the on disk cache short circuits the LLM mocks).I haven't run a full end to end
king-scrapeagainst a live docs site in this PR, happy to do that if you'd prefer before merge, or to land it as part of the follow-up audit/update PRs where it'll get exercised more naturally.Checklist
pytest)docs/and/orREADME.mdif behavior changed (no behavior change at the public surface; ADR-0012 added under.king-context/adr/and an entry inCHANGELOG.md)Additional notes
A few things worth flagging:
cachehashwas on the table as a backing store. I rejected it for two reasons. The package is published as "free for non-commercial use only", which doesn't compose with theproject's MIT license. And its shape (opaque SQLite, eviction/TTL) is the opposite of what ADR-0010 asks for — pipeline-owned, inspectable IO. The current layout lets a contributor
cat .king-context/cache/enrichment/<sha>.jsonto debug a regression, orrm -rffor a clean slate. ADR-0012 records the trade-off.PROMPT_VERSIONis auto-derived fromsha256(ENRICHMENT_PROMPT)[:16], not a manual constant. So if anyone edits the prompt, cached entries invalidate without needing a human toremember to bump a number. I went back and forth on this and ended up convinced the auto-derived version is the safer default.
_metaonly carriescontent_hashfor now. The plan originally includedfetched_atandpage_urlper section too, but those need data that lives in the pagesidecars, and threading them through felt out of scope for this PR. The sidecars are on disk during a scrape, so the future
--updatework can pick them up directly.king-scrape auditsubcommand for drift detection, then ADR-0014 +king-scrape <url> --updatefor theincremental refresh that makes use of all this provenance. Both should sit cleanly on top of this PR. I'd send them as separate PRs to keep each one reviewable.