fix(ingestion): include source URL in extraction fingerprint - #742
Conversation
A changed registry URL was still treated as the same extraction, so stored validators could be sent to the new address and a 304 reported MATCH. The fingerprint now includes the URL, and --check reports drift until --sync re-extracts.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Update extraction fingerprinting to include the source URL so validators (ETag/Last-Modified) are not reused when a registry URL changes, and add tests to validate the new behavior.
Changes:
- Include
urlinextraction_fingerprint()identity calculation and update documentation/comments. - Add tests ensuring URL changes trigger unconditional refetch, drift detection, and file rewrite during
--sync. - Update
sources.yamlinline documentation to reflect the new fingerprint definition.
| File | Description |
|---|---|
| app/scripts/sync_sources.py | Extends extraction fingerprint to include URL and clarifies intent in docstrings/comments. |
| app/tests/test_sync_sources.py | Adds regression tests for URL-change behavior in --check and --sync. |
| app/data/sources.yaml | Updates schema comments to document URL-included fingerprint behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def extraction_fingerprint(source: SourceEntry) -> str: | ||
| """Identity of the inputs that turn upstream HTML into the stored Markdown.""" | ||
| """Identity of the page and the inputs that turn its HTML into stored Markdown. | ||
|
|
||
| ``url`` is part of the identity. Validators stored for one page must not be | ||
| sent to a different address after the registry URL changes. | ||
| """ | ||
| raw = "\n".join(( | ||
| EXTRACTOR_VERSION, | ||
| source.type or "", | ||
| source.selector or "", | ||
| source.part or "", | ||
| source.url, | ||
| )) | ||
| return hashlib.sha256(raw.encode("utf-8")).hexdigest() |
| source.type or "", | ||
| source.selector or "", | ||
| source.part or "", | ||
| source.url, |
| result = check_source_drift(entry, tmp_path) | ||
|
|
||
| assert result.status == "DRIFT_DETECTED" | ||
| assert result.status != "MATCH" |
Code Review Roast 🔥Verdict: No Issues Found | Recommendation: Merge Oh wait, this PR is actually clean. I need to sit down. I had my flamethrower warmed up and everything. The fingerprint change is wired in correctly on every path that matters: 📊 Overall: Like finding a unicorn in production — I didn't think clean PRs existed anymore, but here we are. Files Reviewed (3 files)
Reviewed by efficient · Input: 0 · Output: 0 · Cached: 0 |
Summary
extraction_fingerprintsigned extractor version, type, selector, and part, and left outsource.url. After a registry URL change, the local file could still matchcontent_hash, so--checkand--syncsent the previous page'sIf-None-MatchandIf-Modified-Sinceto the new address. A 304 then reported MATCH for a page that was never fetched.The fingerprint now includes the URL. A changed URL is an unconditional fetch.
--checkreportsDRIFT_DETECTEDwithExtraction inputs changedwhen the extracted hash still matches, and--syncrewrites the file and stores a new fingerprint. Catalogue entries onmaindo not have a stored fingerprint yet, so this does not change the next scheduled--checkuntil a sync writes one.Test plan
python:3.14-slim):pytest tests/test_sync_sources.py --noconftest— 74 passed--syncrewrites the file with the new URL and replaces the fingerprint