Skip to content

Do not cache a bot-check page as a reference, and address the #94 review - #96

Closed
cmungall wants to merge 2 commits into
mainfrom
fix/stale-html-followups
Closed

Do not cache a bot-check page as a reference, and address the #94 review#96
cmungall wants to merge 2 commits into
mainfrom
fix/stale-html-followups

Conversation

@cmungall

@cmungall cmungall commented Sep 26, 2026

Copy link
Copy Markdown
Member

Two commits: the review follow-ups from #94, and a guard that stops a bot-check page being cached as a reference.

Do not cache a bot-check page as a reference

PMC and Cloudflare answer a Python HTTP client with a challenge page on an HTTP 200, so no status check sees it. URLSource took its <title> as the reference title. Downstream that surfaced as:

[ERROR] Title mismatch: expected 'Pharmacotherapy for Alcohol Use Disorder…'
        but got 'Checking your browser'

on a PR 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 false-positive direction is the one that matters. 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, not a fixture:

PMC: 200, 21,207 chars, <title>Checking your browser - reCAPTCHA</title>
  recognised as a challenge: True

This does not make PMC reachable. It stops a page that is not the reference being stored as though it were.

Review follow-ups from #94

  • The path consumers actually take had no test. 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. Verified before writing the tests that the flag does reach that path; three tests now pin it.
  • The stale-HTML warning told users to retry, which for a PMC-only article they cannot act on. 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.
  • The comment above the second call site said stale HTML is "withheld here" — true only by default now.
  • find_pmc_article_body loses its underscore and is imported at module level, since two modules share it.
  • PMC_ARTICLE_BODY_CLASSES records that its order is a priority order.
  • A negative test for a challenge page carrying a classed <body>.

Verification

just test: 1174 passed, 1 skipped; mypy clean; ruff clean.

Whether to keep the unreachable PMC HTML route in the default provider chain is still open in #95.

🤖 Generated with Claude Code

cmungall and others added 2 commits September 25, 2026 18:31
@cmungall @claude
Address review on #94: test the path consumers actually take
63236b1 
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 `<body>`, 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016WUDZDTqMP5ih9rD7QqGJ4
@cmungall @claude
Do not cache a bot-check page as a reference
4c824cd 
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 `<title>` 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>
@claude

claude Bot commented Sep 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Claude finished @cmungall's task in 56s —— View job

Review of PR #96

  • Read the diff
  • Traced how None from URLSource.fetch is handled in ReferenceFetcher.fetch
  • Run tests: not done. Running uv run pytest / uv run python needs approval in this job, so I could not run anything. Everything below comes from reading the code. The false-positive cases in point 1 are traced by hand.
  • Post review

Overall this is a well-scoped PR. The bot-check guard handles a real failure mode, and returning None is correct. It falls through to _stale_fallback (reference_fetcher.py:383-384), which does not write to disk and does not remember a None outcome in memory. So a later run from an unblocked network can still succeed, as the PR description says. The #94 follow-ups (renaming to find_pmc_article_body, the module-level import, the corrected comment, the ordering note) are clean.

I have one substantive concern and a few smaller ones.

1. The check can still fire on real references (url.py:179-189)

The PR says false positives are what matter most, but two paths can still throw away a real reference:

  • The title check has no length limit. "unusual traffic" or "just a moment" anywhere in the <title> rejects the page, however long it is. A page titled "Detecting unusual traffic in enterprise networks" with a 100 KB body counts as a challenge. The docstring says a real paper "does not carry the phrase in its title", but nothing enforces that.
  • The short-body check covers most of the web. URL-cited references are often not PMC articles: gene pages, database records, GitHub READMEs, short news items. Many are under 60 KB. "just a moment" is ordinary English, so a short page saying "please wait just a moment while the viewer loads" is thrown away. So is a short page that quotes "please verify you are a human".

Possible fixes, in rough order of preference:

  • Require the phrase in the <title> and a short body. That matches PMC (~21 KB, phrase in title) and Cloudflare's "Just a moment..." (small, phrase in title).
  • For the body-only path, look for real challenge markers instead of prose phrases, e.g. cf-challenge, challenge-platform, /cdn-cgi/, g-recaptcha.
  • Drop "just a moment" from the body-only list, since it is the most generic phrase.

Fix this →

2. The negative test does not exercise the case it is named for (tests/test_url_bot_challenge.py)

article-whose-subject-is-bot-detection contains none of _BOT_CHALLENGE_PHRASES. "human verification methods" and "CAPTCHA research" match nothing. It would pass even if the size limit were deleted. To pin the documented behaviour:

  • a page over 60 KB whose body contains "verifying you are human" → False
  • the same page cut under 60 KB → whatever the intended answer is (with point 1's fix, False unless the title also matches)
  • a long page with a phrase in the title (currently True, see point 1)

The doctest "<title>A real paper</title>" + "x" * 70000 has the same gap: it contains no phrase, so the size limit is never tested.

3. Nothing tests fetch() returning None

The tests only call the classmethod _is_bot_challenge. A test that patches ContentAcquirer.fetch_bytes to return challenge HTML would check that URLSource.fetch returns None. Better still would be ReferenceFetcher.fetch("url:…") checking that nothing is written to cache_dir. That is the behaviour the PR title promises, and it would catch someone reordering the check after _extract_title or after the save.

4. Minor

  • url.py:180: re.IGNORECASE does nothing because the search runs on lowered. Harmless, but it suggests the case handling happens there.
  • url.py:179: content.lower() copies the whole document even when it is over the limit and only the title is needed. Pages are capped by ContentAcquirer, so this is cheap, but lowering only the title match plus a gated body lowering would be tidier.
  • reference_fetcher.py:1541-1547: the new warning names PMC for every refused stale HTML entry, including non-PMC ones. Something like "…not for sources that serve a bot-check page (e.g. PMC-only articles)…" would read correctly in both cases.
  • test_refused_overwrite_leaves_the_file_alone_either_way: each loop iteration calls _cache(tmp_path), which rewrites the file. The True iteration therefore does not start from the file the False iteration left behind. That is fine for what the test asserts, but pytest.mark.parametrize over the flag would make each case independent and name the failing one.

Security / performance

No concerns. No new network calls or user-controlled regexes. [^<]* in the title regex is linear.

Test coverage summary

  • The three _preserve_cached_full_text tests fill the gap noted in Let a reviewed cache serve stale HTML, and find PMC's current article container #94 and look right: default serves the abstract, opting in serves the cached body, and the file is never stamped.
  • The classed-<body> negative test for find_pmc_article_body is a good guard.
  • The bot-check tests need the gaps in points 2–3 closed before they back up the "false positives are the important case" claim.

@cmungall cmungall mentioned this pull request Sep 26, 2026
Closed
@cmungall

cmungall commented Sep 26, 2026

Copy link
Copy Markdown
Member Author

Closing in favour of a much smaller fix.

The CAPTCHA-detection guard in here was solving the wrong problem. The reason a bot-check page ever reached the title comparison is that the validator re-fetches a cache entry whose extractor stamp is missing — so the real fix is to stop re-fetching a cache the consumer already trusts, not to teach the fetcher to recognise interstitials. One config flag instead of a phrase list and a size heuristic.

Opening that instead. The six review follow-ups from #94 will come with it; they were small and worth keeping.

@cmungall cmungall closed this Sep 26, 2026
@cmungall cmungall mentioned this pull request Sep 26, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cmungall