From 63236b1bb3463a6c5eae5d6f6b178be9e8c7b4f2 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Fri, 25 Sep 2026 18:31:04 -0700 Subject: [PATCH 1/2] Address review on #94: test the path consumers actually take Five follow-ups from the review, plus the coverage gap that mattered. `test_serve_stale_html_optin.py` only covered `_stale_fallback`, which needs the source to yield *nothing*. For a PMC-only article PubMed still returns the abstract, so the refresh reaches `_preserve_cached_full_text` instead: the overwrite is refused, and a separate question decides what may be served. That second question is the one the flag exists to answer and the one dismech hits, and it had no test. Verified before writing them, in case the flag did not reach that path -- it does: with the flag on the cached body is served, with it off the fresh abstract is, and the file on disk is unchanged either way. So this was missing coverage rather than a defect, and there are now three tests pinning it. The warning in `_load_from_disk` still told users to retry, which for a PMC-only article is advice they cannot act on -- PMC answers a Python HTTP client with a bot-check interstitial on an HTTP 200. It now says a re-fetch repairs the entry where the source still serves full text, says that does not include PMC-only articles, and names `serve_stale_html_full_text` so the next person finds the option from the log line instead of from a merged PR. Also: - The comment above the second call site said stale HTML is "withheld here exactly as _stale_fallback withholds it". True only by default now, and the comment did not say which path is the common one. It does. - `find_pmc_article_body` loses its underscore and is imported at module level. It is shared by two modules, and a function-local import of a private name read like a circular-import workaround, which it was not. - `PMC_ARTICLE_BODY_CLASSES` records that its order is a priority order. For a legacy page with several `tsec` sections only the first is returned -- the previous code did the same, so it is a known limit, but reordering the tuple would silently change which section that is. - A negative test for a challenge page carrying a classed ``, since that markup is what the selector has to keep out of the cache. Whether to keep the PMC HTML fallback at all is filed separately as #95: it cannot succeed from a Python HTTP client, yet sits in the default provider chain costing a rate-limit delay and a request per reference. just test: 1168 passed, 1 skipped; mypy clean; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016WUDZDTqMP5ih9rD7QqGJ4 --- .../etl/fulltext/pmc.py | 14 +++-- .../etl/reference_fetcher.py | 16 +++-- .../etl/sources/pmid.py | 8 +-- tests/test_pmc_modern_container.py | 24 ++++++-- tests/test_serve_stale_html_optin.py | 59 +++++++++++++++++++ 5 files changed, 102 insertions(+), 19 deletions(-) diff --git a/src/linkml_reference_validator/etl/fulltext/pmc.py b/src/linkml_reference_validator/etl/fulltext/pmc.py index 360a49f..6d1c304 100644 --- a/src/linkml_reference_validator/etl/fulltext/pmc.py +++ b/src/linkml_reference_validator/etl/fulltext/pmc.py @@ -46,10 +46,16 @@ class TransientFullTextError(RuntimeError): #: same element. Matching it alone would match every page's ````, and the #: structural test is the only thing standing between this fetch and caching a #: bot-check interstitial served on an HTTP 200. +#: +#: Order is a priority order, not an alphabetical one: the first match wins, so +#: the current wrapper is tried before the legacy ones. It matters for a legacy +#: page carrying several ``tsec`` sections, where only the first is returned -- +#: the previous code behaved the same way, so this is a known limit rather than +#: a regression, but reordering the tuple would change which section that is. PMC_ARTICLE_BODY_CLASSES = ("main-article-body", "article-body", "tsec") -def _find_pmc_article_body(soup: BeautifulSoup) -> Optional[Any]: +def find_pmc_article_body(soup: BeautifulSoup) -> Optional[Any]: """Return PMC's article-body element, or None if the page has none. Matches on class rather than element name: the container moved from ``div`` @@ -65,9 +71,9 @@ def _find_pmc_article_body(soup: BeautifulSoup) -> Optional[Any]: Examples: >>> from bs4 import BeautifulSoup >>> html = '

