Read OtherAbstract, and re-test the entries that claim no content

[cmungall]

Closes #88.

A refresh could delete the abstract a cache entry already held. _parse_abstract read only <Abstract>, so a record whose abstract PubMed keeps under <OtherAbstract> came back as having no content at all, and the refresh overwrote the cached text with an empty body.

PMID:5697815   1787 -> 850 bytes    967 characters lost
PMID:6680124   1695 -> 650 bytes   1175 characters lost

OtherAbstract is where PubMed keeps abstracts contributed by other indexing programs — PIP, KIE, NASA, AIDS — mostly on pre-1990 records. They are genuine abstracts, not metadata.

Three rules the tests pin

• Abstract still wins when a record has both. The fallback is a fallback.
• English is preferred. OtherAbstract also carries translations, so taking the first element would return the French on a record that has both — and an English snippet would then stop matching its own source.
• A translation is still used when it is the only text available. Reporting nothing blanks the entry, and the text is at least the paper's own summary.

Fixing the extractor recovers nothing on its own

This is the half that is easy to miss. An entry already damaged by this bug carries a current extractor_version, so _is_stale_cache_entry calls it fresh and it is never re-fetched. The text stays deleted permanently.

ABSENT_CONTENT_CACHE_VERSION versions the claim "this reference has no content at all", scoped to content_type: unavailable exactly as html_full_text_version and xml_extraction_version are scoped to theirs. Such an entry records what an extractor could not find, so an extractor fix can make it wrong; the stamp is what lets a fixed version re-test it.

Scoped rather than bumping EXTRACTOR_CACHE_VERSION because an entry that records content cannot be wrong in this way. In the dismech cache that is 1,735 files against 57,551 — a blanket bump would re-fetch every cached reference to find them, and churn on that scale is its own harm.

Correction (review round 1): the converse does not hold, and an earlier version of this section implied it did. unavailable is emitted by doi, url, clinicaltrials, json_api, entrez and ppr as well as pmid, and the OtherAbstract fix cannot help any of those. Of the 1,735 entries in the dismech cache, 1,499 are DOI and only 236 are PMID, so most of this refresh is cost rather than repair. That is accepted deliberately: the stamp means "this no-content claim was made by extractor version N", which a later fix to any source will want, and it is still a 3% refresh against 100%.

One gap, config-dependent: with source_extra_fields["PMID"] set, a record with no abstract is stored as summary carrying the extra-fields blob rather than as unavailable, so an OtherAbstract-only record damaged by #88 falls outside this stamp and is never re-tested. dismech does not set that option, so the figures above hold there. It is documented in the staleness rule and in troubleshooting.md with the manual remedy; widening the scope to summary would re-fetch all 2,290 such entries in dismech to reach a config-dependent few.

There is a round-trip test for this, because the emitter and the staleness rule have to agree or the fix is inert in either direction: without the emitted stamp every unavailable entry is re-fetched forever, and without the staleness rule the damaged ones are never re-fetched at all.

Why not simply "never demote to unavailable"

Because sometimes the demotion is right. Of the four records that changed in the run that surfaced this, two have no abstract element of any kind:

PMID:5697815    OtherAbstract Type="PIP"     <- bug, recovered
PMID:6680124    OtherAbstract Type="PIP"     <- bug, recovered
PMID:4869291    no abstract of any kind      <- correctly reports nothing
PMID:37769103   no abstract of any kind      <- correctly reports nothing

Those last two had cache entries storing the MEDLINE citation header as though it were content, so unavailable is the more honest label for them. A blanket protection would have preserved the wrong data.

Verification

All four records above checked against live PubMed XML, not fixtures. Full suite: 1149 passed, 1 skipped. Ruff clean, doctests pass.

[claude]

Claude finished @cmungall's task in 3m 3s —— View job

---

Review: OtherAbstract fallback + absent-content re-test stamp

• Gather context (diff, changed files)
• Review pmid.py abstract extraction changes
• Review reference_fetcher.py staleness / cache-version changes
• Review test coverage
• Post review

---

