Sanitize URLSource HTML and recover PDF titles

[caufieldjh]

URLSource is the one source that did no extraction. It stored the response body as fetched, and it named every PDF after its own URL. dismech carries a runtime patch for each problem. These are the last two patches it carries.

Closes #92. Closes #93.

#93: a PDF fetched by URL gets a real title

The PDF branch now tries three things in order:

1. The landing page's citation_title. The page is found by a rule in LANDING_PAGE_RULES. There is one rule, for J-STAGE: .../_pdf[/-char/ja] becomes .../_article[/-char/ja]. That covers the 53 dismech entries.
2. The PDF's embedded /Title. Placeholders are rejected: Microsoft Word - x.doc, bare filenames, Untitled.
3. The URL. This fallback is kept on purpose, so title == url still means no title was found.

A page's bare <title> is not used at step 1, because it is often the site's name. On ordinary HTML fetches, citation_title now wins over <title> too.

#92: HTML is sanitized before caching

sanitize_html removes these:

• script, style, noscript and template elements, and all comments.
• meta, link and base. They hold nothing but attributes.
• Every attribute except rowspan, colspan and scope.

Body markup and text are kept. Plain text and XML are stored as fetched. The title is read before sanitizing, because citation_title lives in a <meta> attribute. A second pass gives the same output, and whitespace inside <pre> is left alone.

This is kept apart from HTMLExtractor. That extractor flattens a page to plain text, and url: pages are stored as HTML.

Old entries are refetched once

Neither fix changes entries already on disk. Those entries carry a current extractor_version, so they would be served as they are forever. url: entries now carry url_source_version: 1. It is written only on a fresh fetch, the way html_full_text_version is. An entry with no stamp, or an older one, is stale and is fetched again once. If the fetch fails, the old entry is still served and is not rewritten. Other sources are unaffected. With trust_cached_entries set, nothing is refreshed.

For dismech, expect one diff on every url: entry in the cache: a stamp line, and sanitized bodies on the HTML pages.

One existing test changed. test_entry_whose_id_contains_a_horizontal_rule_is_not_perpetually_stale saved a url: entry by hand with no stamp, and under the new rule that entry is stale. The test entry now carries the stamp a real fetch writes. The stamp line sits below reference_id, so the test still checks the frontmatter truncation it was written for.

Commits

Each commit passes its own tests.