Text.

' - >>> _find_pmc_article_body(BeautifulSoup(html, "html.parser")) is not None + >>> find_pmc_article_body(BeautifulSoup(html, "html.parser")) is not None True - >>> _find_pmc_article_body(BeautifulSoup("

x

", "html.parser")) is None + >>> find_pmc_article_body(BeautifulSoup("

x

", "html.parser")) is None True """ for class_name in PMC_ARTICLE_BODY_CLASSES: @@ -192,7 +198,7 @@ def _fetch_pmc_html(self, pmcid: str, config: ReferenceValidationConfig) -> Opti ) soup = BeautifulSoup(response.content, "html.parser") - article_body = _find_pmc_article_body(soup) + article_body = find_pmc_article_body(soup) if article_body: # The region is selected here, but the text comes out of the shared # extractor rather than a private copy of the paragraph walk, so diff --git a/src/linkml_reference_validator/etl/reference_fetcher.py b/src/linkml_reference_validator/etl/reference_fetcher.py index 5724d7d..4e42527 100644 --- a/src/linkml_reference_validator/etl/reference_fetcher.py +++ b/src/linkml_reference_validator/etl/reference_fetcher.py @@ -521,9 +521,12 @@ def _preserve_cached_full_text( cached.content_type, ) - # May its text be served? Asked without the bypass, so stale HTML is - # withheld here exactly as _stale_fallback withholds it, and validation - # falls back to the freshly fetched abstract. + # May its text be served? Asked with the consumer's serve policy, the + # same one _stale_fallback uses. By default stale HTML is withheld and + # validation falls back to the freshly fetched abstract; a consumer that + # has opted in gets the cached body instead. This is the path a refresh + # normally takes -- the source still answers, with an abstract -- so it + # is the one most consumers see, not _stale_fallback. servable = self._load_from_disk( normalized_reference_id, allow_stale=True, @@ -1536,8 +1539,11 @@ def _load_from_disk( ): logger.warning( "Refusing stale HTML full text for %s: it may be a repository " - "landing page. Retry when the source serves full text again to " - "repair it.", + "landing page. A re-fetch repairs it where the source still " + "serves full text -- but not for a PMC-only article, which " + "answers a Python HTTP client with a bot-check interstitial on " + "an HTTP 200. Set serve_stale_html_full_text if this cache is " + "one you review.", reference_id, ) return None diff --git a/src/linkml_reference_validator/etl/sources/pmid.py b/src/linkml_reference_validator/etl/sources/pmid.py index d2439a3..fff79e4 100644 --- a/src/linkml_reference_validator/etl/sources/pmid.py +++ b/src/linkml_reference_validator/etl/sources/pmid.py @@ -23,6 +23,7 @@ from bs4 import BeautifulSoup # type: ignore import requests # type: ignore +from linkml_reference_validator.etl.fulltext.pmc import find_pmc_article_body from linkml_reference_validator.models import ReferenceContent, ReferenceValidationConfig from linkml_reference_validator.etl.extract.html import HTMLExtractor from linkml_reference_validator.etl.extract import MIN_FULLTEXT_CHARS @@ -582,12 +583,7 @@ def _fetch_pmc_html( return None soup = BeautifulSoup(response.content, "html.parser") - # Shared with the PMC full-text provider: the container moved from a - # ``div`` to a ``section`` and changed class, so both call sites went - # blind at once (dismech#12672). - from linkml_reference_validator.etl.fulltext.pmc import _find_pmc_article_body - - article_body = _find_pmc_article_body(soup) + article_body = find_pmc_article_body(soup) if article_body: # Region selected here, text extracted by the shared extractor, for diff --git a/tests/test_pmc_modern_container.py b/tests/test_pmc_modern_container.py index 8944c5d..00ace8a 100644 --- a/tests/test_pmc_modern_container.py +++ b/tests/test_pmc_modern_container.py @@ -18,7 +18,7 @@ import pytest from bs4 import BeautifulSoup -from linkml_reference_validator.etl.fulltext.pmc import _find_pmc_article_body +from linkml_reference_validator.etl.fulltext.pmc import find_pmc_article_body BODY = "