Verdict: the design is right and the two halves are correctly coupled. The insight that fixing the extractor alone recovers nothing — because a damaged entry carries a current extractor_version and so is never re-tested — is the part that would have been easy to miss, and the round-trip test (test_a_freshly_written_absent_entry_is_not_immediately_stale) is the correct guard for it: it pins that the emitter at reference_fetcher.py:1322 and the staleness rule at reference_fetcher.py:1656 agree, which is exactly where this class of fix goes inert.

Scoping to content_type: unavailable rather than bumping EXTRACTOR_CACHE_VERSION is the right call and consistent with the existing html_full_text_version / xml_extraction_version precedent.

I verified the surrounding code statically. I was not able to run the test suite — uv run pytest is outside this job's allowed tools — so the pass/fail claims in the PR body are unverified here.

Findings

1. The new stamp isn't documented, unlike the other two. (docs/)

xml_extraction_version is documented at docs/troubleshooting.md:875 and html_full_text_version at docs/how-to/fetch-full-text-and-pdfs.md:304, both explaining the one-time refresh users should expect. absent_content_version adds a third stamp that will re-fetch ~1,735 entries on the next validation run with nothing in the docs to explain it. Given the repo's own precedent — including the "a cache-wide refresh adds one line to every enriched entry — worth knowing before reviewing that commit" note — this seems like a gap worth closing.

Fix this →

2. Some damaged PMID entries are still unreachable, via the summary escape hatch. (src/linkml_reference_validator/etl/sources/pmid.py:174)

content_type = "abstract_only" if abstract else "unavailable"
...
if (content or "").strip() and content_type == "unavailable":
    content_type = "summary"

When source_extra_fields["PMID"] is configured, a record with no Abstract gets the extra-fields blob as content and is written as content_type: summary, not unavailable. Under the old extractor an OtherAbstract-only record in such a cache lost its abstract and landed outside the new stamp's scope, so it will never be re-tested. This is config-dependent and may be empty in the dismech cache — but if so, it's worth one sentence in the PR body, because the "1,735 files" figure quietly assumes no such entries exist. No change needed if source_extra_fields is unset in practice; I'd just want it stated rather than assumed.

3. The stamp is emitted from the constant, while the other two are read from reference.metadata. (src/linkml_reference_validator/etl/reference_fetcher.py:1322)

html_full_text_version and xml_extraction_version are sourced from the reference's own metadata, which is what makes the documented promise "inventory and metadata-only rewrites preserve the original XML stamp, including future versions" hold. absent_content_version is written from the module constant on every save, so any path that re-saves an unavailable entry without re-running the extractor would stamp it as re-tested when it wasn't — silently re-creating the #88 failure mode at version 2.

I traced the save paths and this is safe today: apply_full_text_location only saves when applied is true (which changes content_type away from unavailable), and _maybe_retry_full_text runs on the fetch path where staleness has already forced a re-fetch. So no current caller can produce a false certification. Worth a short comment at the emit site recording why the constant is correct here (the entry is always freshly produced by this extractor version), so a future enrichment path doesn't quietly break the invariant.

4. Blast radius spans all sources, not just PMID. (informational)

unavailable is also emitted by doi.py, url.py, clinicaltrials.py, json_api.py, entrez.py, and ppr.py. The OtherAbstract fix cannot help any of those, so a share of the 1,735 re-fetches is pure cost. Still a reasonable one-time price and far better than a blanket EXTRACTOR_CACHE_VERSION bump — just not as tightly targeted as "only entries claiming no content can be wrong in this way" implies.

5. Nit: the English-preference loop retries English nodes. (src/linkml_reference_validator/etl/sources/pmid.py:293)

for node in english + others:

English nodes appear twice in the concatenation, so when an English OtherAbstract is empty it is rendered a second time in the fallback pass. Harmless and cheap — noting it only because the intent reads as "English first, then the rest."

Things I checked that are correct