1. Read a PDF's embedded /Title in PDFExtractor
2. Prefer citation_title over <title> for URL pages
3. Recover a title for PDFs fetched by URL (A PDF URL cached by URLSource gets the URL as its title #93)
4. Sanitize HTML fetched by URLSource before caching (URLSource caches raw page HTML: scripts, comments and attributes reach a committed cache #92)
5. Re-fetch url: entries written before the URLSource fixes
6. Docs

Not done

• Crossref by DOI. Finding a DOI in the PDF and asking Crossref for the title would cover publishers with no landing-page rule. It is not here.
• Other empty tags. <img>, <iframe> and similar tags lose their attributes but stay in the page as empty tags.
• Parsing twice. The PDF bytes are parsed once for text and once for the title.

Tests

• 45 new tests in test_pdf_url_title.py, test_url_html_sanitize.py and test_url_source_cache_version.py, plus doctests.
• Full just test: 1223 passed, 10 failed. One failure was the horizontal-rule test above, and it now passes.
• The other nine are in test_pmid_network.py and test_html_fulltext_acceptance.py. They time out connecting to a local test server at 127.0.0.1. A different set fails on each run, and they fail on main too.
• The full suite ran before meta/link/base were added. After that change I reran the targeted tests at each of the last three commits: 120 passed each time.
• mypy clean, ruff clean.

🤖 Generated with Claude Code

[claude]

Claude finished @caufieldjh's task in 1m 25s —— View job

---

Review of PR #98: Sanitize URLSource HTML and recover PDF titles

• Read the source diff
• Check the surrounding code: acquirer, cache staleness, PDF backends
• Check the tests
• Post the review

The PR is well scoped and each part is documented. The staleness stamp follows the same pattern as html_full_text_version, and the tests cover the cases that matter. I found one real bug to fix before merge. The other notes are small.

---

🔴 The landing-page request can raise and lose a PDF that was already fetched

src/linkml_reference_validator/etl/sources/url.py:133-140, in _recover_pdf_title:

page, content_type = ContentAcquirer().fetch_bytes(landing, config)

fetch_bytes calls requests.get(..., timeout=60). That call raises on a timeout, a connection error or a TLS error. It does not return (None, None) in those cases. Nothing in URLSource.fetch or in ReferenceFetcher.fetch_with_provenance (reference_fetcher.py:385) catches the exception. So if the J-STAGE landing page is slow or unreachable, the fetch fails outright, even though the PDF bytes and text were already in hand. The title is optional, so it should never cost the content.

Elsewhere the codebase treats fetch_bytes as an external boundary and wraps it: reference_fetcher.py:941-945 and :755-759 both use except Exception and log a warning. This new call is the only unwrapped one on a best-effort path. CLAUDE.md allows try/except at external boundaries, and this is one.

Suggested fix:

try:  # external system boundary: the title is best-effort, the PDF is not
    page, content_type = ContentAcquirer().fetch_bytes(landing, config)
except requests.RequestException as e:
    logger.debug(f"Landing page {landing} failed for {url}: {e}")
    page = None

Add a test in which fetch_bytes has side_effect=[(pdf_bytes, "application/pdf"), requests.Timeout()], and assert that the PDF text is still returned with the embedded title or the URL as its title.

Fix this →

🟡 "Still served when offline" only covers a None return

test_old_raw_entry_is_still_served_when_offline (tests/test_url_source_cache_version.py:113) simulates being offline with fetch_bytes → (None, None). When a machine is really offline, requests raises ConnectionError, and URLSource.fetch does not catch it. The exception propagates, and _stale_fallback is never reached.

This gap existed before the PR. The PR makes it matter more: once it lands, every unstamped url: entry in dismech goes back to the network. Before, those entries were served from disk with no request. So the first offline or sandboxed run after upgrading will crash on url: references, instead of serving the stale entry as the PR description says it will. Two ways to fix it:

• Catch requests.RequestException around the main fetch_bytes in URLSource.fetch and return None. clinicaltrials.py:124 already does this.
• Or make the test raise ConnectionError, so it proves what its name says.

🟢 Smaller notes

• The meta regexes can over-match (url.py:35-36). \bname\s*= also matches data-name="citation_title", because - counts as a word boundary. \bcontent\s*= also matches data-content=. Anchoring on whitespace fixes both: (?:^|\s)name\s*= and (?:^|\s)content\s*=. This is unlikely to happen in practice but cheap to fix.
• HTML detection has gaps (url.py:113). A page served with text/plain or no content type is only sanitized if it starts with <!doctype html or <html>. Pages that start with a comment, a BOM or <head> are cached raw. sniff_format already strips leading whitespace, but a BOM and a leading <!-- are not handled. It may be worth widening the check, or noting the limit in the docs.
• PypdfBackend.extract_title only catches PyPdfError (pdf.py:53-57). pypdf can raise other errors on malformed metadata, such as KeyError, ValueError or TypeError when decoding an odd /Title. Before this PR, a PDF whose text extracted cleanly could not fail on its title. Now it can. A wider except here fits the external-data comment already on that line.
• <noscript> and <template> are removed with their text. This is reasonable for scripts. Some publisher pages put the article abstract inside <noscript> as a fallback for readers without JavaScript. If a quote was validated against that text before, it will stop matching after the refetch. This is worth a line in docs/troubleshooting.md.
• _PLACEHOLDER_TITLE rejects any title ending in .pdf, .tex and similar (pdf.py:64-67). This is fine in practice. \S+\.(pdf|…) would reject only bare filenames, not a real title that happens to end that way.
• Performance: J-STAGE PDFs now take two requests, so two rate_limit_delay sleeps. The refresh also costs one fetch per url: entry. Both are expected and acceptable. The double parse of the PDF is already listed under "Not done".
• The PDFTextBackend Protocol gained extract_title. Only pypdf is registered today. Any third-party backend will now need this method, so it could go in the changelog.

Security

Removing script, style, meta, link and base and all non-table attributes does what #92 asks: signed URLs and API keys in scripts and src/href stop reaching committed caches. The landing-page URL is built only from a matched https?://www.jstage.jst.go.jp/... pattern, so the rewrite cannot be pointed at another host. I have no other concerns.

Test coverage

The coverage is good: stamp round-trip, fresh fetch vs. save, stale refetch, idempotent sanitize, <pre>, placeholder titles. Two gaps: the exception paths above, and a data-name=/data-content= case for _citation_title.

I could not run the tests here, because the sandbox did not allow uv run pytest. This review is from reading the code only.

[claude]

Claude finished @caufieldjh's task in 1m 34s —— View job

---

Re-review of PR #98 after the new commits

• Read the new commits (the follow-ups to the last review, and the rules module)
• Check the source changes
• Check the tests
• Post the review

The follow-up commits fix every blocking item from the last review, each with a test. I found one new behaviour worth deciding on before merge (🟡). Everything else is small.

Fixed since the last review

Item	Fix	Test
Landing-page request can lose the PDF	_landing_page_title catches RequestException (url.py:599-604)	test_landing_page_error_keeps_the_pdf (Timeout and ConnectionError)
An offline machine raises instead of serving the stale entry	main fetch_bytes is wrapped and returns None, so _stale_fallback runs (url.py:87-91)	test_old_raw_entry_is_still_served_when_offline now raises for real
data-name= / data-content= over-match	lookbehind (?<=\s)	test_citation_title_ignores_look_alike_attributes
HTML detection missed a BOM or leading comments	_looks_like_html skips both and accepts <head>/<body>	doctests plus a parametrized test
Non-text /Title	isinstance(title, str)	test_extractor_ignores_a_title_that_is_not_text
Any title ending in .pdf was rejected	`\S+.(pdf	…)`
<noscript> text is dropped	noted in docs/troubleshooting.md	—

tests/test_pmc_modern_container.py moved into rules.PMC_ARTICLE_BODY_EXAMPLES / _COUNTEREXAMPLES with no loss. I checked all 8 cases, including the two bot-check <body> cases.

---

🟡 The one-time refetch can replace a good url: entry with whatever the site serves today

Until now, url: entries were never fetched again. With this PR every unstamped entry is fetched once. _preserve_cached_full_text only refuses a refresh when the fresh result's type is in NEEDS_FULL_TEXT_TYPES (reference_fetcher.py:533-539, _refresh_loses_full_text). An HTML refetch always comes back as content_type="url", so it is always written. That includes these cases:

• a bot-check or consent page served with HTTP 200, which Akamai, PerimeterX and some publisher paywalls all do;
• a domain that has lapsed and now serves a parking page;
• a page whose wording has changed since a curator validated a quote against it.

In each case the cached body that quotes were validated against is replaced. The only signal is _report_shrinking_refresh, which is a log line. The PR description says "expect one diff on every url: entry: a stamp line, and sanitized bodies." Some of those diffs could be content changes or interstitials, and they would be easy to miss in review.

Two ways to handle it:

• Cheapest: state this in the PR description and in the dismech upgrade notes, so whoever reviews the cache diff checks for text changes and not just stripped attributes.
• Safer: for a stale url: HTML entry, sanitize the cached body in place and stamp it, instead of refetching. URLSource caches raw page HTML: scripts, comments and attributes reach a committed cache #92 needs no network at all. Only PDF entries whose title == reference_id[4:] need a fetch to recover the title (A PDF URL cached by URLSource gets the URL as its title #93). The migration would then change only what the fixes are meant to change. For HTML, the refetch would stop recovering citation_title for old pages, but a cached page that still has its <meta> tags can have its title read in place too.

Fix this →

🟢 Smaller notes

• PypdfBackend.extract_title still catches only PyPdfError (pdf.py:115-119). Resolving /Info on a malformed trailer can raise KeyError, ValueError or AttributeError in some pypdf versions. The same bytes have already produced text, so if this raises, a PDF that fetched fine is lost over its optional title. except Exception, with the external-data comment already there, fits the CLAUDE.md boundary rule.
• _looks_like_html also accepts <header> (head[:5] == b"<head"). This is harmless, since a body that starts with <header> is an HTML fragment anyway. If it is meant, a doctest would record that.
• The J-STAGE rule ignores a query string. …/_pdf?download=1 fails (/.*)?$ and falls through to the embedded title. This is fine unless dismech has such URLs.
• Scope and PR description. rules.py also moves the PMC container classes, the article-body selectors and the stub-notice phrases, which have nothing to do with URLSource caches raw page HTML: scripts, comments and attributes reach a committed cache #92 or A PDF URL cached by URLSource gets the URL as its title #93. The refactor is sound and test_consumers_import_the_rules guards against drift. But the description still lists 6 commits (there are 14) and does not mention rules.py or the deleted test file. Please update it so a reviewer knows that change is there.
• The rules.py docstring says each rule sits beside its "examples … and its counterexamples", but STUB_NOTICE_PHRASES has no counterexamples there. Its counterexamples, such as the Morris 2019 "restricts … to" case, stay with the length-gate tests in xml.py. One sentence pointing there would stop someone adding a counterexample to rules.py and expecting test_rules.py to run it.

Performance

Nothing new since the last review: two requests and two rate_limit_delay sleeps per J-STAGE PDF, one refetch per url: entry, and the PDF parsed twice. All are acceptable. If you adopt the in-place migration above, most of the refetch cost goes away.

Security

sanitize_html removes script/style/noscript/template/meta/link/base, all comments, and all attributes except rowspan/colspan/scope. That covers src, href, srcdoc, data-* and on*, so signed URLs and keys do not reach the cache. The landing-page rewrite is anchored to https?://www.jstage.jst.go.jp/, and LANDING_PAGE_COUNTEREXAMPLES pins the other-host case. I have no concerns.

Test coverage

Coverage is good. Every earlier gap now has a test, and test_rules.py requires an example for every rule. One gap is left: nothing tests a refetch that returns a 200 interstitial for an old url: entry (the 🟡 above). Whichever way that is decided, a test should record the choice.

I couldn't run the test suite, because the sandbox needs approval for uv run pytest. This review comes from reading the code only.

[caufieldjh]

Thanks for the review. Each point is its own commit.

Commit	Point	Change
5a29717	🔴 landing page can raise	_landing_page_title catches requests.RequestException. The PDF keeps its text and falls back to the embedded /Title, then the URL. Tests cover Timeout and ConnectionError.
a362019	🟡 offline only covered None	URLSource.fetch returns None on a request error, as clinicaltrials.py does, so _stale_fallback serves the old entry. The offline test now raises ConnectionError and Timeout, and still covers (None, None).
55b17a7	meta regexes over-match	name and content match only after whitespace, so data-name= and data-content= are ignored. Tested.
d88cbdc	HTML detection gaps	URLSource._looks_like_html skips a UTF-8 BOM and leading comments, and accepts <head> and <body>. sniff_format is unchanged, since other paths use it to pick a format. Text that only mentions a tag is still left alone.
5e617d5	wider except in extract_title	Not widened. I probed pypdf with number, array, dictionary and bytes /Title values and with a non-dictionary /Info. Nothing raised outside PyPdfError, which is already caught. The probe found a different bug: pypdf returns /Title as whatever object it holds, and str() turned a number or array into the title "5" or "['a']". Only text is accepted now.
6b37bfd	_PLACEHOLDER_TITLE too broad	Took your suggestion. Only a bare filename is rejected. The trade: a filename with spaces, such as final draft v2.pdf, now passes as a title.
245add2	<noscript> text	Added a note to docs/troubleshooting.md. A quote that matched only a <noscript> fallback stops matching after the refetch.

No change:

• Performance. Agreed, and accepted as you said.
• PDFTextBackend.extract_title. The repo has no changelog. The Protocol change is recorded in the commit message of 4299d79.

Also in this push, 4c69591. Publisher and site rules now live in etl/rules.py: LANDING_PAGE_RULES, PDF_PLACEHOLDER_TITLE, ARTICLE_BODY_SELECTORS, PMC_ARTICLE_BODY_CLASSES and STUB_NOTICE_PHRASES. Each sits beside examples it must match and counterexamples it must not. tests/test_rules.py runs them through the code that uses each rule, checks that each consumer imports the rule and not a copy, and fails for a rule with no example. test_pmc_modern_container.py only restated the PMC rule, so its cases moved into rules.py and the file is gone.

Tests. Locally, the targeted run over the changed areas passed 411 of 411, and mypy and ruff are clean. The full suite could not finish on my machine: tests that make real network calls stall there on connection timeouts. CI is the full check for this push.