" + ("Real article prose. " * 40) + "

" @@ -34,7 +34,7 @@ ) def test_article_containers_are_found(markup, label): soup = BeautifulSoup(markup, "html.parser") - assert _find_pmc_article_body(soup) is not None, f"{label} should be recognised" + assert find_pmc_article_body(soup) is not None, f"{label} should be recognised" @pytest.mark.parametrize( @@ -49,11 +49,27 @@ def test_article_containers_are_found(markup, label): ) def test_pages_without_an_article_container_are_declined(markup, label): soup = BeautifulSoup(markup, "html.parser") - assert _find_pmc_article_body(soup) is None, f"{label} must not be accepted" + assert find_pmc_article_body(soup) is None, f"{label} must not be accepted" def test_a_bare_body_element_is_not_an_article_container(): """```` is on every page, so matching class="body" alone would accept an interstitial. Only the article-body classes count.""" soup = BeautifulSoup(f"{BODY}", "html.parser") - assert _find_pmc_article_body(soup) is None + assert find_pmc_article_body(soup) is None + + +def test_a_challenge_page_carrying_a_classed_body_is_still_declined(): + """The interstitial arrives on an HTTP 200, so this selector is the only check. + + PMC's challenge page carries a classed ````. None of its classes may + overlap the article-body list, or a bot-check page gets cached as full text. + """ + markup = ( + '' + "

Checking your browser before accessing

" + "

Enable JavaScript and cookies to continue.