• soup.find("Abstract") runs against a single-PMID efetch document (pmid.py:152), so find_all("OtherAbstract") cannot pull text from a neighbouring record.
• Defaulting a missing Language to eng matches the PubMed DTD, where Language on OtherAbstract is #IMPLIED with an eng default — so the common attribute-less case is correctly treated as English rather than as a translation.
• An Abstract element that exists but renders empty now correctly falls through to OtherAbstract, rather than short-circuiting to None on element presence alone.
• content_type is unconditionally written at reference_fetcher.py:1344, so the metadata.get("content_type") == "unavailable" guard in the staleness rule can't miss entries through an absent key.
• type(x) is not int rather than isinstance correctly keeps YAML booleans from passing as version stamps, matching the XML rule.
• No other module parses PubMed article XML for abstracts, so there's no second extraction site left unfixed.

Test coverage

Good, and notably the tests pin the reasons rather than the mechanics. test_entries_that_do_have_content_are_untouched enumerating every other content_type is the right shape for a scoped-invalidation rule — it's the test that fails if someone later widens the scope by accident. test_a_newer_stamp_is_not_stale preserving the existing forward-compatibility convention is a nice catch.

Two gaps worth considering:

• No test that an Abstract element present-but-empty falls through to OtherAbstract. That's a distinct branch from "no Abstract at all" and the one that regresses if someone rewrites the guard back to if not abstract:.
• No test for a record with only an OtherAbstract that has no Language attribute at all. The or "eng" default is load-bearing for real PIP records and is currently only exercised implicitly.
  • branch fix/other-abstract

[cmungall]

All five findings taken, both test gaps closed, and the CI failure fixed — which was mypy, not the tests.

The tests passed. 1151 on every Python version. The single error was node.get("Language") returning str | AttributeValueList, so .strip() failed type checking. Handled rather than silenced. I had run bare pytest where just test is pytest + mypy + format, which is how it reached you.

Finding 4 is right, and the numbers are worse than either of us had. I checked what the 1,735 entries actually are:

DOI    1499
PMID    236

So 86% of that refresh is cost, not repair — the OtherAbstract fix cannot help a DOI entry. "Only entries claiming no content can be wrong in this way" was true but invited the converse reading, and the tight-targeting framing in the PR body did not survive contact with the data. The constant's comment now says this outright, and the design rests on a different argument: the stamp is deliberately general — "this no-content claim was made by extractor version N" — which a later fix to DOI or any other source will want. Still a 3% refresh against 100%, so the scoping call stands; my justification for it did not.

Finding 2, the summary escape hatch: confirmed real, and worth stating rather than assuming, as you asked. dismech does not set source_extra_fields, so no PMID record there becomes summary and the 1,735 figure holds. The gap is real for anyone who does set it, so it is now documented in both the staleness rule and troubleshooting.md, with the manual remedy.

I decided against widening the scope to summary. Most summary entries have nothing to do with a missing abstract — dismech has 2,290 of them, none from this path — so widening would re-fetch all of them to reach a config-dependent few.

Finding 3 — comment added at the emit site recording why the constant is correct here, and naming the invariant that a future enrichment path would break. Your trace matches mine: no current caller can produce a false certification.

Finding 1 — documented beside the xml_extraction_version paragraph in troubleshooting.md, including the one-time refresh and what is unaffected.

Finding 5 — fixed; English nodes no longer appear twice in the fallback pass.

Both test gaps added. The Language-less one earns its place: or "eng" is load-bearing for real PIP records and was only exercised implicitly, so nothing would have caught its removal.

On not being able to run the suite — that split the work usefully rather than limiting it. The static read found five real things including the two test gaps; the part you flagged as unverified is exactly the part CI caught.

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

[claude]

Claude finished @cmungall's task in 3m 58s —— View job

---

Re-review after 0c73af5

• Diff the new commit against my previous review's findings
• Re-review pmid.py and reference_fetcher.py in full
• Check the new docs section
• Review test coverage, including the gaps I flagged
• Run the test suite if tooling allows — blocked, see note
• Post review

---

