Let a consumer trust its cache instead of re-fetching it

[cmungall]

Staleness re-fetching suits a cache the tool owns and can rebuild. For a cache that is committed to a repository and reviewed, it inverts: the re-fetch is the risk, not the repair.

Three failures reported downstream are the same re-fetch seen from different angles:

dismech#12672	an unstamped full_text_html entry is withheld, and the repair route it points at cannot be reached
dismech#12867	a fetch returns PMC's bot-check page on an HTTP 200 and its <title> is compared against the curated title
dismech#12879	a re-fetch shortens 64 bodies and breaks snippets that verified, in a PR about something else

Each can be attacked on its own — serve the stale entry anyway, detect the interstitial, refuse a refresh that shrinks a body. I started doing exactly that, and it was the wrong shape: the third needs a size heuristic this project already tried and removed after three review rounds each found a defect in it.

The flag

trust_cached_entries (default False). When set, a cache entry is served as it is on disk — no staleness re-fetch, no full-text retry. There is no fetch during which any of the above can happen.

Two lines at the call site. The default is unchanged, so caches still migrate for consumers that want that.

An imperfect cached entry stays imperfect, and that is the trade. It is a content problem to fix deliberately — cache reference --force still rebuilds one on purpose — rather than something re-litigated on every validation run.

Tests

5 tests: served with no network call at all for full_text_html, full_text_xml and abstract_only; the file byte-identical afterwards; and the default still re-fetching, so the flag is genuinely opt-in.

Also here: the #94 review follow-ups

• 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; a PMC-only article still returns its abstract, so the refresh reaches _preserve_cached_full_text instead. Three tests now pin that.
• The stale-HTML warning told users to retry, which for a PMC-only article they cannot act on.
• 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.

Supersedes #96

That PR added bot-check phrase matching and a body-size threshold to URLSource. Closed: it recognised the interstitial instead of not asking for it.

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

🤖 Generated with Claude Code

[claude]

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

---

PR review: trust_cached_entries

• Read the PR diff against origin/main
• Traced fetch_with_provenance, _load_from_disk and cache reference with the flag set
• Reviewed the tests
• Ran the tests. I couldn't: running uv run pytest needs permission that this job doesn't have.

Summary: One opt-in flag at a single site stops the re-fetch from happening, which is a better fix than recognising what a re-fetch returns (the approach of #96). The default path doesn't change, and the tests check the thing that matters: no fetch call at all. There is one real problem, in how cache reference reports a trusted stale entry. The other points are small.

---

1. cache reference reports a trusted stale entry as current (should fix)

reference_fetcher.py:351-356 returns FetchOutcome(content=cached) with served_stale=False, even when the entry would fail _is_stale_cache_entry. cli/cache.py:153-172 promises that "the cache holds a current entry afterwards", and it treats served_stale as the only signal that this promise wasn't met.

If a consumer sets trust_cached_entries: true in the config file that load_validation_config reads, and then runs cache reference PMID:X without --force on an unstamped entry:

• nothing is fetched;
• it exits 0 and prints the entry as cached;
• the entry is still stale.

That is the same "green through an outage" result the comment in cache.py is written to prevent. Two ways to fix it:

• have cache reference ignore trust_cached_entries, since its whole job is to refresh; or
• have the trusted branch set served_stale=self._is_stale_cache_entry(...), so callers that care can still tell. This needs _load_from_disk to expose staleness, or a second check on the raw text.

Either way, a test for cache reference with the flag set would pin it down.

Fix this →

2. A committed file that won't parse is still overwritten under trust (minor)

_load_from_disk returns None when the frontmatter is malformed (_load_markdown_format, around line 1739). With the flag set, fetch then treats the entry as missing: it fetches, and _save_by_access rewrites the committed file.

That fetch is exactly what the flag is meant to prevent, and it replaces a hand-edited entry that has a typo. With trust on, it may be better to fail loudly, e.g. raise, or return None with an error log, rather than silently re-fetch. At the least, the field description in models.py should say that trust only covers entries that parse.

3. The stale-HTML warning doesn't mention the new flag (nit)

reference_fetcher.py:1551-1558 now points users to serve_stale_html_full_text. For the committed-cache case this PR is about, trust_cached_entries is the more complete answer. It would help to mention both, or to say which one fits which situation.

4. The overlap between the two flags isn't explained (nit)

trust_cached_entries=True implies allow_stale_html=True on the first read, so it overrides serve_stale_html_full_text whenever an entry exists. That's fine, but neither field's description says how the two relate. One sentence in models.py would stop someone from setting both and wondering which one wins.

5. Docs (nit)

Neither flag appears under docs/. The dismech use case ("committed, reviewed cache") would make a short how-to section, perhaps next to docs/how-to/fetch-full-text-and-pdfs.md.

---

Tests

• test_trust_cached_entries.py is well aimed. Asserting calls == [] tests the property itself, not a side effect of it. The parametrisation over full_text_html, full_text_xml and abstract_only also covers the _maybe_retry_full_text skip for abstract_only.
• test_the_default_still_re_fetches shows the flag really is opt-in. Good.
• The three _fetch_with_abstract_source tests fill the gap left by Let a reviewed cache serve stale HTML, and find PMC's current article container #94: the _preserve_cached_full_text path is the one consumers actually hit. test_refused_overwrite_leaves_the_file_alone_either_way reuses one tmp_path across both flag values, but _cache rewrites the file on each pass, so it holds.
• test_a_challenge_page_carrying_a_classed_body_is_still_declined is a useful regression guard for the selector.
• Missing tests:
  • trust with no cache file: it should still fetch and save, so a fresh clone can populate the cache;
  • item 1: cache reference with the flag set;
  • optionally, a check that the memory cache (_remember) makes a second fetch free under trust, too.

Code quality, performance and security

• Making find_pmc_article_body public and importing it at module level in pmid.py is a sensible cleanup. I checked that pmc.py doesn't import pmid.py, so the top-level import creates no cycle.
• The priority-order comment on PMC_ARTICLE_BODY_CLASSES is useful. It records a known limit (only the first legacy tsec is returned) without calling it a bug.
• Performance: with trust set, validation runs make no network calls for cached references, which is a clear gain on large committed caches.
• Security: no new concerns. Fetching less shrinks the attack surface, and in particular no interstitial HTML gets written into the repo.