Consolidate review feedback for PRs #51 to #54 into a single follow up

[deandevz]

Hey Nelson, took the four PRs through local verification end to end on
my side, including a live update run against data/minimax-audio.json
so the audit + update story actually got exercised (about $0.06 of
OpenRouter spend total). Putting everything in one issue because the
cleanest path forward is one consolidated PR rather than four parallel
ones with overlapping cli.py edits and a partial dependency chain
between them. Details below.

What I verified

• pytest passes on each PR branch (fix(db): make insert_documentation an upsert (closes #48) #52: 114, fix(scraper): one chunk failure no longer aborts enrich batch (#47) #53: 122 scraper tests, feat(scraper): SCRAPE_CACHE_MODE and --no-fetch-cache for crawl4ai (#50) #54: 182, Add king-scrape update for incremental corpus refresh (ADR-0014) #51: 106 scraper tests). The five web test failures on main predate all four PRs and are unrelated.
• fix(db): make insert_documentation an upsert (closes #48) #52 upsert flow with re seed twice: same doc_id, created_at preserved, updated_at advances, FK CASCADE actually fires (the PRAGMA foreign_keys = ON per connection is what makes it real), FTS5 'delete' protocol runs before the sections delete so no orphaned index entries, embeddings flush after commit so a rollback cannot desync the numpy file from the DB. Solid.
• fix(scraper): one chunk failure no longer aborts enrich batch (#47) #53 enrich resilience with a real non transient ProviderError on one chunk in a five chunk batch: 4 of 5 enriched, 1 dropped, batch did not abort. Schema fallback success does NOT write to the primary cache (verified by patching enrich_cache.put and watching call count stay at 0 when only the fallback succeeded). PROMPT_VERSION derives from sha256(ENRICHMENT_PROMPT)[:16], so any prompt edit invalidates cache automatically. Best PR in the group, no concerns.
• feat(scraper): SCRAPE_CACHE_MODE and --no-fetch-cache for crawl4ai (#50) #54 cache bypass: env precedence works (existing SCRAPE_CACHE_MODE wins over flag via setdefault), the try/finally block restores or pops correctly, _resolve_cache_mode maps bypass / disabled / read_only / write_only (case insensitive) and warns once on garbage. Honoured by crawl4ai, firecrawl correctly ignores it.
• Add king-scrape update for incremental corpus refresh (ADR-0014) #51 update: reuse by content_hash works (296 of 297 chunks reused on a re run with no upstream changes, zero LLM call on those), staged drift on one corrupted _meta.content_hash correctly triggered exactly one re enrich (reused 296, enriched 1, lost 0), atomic write leaves no .tmp files, _meta is back filled on legacy corpora, CLI error paths are clear.

Per PR punch list

#52 (db upsert)

Blocking: none.

Worth knowing: _generate_and_save_embedding appends to embeddings.npy and never prunes entries from _section_id_to_idx when a section id is replaced by a re seed. Not a regression you introduced, the pre upsert path had the same characteristic. But this PR unlocks the "re seed often" workflow, so the bloat becomes visible. Worth a follow up issue, not a blocker.

#53 (enrich batch resilience)

Blocking: none. Nothing to change.

One thing worth flagging in the PR description: this PR is a hard dependency of #51's UpdateReport.lost contract. Without #53, a single non transient ProviderError on any chunk during king-scrape update would have crashed the process mid pipeline instead of being counted as lost. The "lost 0" assertion my e2e validates on #51 only holds in a world where #53 is merged. Worth a sentence in the PR description so the reviewer of #51 knows.

#54 (cache bypass)

Blocking:

1. --no-fetch-cache only exists on the main parser. king-scrape update foo --no-fetch-cache and king-scrape audit foo --no-fetch-cache both die with unrecognized argument because update_main (Add king-scrape update for incremental corpus refresh (ADR-0014) #51) and audit_main (Add king-scrape audit subcommand for corpus drift #49) have their own argparsers. The workaround is to set SCRAPE_CACHE_MODE=bypass directly, which works, but is not discoverable. Update is the command that most needs cache bypass (force refresh is the point), so the flag should plumb into all three entry points.

Worth knowing: guaranteed merge conflict in cli.py main() with #49 and #51 since all three touch the dispatcher. Mechanical resolve, but worth knowing before you rebase.

#51 (king scrape update)

Blocking:

1. base_url fallback on legacy corpora wipes the curated set and spends LLM credits silently. Repro: data/minimax-audio.json (legacy, no _meta, base_url: https://platform.minimax.io/docs). The original had 12 curated section URLs. After king-scrape update minimax-audio --yes: discover found 116 URLs, filter accepted 113, chunk produced 360 chunks, zero reused, all original enrichment lost, $0.06 of OpenRouter spend with no warning between the cost preview and the writeback. _resolve_source_url falls back to base_url silently in update.py:164, and on legacy corpora base_url is the host root, not the original entry point. Suggested fix: detect if not corpus.get("_meta") and require --allow-legacy-fallback explicitly, or refuse to proceed if the URL count diff against the existing corpus exceeds some threshold (e.g. >2x growth aborts).

2. Fetch failures silently truncate the corpus. On my e2e Run 2 and Run 3, fetch reported fetched 100, failed 14 on stdout. The UpdateReport has no fetch_failed field, the writeback proceeds anyway, and the corpus shrank from 360 to 297 sections between runs without anything obvious in the report. The existing "refuse to wipe a non empty corpus to empty" guard at update.py:317 only fires on a totally empty fetch; partial fetch failures pass through. Suggested fix: add fetch_failed: int to UpdateReport, surface it in the summary line, and abort the writeback if the failure ratio crosses a threshold (e.g. >10%).

3. removed_urls and added_urls counts are unreliable after the first update. They compare corpus_urls - fresh_urls, but once update has written through the existing pipeline the corpus carries slug form URLs (platform-minimax-io-docs-...) while fresh_urls is real https://.... Run 2 reported removed 106, added 114 when the real disk diff was 9 URLs lost and 2 new. Root cause is the pre existing chunk_pages bug below, but this PR inherits the noise. Suggested fix: either canonicalise both sides before diffing, or suppress the counters until the root cause is addressed.

4. Cost confirmation prompt fires even when there are zero new chunks (i.e. cost is $0.0000). I verified by calling _confirm_cost(plan, cfg, yes=False) with new_chunks=[] and input() is still invoked. The if plan.new_chunks check at update.py:333 is after the prompt at line 328. Suggested fix: short circuit the prompt when not plan.new_chunks, or move the prompt to inside the if plan.new_chunks: branch.

5. Discover is non deterministic. Three consecutive runs returned 116, 117, 117 URLs. Partly upstream's fault but the update flow has no stability gate against transient discovery drift. Suggested fix: if the fresh URL count diverges from the existing corpus by more than X%, abort with a clear error pointing at investigation.

6. ADR-0014 has two factual drifts with the code as merged. Line 65 says "Atomic write via tempfile + replace is a future hardening" and references save_and_index, but update.py:135 already implements _atomic_write_json and update.py:360 calls it (the actual export path is export_to_json + _atomic_write_json, not save_and_index). One paragraph rewrite to keep the ADR honest.

Worth knowing (not from your PR, but surfaced in my e2e):

• The audit subcommand (Add king-scrape audit subcommand for corpus drift #49) is broken on main: audit.py:242 calls resolve_provider_name(stage, explicit=...) but the live scraper_providers API is resolve_provider_name(stage) with no explicit kwarg. Blocks the audit + update demo as a pair. Worth a small fixup commit, either on top of Add king-scrape audit subcommand for corpus drift #49 or as part of the unified PR below.
• chunk_pages at chunk.py:202 uses md_file.stem as the chunk source_url instead of reading the real URL from the .meta.json sidecar that fetch_pages writes alongside each page. This is the root cause of the slug form URLs in the corpus and the reason concern (3) on Add king-scrape update for incremental corpus refresh (ADR-0014) #51 surfaces. Pre existing since Feb (3793aba), affects every corpus produced by the current pipeline (hono-test.json, httpx.json both show this). Worth fixing in the unified PR if you scope it that way.

Suggested unification

Rather than juggle four parallel PRs with cli.py conflicts and the #53 to #51 dependency, my suggestion is one consolidated PR that lands in this order:

1. fix(db): make insert_documentation an upsert (closes #48) #52 upsert verbatim (clean).
2. fix(scraper): one chunk failure no longer aborts enrich batch (#47) #53 enrich resilience verbatim (clean, unblocks Add king-scrape update for incremental corpus refresh (ADR-0014) #51's lost contract).
3. The two small pre existing fixes: audit.py:242 resolve_provider_name signature, and the chunk_pages slug bug if you want to clear concern (3) on update.
4. Add king-scrape update for incremental corpus refresh (ADR-0014) #51 update with the six fixes above.
5. feat(scraper): SCRAPE_CACHE_MODE and --no-fetch-cache for crawl4ai (#50) #54 cache bypass with the flag plumbed into update_main and audit_main so it actually works on the subcommands.

CHANGELOG entries fold into one section. ADR-0014 gets the paragraph fix. The cli.py main() ends up with one clean dispatcher block covering audit, update, and the cache env handling.

If you prefer to keep them separate, the dependency order to avoid mid pipeline crashes is: #52, then #53, then #49 fix + #51 + #54 together (or strictly ordered). Either way #51 should not merge without the six items above.

Happy to run the e2e again on whatever you ship, and to push the small fixups myself if it saves you time. Let me know which shape you want.

[nelsonmfinda]

Hey @deandevz thanks for the e2e $0.06 to surface 6 real bugs is a much better deal for the project than catching them in production. Going with your unified PR shape.

Acks per PR:

• fix(db): make insert_documentation an upsert (closes #48) #52: will add follow up issue link for embeddings.npy pruning.
• fix(scraper): one chunk failure no longer aborts enrich batch (#47) #53: will add the dependency note to the description.
• feat(scraper): SCRAPE_CACHE_MODE and --no-fetch-cache for crawl4ai (#50) #54: ack on subcommand plumb; will move env-set into a shared helper that audit_main and update_main both call.
• Add king-scrape update for incremental corpus refresh (ADR-0014) #51: ack on all 6. Three quick decisions I'd like your read on before I implement:
  1. Legacy corpus guard: prefer hard refuse + clear error, or --allow-legacy-fallback opt-in flag? Hard refuse is safer for someone running on CI; opt-in matches your existing CLI ergonomics.
  2. Fetch failure threshold: 10% as you suggested, or configurable via --max-fetch-failure-ratio? Default 10% either way.
  3. Discover divergence threshold: 2× growth aborts? Or asymmetric (e.g. >50% loss aborts, growth is allowed)?

On the pre existing fixes you flagged: please push the audit.py:242 signature fix as a standalone commit, pure mechanical, no design content. I'll own the chunk_pages source_url rewrite myself since it touches the exported JSON shape and I want to be thoughtful about backward compat with legacy data/*.json files (likely needs the sidecar reader to fall back to slug when no sidecar exists).

Will close #50/#52/#53/#54/#55 as superseded by the unified PR once it lands.

[deandevz]

Hey Nelson, glad we're aligned on the unified shape. Quick reads on the
three design questions, plus an update on the audit fix.

1. Legacy corpus guard: hard refuse. The failure mode this guard is
protecting against is silent ($ spend plus curated set wipe), and the
asymmetry between "user wanted full rescrape" and "user wanted update
to refresh the existing set" is exactly what king-scrape <url> vs
king-scrape update <name> already encodes. Opt-in flags become muscle
memory and then bypass real safety checks; the error message itself can
do the teaching here ("corpus has no _meta, re-scrape with
king-scrape <url> first to anchor it"). Legacy is a finite, one-time
concern (five files in data/), not a recurring workflow worth a
permanent flag for.

2. Fetch failure threshold: fixed 10%, no flag. KISS. The knob has
real cost (docs, tests, surface area, one more thing to reason about in
CI). If a legitimate need surfaces later, that is a focused follow-up.
I would rather ship one well-chosen default than a configurable that no
one knows where to set.

3. Discover divergence: asymmetric, >50% loss aborts, growth allowed.
Once the legacy guard in (1) is in place, growth on a _meta-carrying
corpus is genuine evolution (docs sites add pages, sections move). The
dangerous signal is loss, because it correlates with upstream breakage,
auth dropped, provider misconfigured. 50% is conservative enough to
catch real breakage without tripping on routine site reorganisations.
Same shape as the existing "refuse to wipe a non-empty corpus to empty"
guard, just at the URL-set level.

Audit fix landed

Pushed the audit.py signature fix as a standalone commit on main:
4a50531. One small thing to flag: there were actually two sibling bugs
on adjacent lines in _discover_fresh_urls, both with the same shape
(extra argument that the target function does not accept). I fixed both
in the same commit since the second one would have crashed immediately
after the first.

• resolve_provider_name("discover", explicit=...) (the one you
  flagged) -> dropped the explicit= kwarg
• get_discovery_provider(provider_name, config) -> dropped the
  config positional, since the function signature is
  get_discovery_provider(name: str)

Both calls now match the pattern already used in scraper/cli.py:126
(get_discovery_provider(resolve_provider_name("discover"))). The
existing audit tests passed because they mocked _discover_fresh_urls
wholesale rather than the provider boundary, which is why the bugs
shipped with #49. No behaviour change in the --no-discover and
"corpus has no base_url" paths that already worked.

Suite is green on the venv (735 passed), pure mechanical, no design
content.

chunk_pages source_url

Agreed on you owning that one. Your reasoning is right: the slug
fallback for legacy data/*.json without sidecars is exactly the kind
of judgement call I would rather have you make in one place with full
context than try to scope from outside.

E2e on the unified PR

Happy to run the e2e again once the unified PR is up. Same shape as
last time, against data/minimax-audio.json (legacy guard exercise)
and one non-legacy corpus for the discover divergence path. Ping me
when it is ready.

[deandevz]

Done. Unified PR #56 merged as ae1f941. All four PRs (#51,
#52, #53, #54) closed as superseded. The six fixes from the
e2e review are in, the three design decisions are
implemented as agreed (hard refuse, fixed 10%, asymmetric
50%), and the legacy guard was validated live against
data/minimax-audio.json (the corpus that triggered this
whole thread, file byte-identical after a refused update
attempt, $0 spend). Cosmetic follow-ups tracked in #57.
Closing.