Add king-scrape audit subcommand for corpus drift - #49
Conversation
deandevz
left a comment
There was a problem hiding this comment.
Hey Nelson, gave this a careful read and a live run. Strong PR, third
in the row from you that lands clean against the project's direction.
What stood out as good:
- ADR-0013 is the right artifact for this kind of feature. Real
alternatives (inline--auditflag, standaloneking-auditscript,
full content-hash diff in this PR) each rejected with a grounded
reason, and the deferral of content-hash to ADR-0014 is the right
call. Keeps this PR doing one thing well. - The dispatcher pattern (
if argv[1] == "audit":) is exactly the
minimum needed. Existingking-scrape <url>flow stays byte for
byte unchanged, and theupdatesubcommand has a clear precedent to
follow. I prefer this over an argparse subparser refactor for the
current count of subcommands. - Read-only contract is honest end to end. Never touches
data/<name>.json, never writes to the DB, no provider key needed
for the URL health pass. Safe to wire to a CI cron, which is the
whole point. - Test coverage is where I want it. Canonicalization (fragment,
trailing slash, host caps), HEAD to GET fallback on 405/501, 429
retry honouringRetry-Aftercapped at 30s, narrow except on
discovery soTypeErrorpropagates instead of masquerading as
"discovery skipped", ASCII-only assertion on the report. The
bug-class propagation test in particular is the kind of detail that
catches real future regressions. - Live run against
data/elevenlabs-api.json(181 sections) finished
in seconds and produced a clean Markdown report. Exit code 2 fired
on broken URLs as advertised.
What I need before I merge:
-
Redirect-to-broken misclassified as
moved._classifychecks
response.historyfirst and returnsmovedwhenever the chain has
any 3xx hop, regardless of the final status. Live repro from the
elevenlabs corpus (which I indexed myself months ago, when the URL
was live and served real "Error Messages" content):HEAD https://elevenlabs.io/docs/developers/resources/error-messages -> 308 -> https://elevenlabs.io/docs/eleven-api/resources/error-messages -> final status 404ElevenLabs has since reorganised their docs, set up a redirect, but
the new target was never published. Visually confirmed in the
browser, the final URL renders the ElevenLabs 404 page. This is
exactly the kind of drift the audit exists to surface, but the
current classifier reports it asmovedwithHTTP 404 | -> ...
in the moved section. The broken count does not include it, so
audit_mainreturns0and the CI gate passes silently on a dead
link. The existing test only covers301 -> 200, which is why it
slipped.Fix shape I would take: classify by the final response status
first, and only mark asmovedwhen the chain ended in 2xx. A
301 -> 404should reportbrokenwith the final URL captured for
context. Add a test that asserts301 -> 404returnsbrokenand
audit_mainreturns2. -
Docs and CHANGELOG. The PR description has both boxes ticked, but I
could not find any audit entry indocs/CLI_GUIDE.md(which has a
detailedking-scrapeflag table) or inREADME.md, and the
CHANGELOG section in this PR's body looks like it was carried over
from #46. A short subsection indocs/CLI_GUIDE.mdplus an
## [Unreleased]entry pointing at ADR-0013 is enough.
A couple of things I noticed but I am leaving to your judgment:
- The audit timestamp in the report filename strips
:,-, and.
but leaves+0000from the ISO offset, which yields names like
elevenlabs-api-20260508T154243082839+0000.md. Filename works on
every filesystem I care about, just slightly long. Up to you whether
to drop the offset for the filename only. _print_summaryreports+N / -Mfor new and orphan upstream URLs
in the stdout one-liner. Easy to read once you know the convention,
but a future contributor might expectnewandorphanwords. Not
worth changing now, only flagging.
On the follow-up (ADR-0014 + king-scrape <url> --update), keeping it
in a separate PR is right. Once #1 is fixed and the docs are in,
this is good to land.
Thanks for the careful turnaround on this one.
|
Thanks @deandevz . All five addressed. Required:
Optional, took both:
Bonus while I was in there:
|
deandevz
left a comment
There was a problem hiding this comment.
Thanks Nelson, all five addressed cleanly and the bonus work is the
right kind of bonus.
Verified locally:
- Full audit suite is now 33 passing (up from 20). The new
test_audit_main_returns_2_on_redirect_to_brokenis the test I
wanted; redirect chains ending in 500 / 401 / 429 are covered too. - Live run on
data/elevenlabs-api.json --no-discoverfinished in
seconds and surfaced 3 broken URLs including the elevenlabs
error-messages chain (HTTP 404 | -> .../eleven-api/resources/...).
Exit code 2, no falsemovedclassification. That was the bug. docs/CLI_GUIDE.md"Audit a corpus for drift" section reads well
alongside the existingking-scrapeflag table. CHANGELOG entry
matches the actual behaviour and lists the new statuses.- Filename now
<name>-<UTC>Z.mdwith microsecond precision. Two
audits in the same second do not collide. - Summary line uses
new N / orphan M. Clearer for a first time
contributor reading the stdout.
The bonus items are worth keeping:
- HTTP-date
Retry-Afteris the correct shape for upstreams that
return RFC 7231 dates. Past dates falling back to 1s and a 30s cap
on future dates is exactly the right tradeoff for an audit that
must make forward progress. - Naive
audited_atno longer crashing_report_pathis one of
those small robustness wins that pay off the first time someone
feeds in a hand built corpus.
ADR-0013 lines up with what landed. Ready to merge.
On the follow up (ADR-0014 + `--update`), separate PR sounds right.
Looking forward to it.
The audit subcommand's discover path called `resolve_provider_name("discover", explicit=...)` and `get_discovery_provider(name, config)`, but neither function accepts those extra arguments. Both calls raised TypeError at runtime, so `king-scrape audit <name>` (with discover enabled) crashed before any URL was fetched.
The `--no-discover` and "corpus has no base_url" paths return earlier and never reach this call site, which is why existing tests (which mocked `_discover_fresh_urls` wholesale) passed and the bug shipped with #49.
Aligns the audit call site with the pattern already used in `scraper/cli.py:126` (`get_discovery_provider(resolve_provider_name("discover"))`). The `load_config()` import becomes unused and is removed.
Pure mechanical fix, no behaviour change in the paths that already worked.
Refs #55.
Summary
king-scrape audit <name>. Read-only. Walks corpus URLs. Reports fresh / moved / broken / throttled / auth_required / unreachable. Optional discover diff (--no-discoverto skip) reports new + orphan URLs. Markdown report to.king-context/audit/<name>-<ts>.md. Exit 2 on any broken URL, CI-gateable. Never mutates corpus or DB.Type of change
How it was tested
pytest -q
702 passed, 1 skipped
20 tests in
tests/test_scraper/test_audit.py:Retry-AfterHaven't run against a live corpus yet. Can pair with PR #N's audit step or run on
data/openrouter.json/data/elevenlabs-api.jsonif you want a live demo before merge.Checklist
pytest)docs/and/orREADME.mdif behavior changed (no public-surface change beyond the new subcommand; ADR-0013 added; CHANGELOG updated)Additional notes
A few choices worth flagging:
if argv[1] == "audit":branch incli.main. No argparse subparser refactor. Keeps the existingking-scrape <url>flow byte-for-byte unchanged.Establishes a precedent that ADR-0014's
updatecan follow the same way without touching the existing parser. Documented in ADR-0013 with the alternatives.entries for purely cosmetic variations (e.g.
/pagevs/page/vs/page#install). The original URL is preserved in the report — contributors see what's actually in their corpus.follow_redirects=True+response.history. A301 → 301 → 200chain reportsmovedwith the final URL, not the first hop. A chain that ends in404reportsbroken, which is what a contributor wants.unreachable. RespectsRetry-After(capped at 30s), retries once, then classifies. Keeps a noisy upstream from masquerading as broken.auth_required. A page behind auth still exists; lumping it with timeouts would be misleading.exceptis narrowed to(httpx.RequestError, RuntimeError, ImportError, OSError). A bug-class exception (TypeError, AttributeError) propagates so a typo in providercode can't silently masquerade as "discovery skipped".
_meta.content_hashjust fine. Once #N lands, the audit can extend to hash-based driftdetection without restructuring — that's deferred to ADR-0014's
--updatework where it composes naturally.Follow-up I'd send next, if you're open to it: ADR-0014 +
king-scrape <url> --updatefor the incremental refresh that closes the loop on this stack. Separate PR.No rush. Happy to pair with you on a live audit before merge if useful.