" + "" + ) + soup = BeautifulSoup(markup, "html.parser") + assert find_pmc_article_body(soup) is None diff --git a/tests/test_serve_stale_html_optin.py b/tests/test_serve_stale_html_optin.py index d0a587d..7c7dadc 100644 --- a/tests/test_serve_stale_html_optin.py +++ b/tests/test_serve_stale_html_optin.py @@ -22,6 +22,7 @@ from linkml_reference_validator.etl.reference_fetcher import ReferenceFetcher from linkml_reference_validator.models import ( + ReferenceContent, ReferenceValidationConfig, ) @@ -73,3 +74,61 @@ def test_opting_in_does_not_stamp_or_rewrite_the_entry(tmp_path): text = (tmp_path / "PMID_30929739.md").read_text(encoding="utf-8") assert "extractor_version" not in text assert BODY_SENTENCE in text + + +def _fetch_with_abstract_source(tmp_path, **cfg): + """The path a refresh normally takes: the source answers, with an abstract. + + ``_stale_fallback`` needs the source to yield *nothing*. For a PMC-only + article PubMed still returns the abstract, so the refresh reaches + ``_preserve_cached_full_text`` instead: the overwrite is refused, and then a + separate question decides what may be served. That second question is the + one the flag has to answer, and it is the one dismech actually hits. + """ + _cache(tmp_path) + fetcher = ReferenceFetcher( + ReferenceValidationConfig( + cache_dir=tmp_path, email="me@example.org", fetch_full_text=False, **cfg + ) + ) + fresh = ReferenceContent( + reference_id="PMID:30929739", + content="Just the abstract.", + content_type="abstract_only", + title="A paper", + ) + + class _Source: + def fetch(self, identifier, config): + return fresh + + with patch( + "linkml_reference_validator.etl.reference_fetcher" + ".ReferenceSourceRegistry.get_source", + return_value=_Source, + ): + return fetcher.fetch("PMID:30929739") + + +def test_refused_overwrite_serves_the_abstract_by_default(tmp_path): + content = _fetch_with_abstract_source(tmp_path) + assert content is not None + assert BODY_SENTENCE not in (content.content or ""), ( + "by default the cached body stays withheld even though it was preserved" + ) + assert "Just the abstract." in (content.content or "") + + +def test_refused_overwrite_serves_the_cached_body_when_opted_in(tmp_path): + content = _fetch_with_abstract_source(tmp_path, serve_stale_html_full_text=True) + assert content is not None + assert BODY_SENTENCE in (content.content or "") + + +def test_refused_overwrite_leaves_the_file_alone_either_way(tmp_path): + """Serving is not overwriting: the entry keeps its body and stays un-stamped.""" + for flag in (False, True): + _fetch_with_abstract_source(tmp_path, serve_stale_html_full_text=flag) + text = (tmp_path / "PMID_30929739.md").read_text(encoding="utf-8") + assert BODY_SENTENCE in text, f"body lost with flag={flag}" + assert "extractor_version" not in text, f"entry stamped with flag={flag}" From 4c824cdebbef1695f17d82741d01a71d01c39a31 Mon Sep 17 00:00:00 2001 From: Chris Mungall Date: Fri, 25 Sep 2026 23:25:58 -0700 Subject: [PATCH 2/2] Do not cache a bot-check page as a reference PMC and Cloudflare answer a Python HTTP client with a challenge page carried on an HTTP 200, so no status check sees it. `URLSource` took its `` as the reference title and its markup as the content. Downstream that surfaced as [ERROR] Title mismatch: expected 'Pharmacotherapy for Alcohol Use Disorder...' but got 'Checking your browser' on a pull request citing PMC articles by URL (monarch-initiative/dismech#12867). Those URLs were not cached, so validation fetched them live and compared a curated title against an interstitial. `_is_bot_challenge` now treats such a response as a transient failure rather than an absence: nothing about the reference is known, so a retry from an unblocked network can still succeed, whereas caching it records an interstitial as a paper. The log line says so and suggests citing by identifier instead. The false-positive direction is the one that matters, because a paper *about* CAPTCHAs contains these words and discarding a real reference is worse than caching a challenge page. So the check fires on the `<title>`, or on a body under 60 KB -- PMC's challenge is ~21 KB against 150-240 KB for an article, so the margin is wide. Negative tests cover a real PMC article, an article whose subject is bot detection, and plain text. Verified against the live response rather than a fixture: PMC returns 200 with 21,207 characters titled "Checking your browser - reCAPTCHA", and the guard recognises it. This does not make PMC reachable. It stops a page that is not the reference being stored as though it were. just test: 1174 passed, 1 skipped; mypy clean; ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --- .../etl/sources/url.py | 68 +++++++++++++++++ tests/test_url_bot_challenge.py | 74 +++++++++++++++++++ 2 files changed, 142 insertions(+) create mode 100644 tests/test_url_bot_challenge.py diff --git a/src/linkml_reference_validator/etl/sources/url.py b/src/linkml_reference_validator/etl/sources/url.py index 384e945..00fa6c3 100644 --- a/src/linkml_reference_validator/etl/sources/url.py +++ b/src/linkml_reference_validator/etl/sources/url.py @@ -92,6 +92,19 @@ def fetch( ) content = self._decode(data, content_type_header) + + if self._is_bot_challenge(content): + # An HTTP 200 carrying a challenge page. Nothing about the reference + # is known, so this is a transient failure, not an absence: caching + # it would record an interstitial's <title> as the paper's title. + logger.warning( + "Bot-check page returned for %s; not caching it. Retry from an " + "unblocked network, or cite the record by identifier (PMID, DOI) " + "rather than by URL.", + url, + ) + return None + title = self._extract_title(content, url) return ReferenceContent( @@ -120,6 +133,61 @@ def _decode(self, data: bytes, content_type: str) -> str: except LookupError: return data.decode("utf-8", errors="replace") + #: Phrases a bot-check page carries. Matched against the ``<title>``, or + #: against the whole document only when it is too short to be an article -- + #: a paper *about* CAPTCHAs contains these words in its prose, and + #: discarding it would be worse than caching an interstitial. + _BOT_CHALLENGE_PHRASES = ( + "checking your browser", + "just a moment", + "unusual traffic", + "verifying you are human", + "enable javascript and cookies", + "please verify you are a human", + ) + + #: A challenge page is small. PMC's is ~21 KB against ~150-240 KB for an + #: article, so this is well clear of both. + _BOT_CHALLENGE_MAX_CHARS = 60_000 + + @classmethod + def _is_bot_challenge(cls, content: str) -> bool: + """Is this a bot-check interstitial rather than the requested document? + + PMC and Cloudflare both serve these on an HTTP 200, so the status code + cannot distinguish them and the title becomes the reference title + (dismech#12867: "expected 'Pharmacotherapy for Alcohol Use Disorder...' + but got 'Checking your browser'"). + + Deliberately conservative -- it fires on the title, or on a body too + short to be an article. A real paper discussing bot detection is longer + than the cap and does not carry the phrase in its title. + + Args: + content: The decoded response body + + Returns: + True if the response looks like a challenge page + + Examples: + >>> URLSource._is_bot_challenge( + ... "<title>Checking your browser before accessing") + True + >>> URLSource._is_bot_challenge("A real paper" + "x" * 70000) + False + """ + lowered = content.lower() + title_match = re.search(r"]*>([^<]*)", lowered, re.IGNORECASE) + if title_match and any( + phrase in title_match.group(1) for phrase in cls._BOT_CHALLENGE_PHRASES + ): + return True + if len(content) <= cls._BOT_CHALLENGE_MAX_CHARS and any( + phrase in lowered for phrase in cls._BOT_CHALLENGE_PHRASES + ): + return True + return False + def _extract_title(self, content: str, url: str) -> str: """Extract title from HTML content or use URL. diff --git a/tests/test_url_bot_challenge.py b/tests/test_url_bot_challenge.py new file mode 100644 index 0000000..be00902 --- /dev/null +++ b/tests/test_url_bot_challenge.py @@ -0,0 +1,74 @@ +"""A bot-check page is not a reference, and must not become one. + +PMC answers a Python HTTP client with a challenge page carried on an HTTP 200, +so no status check sees it. ``URLSource`` took its ```` as the reference +title and its markup as the content, which surfaced downstream as + + [ERROR] Title mismatch: expected 'Pharmacotherapy for Alcohol Use Disorder…' + but got 'Checking your browser' + +on a pull request that cited a PMC article by URL (monarch-initiative/dismech#12867). +The cache had no entry, so validation fetched it live and compared a curated +title against a challenge page. + +Treating it as a transient failure is the honest outcome: nothing about the +reference is known, and a retry from somewhere unblocked can still succeed. +Caching it records an interstitial as a paper. +""" + +from __future__ import annotations + +import pytest + +from linkml_reference_validator.etl.sources.url import URLSource + +CHALLENGE_PAGES = [ + pytest.param( + "<html><head><title>Checking your browser before accessing" + "

Enable JavaScript and cookies to continue.

", + id="pmc-checking-your-browser", + ), + pytest.param( + "Just a moment..." + "
Verifying you are human.
", + id="cloudflare-just-a-moment", + ), + pytest.param( + "Access denied" + "

We have detected unusual traffic from your network.

", + id="unusual-traffic", + ), +] + + +@pytest.mark.parametrize("html", CHALLENGE_PAGES) +def test_a_challenge_page_is_recognised(html): + assert URLSource._is_bot_challenge(html) is True + + +@pytest.mark.parametrize( + "html", + [ + pytest.param( + "Pivotal Role of TLR4 Receptors - PMC" + "

" + + ("Real article prose. " * 40) + + "

", + id="real-pmc-article", + ), + pytest.param( + "A paper about human verification methods" + "

" + ("Prose discussing CAPTCHA research. " * 30) + "

", + id="article-whose-subject-is-bot-detection", + ), + pytest.param("plain text, no markup at all", id="plain-text"), + ], +) +def test_a_real_page_is_not_mistaken_for_a_challenge(html): + """The check must not fire on an article that merely discusses the topic. + + A paper about CAPTCHAs contains the words; a challenge page is short and + carries them in its title or as its whole body. Getting this backwards would + discard real references. + """ + assert URLSource._is_bot_challenge(html) is False