All five findings from the previous round are addressed, and two of them were answered better than I asked. Finding 2 (the summary escape hatch) I raised as "state it rather than assume it"; the commit instead quantified it (dismech doesn't set source_extra_fields), documented it in the staleness rule and in troubleshooting.md with a manual remedy, and gave the reason for not widening. Finding 4 (blast radius) came back with the numbers pointing the other way from the PR body — 1,499 of the 1,735 are DOI, so most of this refresh is cost — and the constant's comment now rests the design on a different argument rather than defending the original one. Revising the claim downward when the evidence went against it is the right move, and the replacement argument ("the stamp means this no-content claim was made by extractor version N, which a later fix to any source will want") is the one that actually holds.

The mypy fix is real and not a silencer: node.get("Language") genuinely can return a list for a multi-valued attribute, and _is_english handles it rather than casting it away.

I could not run just test, pytest, ruff, or mypy — they're outside this job's allowed tools. The 1152 passed, 1 skipped; mypy clean; ruff clean claim is unverified here. I traced the new tests by hand and they look correct (see below).

Remaining findings

Both are minor; neither blocks.

1. The emit-site comment names the wrong guarantee. (src/linkml_reference_validator/etl/reference_fetcher.py:1330)

_maybe_retry_full_text runs on the fetch path where staleness already forced a re-fetch.

It doesn't. _maybe_retry_full_text is called at reference_fetcher.py:351, on the disk-cache-hit path — the branch that returns without fetching. The conclusion is still right, but via a different and stronger route: _load_from_disk rejects stale entries at reference_fetcher.py:1507, so any unavailable entry that reaches line 351 already carries the current stamp, and re-writing the constant is a no-op. At version 2 that still holds — a stamp-1 unavailable entry is stale, _load_from_disk returns None, and the cache-hit branch is never taken.

This matters because the comment exists specifically to orient a maintainer at version 2, and as written it sends them to check the wrong invariant. The accurate statement is a single load-bearing sentence: nothing reaches a save path holding an unavailable entry it did not just produce, because _load_from_disk filters stale entries out and the two paths that deliberately serve stale ones never save.

While verifying this I confirmed the other half of the claim, which the comment does not mention and which is the part that could actually break: _stale_fallback (reference_fetcher.py:403) and _preserve_cached_full_text (reference_fetcher.py:440) both document "the entry is deliberately not re-saved". Those two are the paths that do hold an untouched cached entry, and they are safe only because of that contract. Worth naming them in the comment — they are the ones a future change would have to violate.

Fix this →

2. The emitter's scoping is untested; only the staleness rule's is. (src/linkml_reference_validator/etl/reference_fetcher.py:1340)

test_entries_that_do_have_content_are_untouched is the right shape, but it exercises _is_stale_cache_entry, not _save_to_disk. Delete the if reference.content_type == "unavailable": guard at line 1340 and the whole suite still passes — test_a_freshly_written_absent_entry_is_not_immediately_stale only asserts the stamp is present on an unavailable entry.

The failure that escapes is the one the repo already treats as worth documenting: an unconditional stamp adds a line to every cache entry on refresh, which is precisely the "a cache-wide refresh adds one line to every enriched entry" churn flagged three paragraphs up in troubleshooting.md. One assertion in the existing round-trip test closes it — save an abstract_only entry and assert absent_content_version is not in the written text.

Fix this →

3. Nit: _is_english is evaluated twice per node. (src/linkml_reference_validator/etl/sources/pmid.py:299)

english = [node for node in others if _is_english(node)]
rest = [node for node in others if not _is_english(node)]

The double pass over others is free at these sizes and reads clearly, so this is only a preference — a single partition loop would say the same thing once. The actual defect I raised last round (English nodes rehearsed in the fallback) is gone.

Verified correct this round

• Parser case-sensitivity. find_all("OtherAbstract") only works because _fetch_pubmed_xml builds the soup with BeautifulSoup(xml_content, "xml") (pmid.py:386), which is case-preserving. The html.parser at pmid.py:581 lowercases tag names and would silently match nothing — but it's a different, unrelated path.
• Single-record scope. Entrez.efetch(db="pubmed", id=pmid, ...) fetches one PMID, so find_all cannot reach a neighbouring record's abstract.
• bs4 Tag truthiness doesn't bite. if abstract: is length-based on contents, so <Abstract></Abstract> is falsy while <Abstract><AbstractText></AbstractText></Abstract> is truthy — but both converge on the OtherAbstract fallback, one by skipping the render and one by rendering to None. The new test_parse_abstract_falls_through_an_empty_abstract_element covers the truthy branch, which is the one that would regress under a rewrite to if not abstract: return None.
• The stale paths don't write the stamp. Traced all three: _stale_fallback returns without saving, _preserve_cached_full_text returns without saving, and iter_cached_references is read-only. _save_to_disk has exactly two callers (lines 690 and 822), both on paths that just produced the content.
• Private entries. A private unavailable entry gets the stamp but is never read back by _load_from_disk, so the scoping is unaffected either way.
• Legacy-format entries are exempt from staleness by an existing documented decision, and Refresh deletes a cached abstract when PubMed stores it in <OtherAbstract> (PIP/NASA/KIE/AIDS) #88 only ever wrote Markdown, so no pre-Markdown entry is damaged and left unreachable.
• Doctest output. "An indexer's summary." is the correct repr — the apostrophe forces double quotes.
• Both new tests trace correctly by hand, including the fre-vs-attribute-less ordering in test_parse_abstract_treats_a_language_less_other_abstract_as_english.

Verdict

Ready to merge. Finding 1 is a comment whose stated reason doesn't match the code it describes, and finding 2 is one missing assertion — worth folding in, but neither changes behaviour and neither is a reason to hold the fix.
• branch fix/other-abstract

[cmungall]

Both taken, and the nit.

The comment did name the wrong guarantee — you're right. _maybe_retry_full_text is called inside the if cached: branch, which returns without fetching, so "the fetch path where staleness already forced a re-fetch" describes something that doesn't happen. The conclusion held, but a comment that exists to send a maintainer at version 2 to the right invariant is worth nothing if it sends them to the wrong one.

It now states what you traced: _load_from_disk filters stale entries out, so an unavailable entry surviving a cache hit already carries the current stamp and re-writing it is a no-op — and at version 2 a stamp-1 entry is stale, so that branch is never taken for it at all. It also names _stale_fallback and _preserve_cached_full_text as the two paths that genuinely hold an untouched cached entry, safe only because both document that it is not re-saved. Those are the contracts a future change would have to violate, and you were right that they were the load-bearing half the comment omitted.

The emitter's scoping gap is real — I verified it by removing the guard rather than reasoning about it. 81 tests stayed green. test_an_entry_with_content_is_not_stamped now closes it, and I re-ran the mutation to confirm it fails against it rather than merely passing against correct code.

Your framing of what escapes is the part worth keeping: not a wrong answer, but a line added to every cache entry on refresh — the same churn troubleshooting.md already documents for full_text_access_type. That's in the test's docstring.

Nit taken: single partition pass.

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

[claude]

Claude finished @cmungall's task in 2m 9s —— View job

---

Re-review after 0ac77b1

• Diff 0ac77b1 against my previous round's findings
• Verify the corrected invariant comment against the actual call sites
• Review the new emitter-scoping test
• Check the one-pass partition in pmid.py
• Post review

---

All three findings addressed, and the correction to finding 1 turns out to matter more than I framed it. I raised it as "the conclusion is right, the stated reason isn't" — a documentation accuracy point. Tracing it again with the new comment in hand: unavailable is in NEEDS_FULL_TEXT_TYPES (reference_fetcher.py:63), so _maybe_retry_full_text does not return early for these entries, and when _enrich_with_full_text flips full_text_attempted without finding anything, after != before and it calls _save_by_access → _save_to_disk at line 680. So there genuinely is a path that re-saves an unavailable entry it did not just produce content for. The old comment pointed at that path and gave the wrong reason it was safe; the new one gives the reason that actually holds (_load_from_disk already rejected stale entries, so the re-stamp is a no-op at v1, and at v2 the entry never reaches the cache-hit branch). That is the difference between a comment that happens to be right and one a maintainer at version 2 can rely on.

test_an_entry_with_content_is_not_stamped is a real mutation-killer: delete the content_type == "unavailable" guard at reference_fetcher.py:1346 and it fails on assert "absent_content_version" not in written. The one-pass partition at pmid.py:299-302 reads fine.

I could not run just test, pytest, mypy, or ruff — uv run is outside this job's allowed tools. The 1153 passed, 1 skipped claim is unverified here; I traced the new test by hand.

Two notes on the new comment, neither blocking

1. The universal has one counterexample, and it happens to be harmless. (reference_fetcher.py:1332)

nothing reaches a save path holding an unavailable entry it did not just produce

_load_from_disk returns _load_legacy_format(...) at line 1512 before reaching the staleness check at 1514, and that loader sets content_type=metadata.get("ContentType", "unknown") (line 1885). A legacy entry carrying ContentType: unavailable therefore loads without a staleness check, reaches line 351, and _maybe_retry_full_text re-saves it — in Markdown, stamped absent_content_version: 1 — without any extractor having re-examined it.

I worked through whether this matters and it doesn't, but for a non-obvious reason worth recording: legacy entries are already exempt from staleness by the documented decision at lines 1504-1510, so such an entry was never going to be re-tested at v1 either way. And the migration to Markdown makes it more recoverable, not less — at version 2 the stamp-1 entry becomes stale and gets re-fetched, where the legacy .txt would have stayed exempt forever. So the path improves the outcome. The only inaccuracy is that the sentence reads as absolute. A clause ("…except a legacy entry, which bypasses the staleness check but is exempt from re-testing anyway") would close it; leaving it is also defensible given the comment is already long and this is the one case where being wrong is an improvement.

2. The comment dropped the one path that is safe for a different reason. (reference_fetcher.py:1332)

The previous comment named apply_full_text_location; the new one names only _stale_fallback and _preserve_cached_full_text. But apply_full_text_location (line 803) is a public method taking caller-supplied content, so the _load_from_disk-filters-stale argument doesn't reach it — a caller can hand it a hand-built unavailable record. It's safe because it saves only when applied is true, and _apply_full_text_location sets content.content_type from _FORMAT_TO_CONTENT_TYPE (line 853) before returning True, so a saved entry is never unavailable. That's a third mechanism, not the one the comment describes, and it's the one on the public surface where an external caller could apply pressure. Worth half a sentence given the comment exists to enumerate exactly this.

Verified correct this round

• Line 351 is the cache-hit branch. if cached: at 347 returns at 352 without fetching. The commit message's diagnosis is right.
• _stale_fallback (403) and _preserve_cached_full_text (440) both return without saving, as their docstrings promise — re-confirmed, and they are correctly the two named.
• _save_to_disk still has exactly two callers, lines 690 and 822, both reached only via _save_by_access / apply_full_text_location.
• iter_cached_references (825) is read-only and, per the _is_stale_cache_entry docstring, deliberately not staleness-filtered — so no enrichment walk can launder a stamp.
• The v2 argument holds end to end: a stamp-1 unavailable entry fails at _is_stale_cache_entry:1689-1695, _load_from_disk returns None at 1520, cached is falsy, and the fetch path runs the fixed extractor.
• The new test's mutation actually escapes the old suite — test_a_freshly_written_absent_entry_is_not_immediately_stale asserts only presence on an unavailable entry, and test_entries_that_do_have_content_are_untouched calls _is_stale_cache_entry directly, never _save_to_disk. The commit message's "left 81 tests green — verified by removing it" matches what I traced.
• The partition preserves order within each group, so english + rest still renders in document order within the preferred group. _is_english now evaluated once per node.
• list[Any] annotations are needed here — bare [] would infer list[<element type>] from first append under mypy's strict-optional inference and the Any matches the surrounding _is_english(node: Any) signature.

Verdict

Ready to merge. Both remaining notes are comment-only and neither describes a behaviour defect — note 1 is a universal with a counterexample that improves the outcome, note 2 is an omitted third safety mechanism on the public surface. The substance of the fix, the coupling between emitter and staleness rule, and the test coverage are all in good shape.
• branch fix/other-abstract