Reject empty excerpts and fix content extraction bugs

[cmungall]

Summary

Three reported bugs, all in the same family: things that were substantively empty or corrupted passed or failed validation for the wrong reasons.

1. Empty excerpts passed vacuously (dismech#8550) — the empty string is a substring of every document, so a blank excerpt was recorded as verified evidence quoting nothing at all.
2. Stub detection discarded real articles (genesets#10) — any document whose markup contained "restricted" was thrown away. Morris 2019 (PMC6358485) is a 155 KB open-access paper whose Methods restrict an analysis "to up to 13,977,204 high quality HRC imputed variants"; it was discarded and an 8.7 KB PMC placeholder cached as its full text.
3. Markup stripping corrupted text (genesets#10) — get_text(strip=True) trimmed each markup-delimited run before joining, so (<i>GUSB</i>, <i>GRN</i>, and <i>NEU1</i>) extracted as (GUSB,GRN, andNEU1), breaking every snippet spanning an italicised gene symbol.

⚠️ Behaviour changes to expect on first run

These are the point of the PR, but they will surface immediately:

• Blank excerpts now fail. Data using supporting_text: "" or snippet: "" as a placeholder will report errors where it previously passed. An absent excerpt field is untouched — that stays your schema's business.
• Blank excerpts outrank skip_prefixes and trusted_low_similarity. A blank excerpt on a skipped or trusted reference changes from valid/INFO to ERROR. Those settings are statements about a reference; a blank excerpt is a defect in the data that no reference can excuse.
• Cached references are re-fetched once. Entries written before the extractor fixes hold content those bugs produced, so they are re-fetched the next time a validation needs them. The scope is the whole cache — every reference type, however it was populated — so a large pre-fetched cache re-fetches everything on first use, at rate_limit_delay per reference. Lazy and per-reference: nothing is deleted, and cache export / Zotero enrichment read older entries unchanged. If a reference can't be re-fetched (offline, outage, withdrawn record) the older copy is served with a warning rather than reported as missing. The explanation for the ordinary re-fetch is logged at INFO, so it needs -v. See troubleshooting.
• repair data exits non-zero for a file whose only defect is an empty excerpt.
• Repair report paths changed shape — evidence[0] → evidence[0].supporting_text. That is what stops a repair landing in a different field than the quote came from, but anything downstream parsing those paths sees a different string.

Key changes

Empty excerpts

• check_excerpt_content() is the single source of truth for whether an excerpt has substance, used by the validator, the LinkML plugin and the repairer. It runs before any reference is fetched.
• Rejects blank text, and text that quotes nothing once editorial [brackets] and ... separators are removed.
• INSUFFICIENT_EXCERPT is its own repair action with its own report section and exit-code contribution — deliberately not a REMOVAL, since a removal is a verdict reached by comparison and nothing here was compared.
• min_excerpt_length (opt-in, default 0) rejects excerpts below a minimum, measured in non-whitespace characters of quoted text so PDF-to-text damage and chemical nomenclature don't distort it. Set it under validation: — top-level keys are silently ignored when a validation: section exists.
• _substring_match refuses an empty part list rather than returning found=True, keeping the primitive honest for new callers.

Content extraction

• Stub detection runs on the extracted body, not raw markup, with a length gate — breadth in the phrase list is safe because a real article body can never be short enough to match.
• Whitespace around inline markup is preserved; <br> becomes a newline on every path.
• All three PMC paths share one HTML text extractor via extract_scope(), which takes an already-parsed region without re-parsing or re-scoping it, and does not modify the caller's tree.
• XML is passed to the parser as received — decoding or re-encoding it corrupts articles that declare a non-UTF-8 encoding (François → François).
• One MIN_FULLTEXT_CHARS floor for all four full-text gates.
• The extractor modules point at EXTRACTOR_CACHE_VERSION, so the next extraction fix is made next to a note about the cache it invalidates.

Caching

• Entries record extractor_version; the validation read path treats older stamps as absent so they refresh. Newer stamps are left alone, so two versions sharing one cache don't re-fetch each other's work.
• A stale entry is served, with a warning, when the source yields nothing — and left unrewritten, so the next reachable run still refreshes it. --force opts out.
• Frontmatter ends at a line that is exactly ---. Splitting on the bare string let a value containing --- (a URL reference id, a title) truncate the block: it hid the version stamp, making such entries re-fetch on every run, and dropped every field after it on load.

Testing

799 tests, 200 doctests, mypy and ruff clean. Regression tests name the exact cases from the reports — the Morris 2019 sentence, the (GUSB, GRN, and NEU1) gene list — and assert the previously-rejected snippets now validate end to end.

The full-text floor, the empty-excerpt exit code and the offline fallback are verified by mutating the source and confirming the intended tests fail, rather than by writing tests and watching them pass.

Smaller behaviour notes

• repair data now checks every excerpt key on an item, matching the plugin. An item carrying the same quote under both supporting_text and snippet is therefore reported once per key; the reference itself is fetched once.
• A non-string supporting_text (a list, say) is dropped by the repair CLI rather than throwing inside _split_query.

https://claude.ai/code/session_018ePtLQDGiymEgrfJKKPrLs

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read PR diff)
• Review source changes
• Review test coverage
• Post review feedback

---

Overall this is a well-constructed PR. The core insight — that "" in anything is True, so a blank excerpt was being recorded as verified evidence — is correct and important, and the fix is applied consistently across all four layers. The two extractor bug fixes are real and the regression tests explain them clearly. Docstring/doctest discipline matches the repo conventions in CLAUDE.md.

Below are the issues I found, roughly in order of importance.

Note: I was unable to execute just test / uv run pytest in this environment (command approval was denied), so everything below is from static reading, not from a test run.

---

1. Cached stub content is not invalidated by the extractor fix

tests/test_extractor_fidelity.py:10 says the old bug caused "an 8.7 KB stub cached in its place, labelled as full text." Fixing the extractor does not fix the cache — ReferenceFetcher.get_cache_path() (etl/reference_fetcher.py:494-524) keys purely on the reference ID, with no extractor/format version component. Every user who validated Morris 2019 (or any similarly-affected paper) before this PR will keep getting the poisoned 8.7 KB stub served from references_cache/ and will keep seeing correct snippets rejected, with nothing in the output hinting why.

At minimum this needs a release note telling users to clear their cache. Better would be a version stamp in the cache path or the cached file's header, so stale entries are re-fetched automatically.

Fix this →

2. The HTML fallback path still has the bug that was just fixed

etl/extract/html.py:58 — the <p> branch was correctly changed to p.get_text().strip(), but the fallback immediately below still calls scope.get_text(separator="\n", strip=True), which injects a newline around every inline tag: (<i>GUSB</i>, <i>GRN</i>) becomes (\nGUSB\n,\nGRN\n).

The regression test for this path, test_html_fallback_without_paragraphs_keeps_symbol_boundaries (tests/test_extractor_fidelity.py:252), only passes because it asserts through find_text_in_reference, and normalize_text strips punctuation and collapses whitespace. An assertion in the style of the sibling tests — assert "(GUSB, GRN, and NEU1)" in text — would fail on that path. So the fallback is producing corrupted text that happens to survive a lenient matcher, which is exactly the fragility this PR is trying to remove.

Suggest applying the same treatment to the fallback (or separator=" "), and tightening the test to assert on the extracted string directly so it actually pins the behaviour.

Fix this →

3. STUB_NOTICE_PHRASES is both redundant and too narrow

etl/extract/xml.py:13-19:

"full text is restricted",
"access to the full text is restricted",

The second is a strict superset of the first, so it can never match independently — dead weight. Meanwhile common PMC wordings like "Access to this article is restricted" match neither entry, so the fix trades a false-positive bug for a false-negative one.

The length gate (MAX_STUB_NOTICE_CHARS) is what makes phrase matching safe here — that's stated well in the comment. Given that, a single broad "restricted" under the ≤1000-char gate would be simpler, catch more notice wordings, and still be safe from the Morris 2019 failure mode (155 KB ≫ 1000). Worth reconsidering the phrase list now that length does the real work.

4. Blank-excerpt check bypasses skip_references and trusted_low_similarity

validation/repairer.py:470-490 — the new early return sits before both the skip-list check and the trusted-list handling. The trusted branch at line 522-526 explicitly strips REMOVAL actions ("don't flag for removal"), but a blank excerpt on a trusted reference now emits a REMOVAL that logic was written to suppress.

This is probably the intent (a blank quote is a data defect, not a low-similarity judgement call — which is how validate() documents the equivalent ordering vs. skip_prefixes), but the interaction is undocumented and untested. A test pinning whichever semantics you want would keep someone from "fixing" it later.

5. check_excerpt_content misses excerpts that quote nothing

validation/supporting_text_validator.py:454-468 — with min_excerpt_length at its default 0, an excerpt of "..." or "[editorial note]" has zero quoted characters and is exactly as vacuous as "", but passes check_excerpt_content. It is still ultimately rejected, by the Query is empty after removing brackets and splitting branch in find_text_in_reference — so no vacuous pass — but only after the reference is fetched, and with a different message than the sibling defect.

Since count_quoted_characters() already exists, rejecting count == 0 unconditionally (independent of min_excerpt_length) would make the diagnosis consistent, skip a pointless network fetch, and match the PR's own framing that these checks "hold regardless of what the excerpt cites."

Fix this →

6. XMLExtractor.extract is typed bytes but is now deliberately fed str

etl/sources/pmid.py:425 passes handle.read() straight through, and test_pmc_xml_fetch_keeps_article_mentioning_restricted (tests/test_extractor_fidelity.py:157) explicitly feeds it a str. The implementation handles both (xml.py:81), so this works — but the base signature in etl/extract/base.py says data: bytes, and mypy only lets it through because handle.read() on a MagicMock/untyped Entrez handle is Any. Since str input is now a tested, intended contract, widen the annotation to Union[bytes, str] on the base Extractor.extract so the type reflects reality.

7. Smaller items

• plugins/reference_validation_plugin.py:206 — instance={"supporting_text": excerpt_value} hardcodes the key even when excerpt_field is snippet. Use {excerpt_field: excerpt_value} to match the field the message names.
• etl/extract/xml.py:99 — discarding an article is logged at debug. Given issue Bump actions/checkout from 4 to 5 #1 above (silent stub caching is what made the original bug so hard to spot), info or warning would make a dropped article visible without -vvv.
• MAX_STUB_NOTICE_CHARS = 1000 — a legitimately short body (correction notice, editorial, brief comment) containing "cannot be obtained" is still silently dropped. Very low risk; combined with the logging change above it would at least be diagnosable.
• cli/repair.py:326-338 — the new candidate loop is isinstance(text_value, str)-guarded, so a non-string supporting_text (e.g. a list) is now dropped where the old or chain would have passed it through. Almost certainly an improvement, but it's an unmentioned behaviour change.
• Migration note — the plugin now yields an ERROR for any present-but-empty-string excerpt field on any instance. Data using supporting_text: "" as a placeholder will light up. That is the point of the PR, and None is correctly left alone by the isinstance guard, but it deserves a line in the release notes.
• Formatting — excerpt_path = (...) at reference_validation_plugin.py:189 and the actions=[RepairAction(...)] block at repairer.py:480 are wrapped in a style the surrounding files don't use. just format runs ruff check (not ruff format) and E501 isn't in ruff's default rule set, so nothing will flag it — purely cosmetic.

---

What's good

• The layering is right: check_excerpt_content() is the single source of truth, EMPTY_SUPPORTING_TEXT_MESSAGE keeps every layer reporting the same diagnosis, and the check consistently runs before any fetch.
• count_quoted_characters() measuring non-whitespace characters (rather than tokens) is a genuinely good call — the "t he pro tein" and "2,3-dihydroxybenzoate" cases in the docstring show the reasoning, and reusing _split_query means bracket semantics can't drift from the matcher.
• min_excerpt_length defaulting to 0 with a ge=0 constraint is the right migration posture.
• Deleting the duplicated check in pmid.py in favour of the shared XMLExtractor removes the second copy of the bug — exactly the right fix, not just a patch at the reported site.
• MAX_STUB_NOTICE_CHARS being importable by tests rather than duplicated as a literal.
• Test coverage is thorough: BLANK_TEXTS including U+00A0 is a nice touch (both str.strip() and re.sub(r"\s", ...) handle it correctly), and the absent-vs-None-vs-blank distinction is properly pinned by three separate tests.
• Documentation actually explains why, not just what.

None of the above is blocking. Items 1 and 2 are the ones I'd want addressed before merge — the first because it means users won't see the benefit of the fix, the second because the same bug is still live one branch away with a test that reads like it's covered.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read PR diff)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 82cec11. The follow-up commit addresses most of the earlier round cleanly:

• ✅ HTML fallback no longer uses get_text(separator="\n") — block boundaries are marked explicitly and the regression test now asserts on the extracted string (test_extractor_fidelity.py:277), so it actually pins the behaviour.
• ✅ count == 0 is rejected independently of min_excerpt_length, with its own NO_QUOTED_TEXT_MESSAGE.
• ✅ STUB_NOTICE_PHRASES collapsed to a broad "restricted" under the length gate, with the two new PMC wordings covered by parametrized tests.
• ✅ PMIDSource encodes to bytes so the declared bytes contract holds; discard is logged at info; the plugin reports under excerpt_field rather than a hardcoded key; skip/trusted precedence is documented and pinned by test_blank_check_precedes_skip_references / test_blank_check_precedes_trusted_low_similarity.

Note: uv run pytest was denied approval in this environment again, so everything below is static reading, not a test run.

Four things remain, in order of importance.

---

1. The new diagnosis never reaches the human-facing repair report

repairer.py:480-496 builds a careful description ("Supply a real quote from the reference, or remove the item") and message, but a blank excerpt lands in the removals bucket (repairer.py:608), and that renderer never prints description:

lines.append(f"    Similarity: {action.similarity_score*100:.0f}%")
lines.append(f"    Snippet: '{result.original_text[:60]}...'")

For supporting_text="" the user sees:

RECOMMENDED REMOVALS (low confidence):
  PMID:TEST001 at evidence[0].snippet:
    Similarity: 0%
    Snippet: '...'

Similarity: 0% implies a comparison was made against the reference (none was — the fetch was deliberately skipped), and Snippet: '...' reads like an ellipsis-only excerpt rather than an empty one. So the one place a curator actually looks is the one place the whole point of the PR is invisible, and an empty quote is indistinguishable from a fabricated one. Printing action.description in the removals branch — or splitting empty/too-short into their own section — would fix it. Worth a test on format_report output.

Fix this →

2. <br> inside a <p> still welds words together

html.py:74-75 turns <br> into "\n" — but only in the fallback, after the paragraph branch has already returned. On the common path, <p>Line one<br>Line two</p> still extracts as "Line oneLine two", because bare get_text() concatenates and <br> contributes no text.

That is pre-existing rather than a regression, but it is the same failure mode this PR exists to fix (words welded across markup), and test_html_fallback_treats_br_as_a_break (test_extractor_fidelity.py:301) now pins the correct behaviour on the path users hit less often. Hoisting the two <br> lines above the paragraphs = scope.find_all("p") block makes both paths agree and costs nothing.

Fix this →

3. Dead branch, and a doc page still describing the message it emitted

Now that check_excerpt_content rejects count_quoted_characters() == 0, the empty-query branch in find_text_in_reference is unreachable:

query_parts = self._split_query(supporting_text)
if not query_parts:                        # supporting_text_validator.py:353
    return SupportingTextMatch(found=False, error_message="Query is empty after removing brackets and splitting")

count_quoted_characters sums over the same _split_query output, and every retained part has at least one non-whitespace character (_split_query filters on p.strip()), so count == 0 ⟺ query_parts == []. The branch can never run. Given CLAUDE.md's line on defensive code that isn't called for, it should go.

Related and more user-visible: docs/troubleshooting.md:295-299 documents Error: Query is empty after removing brackets as a symptom to look for. That string is no longer produced by anything — users now get NO_QUOTED_TEXT_MESSAGE. how-it-works.md and repair-validation-errors.md were updated; this page was missed.

Fix this →

4. Still open from the last round: stale cached stubs

ReferenceFetcher.get_cache_path() is unchanged and keys purely on reference ID, and no doc in this PR tells users to clear references_cache/. Anyone who validated Morris 2019 before this PR keeps getting the 8.7 KB stub served from cache and keeps seeing correct snippets rejected, with nothing in the output explaining why — the extractor fix simply doesn't reach them.

fetch(force_refresh=True) exists (reference_fetcher.py:97) but isn't exposed on the CLI, and there is no cache clear command, so the only remedy is rm -rf on the cache dir. If a version stamp is out of scope for this PR, a line in the release notes / troubleshooting doc saying "clear your reference cache after upgrading" would at least make it diagnosable.

5. Smaller items

• xml.py:90 — data.decode("utf-8") will raise UnicodeDecodeError on XML that declares a non-UTF-8 encoding. BeautifulSoup(data, "xml") accepts bytes and honours the declaration, so dropping the manual decode is strictly safer. Pre-existing behaviour, but it's now on the shared path for every XML source rather than just pmid.py. Relatedly, since PMIDSource now encodes to bytes, the isinstance(data, bytes) tolerance has no remaining production caller.
• Broad "restricted" + 1000-char gate — this is the trade I suggested and it's the right one, but note the residual: a genuinely short body (correction, erratum, brief comment) that uses the word is silently dropped. The info log makes it diagnosable, which is enough.
• cli/repair.py:326-338 — the isinstance(text_value, str) guard drops a non-string supporting_text (e.g. a list) where the old or chain passed it through. Almost certainly an improvement; still unmentioned in the PR description.
• Migration note — the plugin now emits an ERROR for any present-but-empty-string excerpt on any instance. Data using supporting_text: "" as a placeholder will light up (None is correctly left alone). Worth a release-note line.
• check_excerpt_content runs twice per validate() (once directly, once via find_text_in_reference). Harmless — two regex passes — just noting it's redundant rather than accidental.

---

What's good

• The fallback fix is the right shape: marking block boundaries and then letting get_text() run bare preserves source spacing inside a block while still separating blocks — and test_html_fallback_separates_adjacent_block_elements / ..._headings_and_list_items guard the direction the fix could have over-corrected in.
• Tightening the fallback test to assert text == "Variants near (GUSB, GRN, and NEU1) were found." with a docstring explaining why it doesn't go through the matcher is exactly right — the previous version would have passed on corrupted text.
• is_stub_notice's doctests now demonstrate both the wordings a phrase list would miss and the length gate that makes breadth safe.
• The skip/trusted precedence tests state the semantics in prose before asserting them, and test_skip_references_still_honoured_for_real_text guards the other side of the precedence change.
• count_quoted_characters counting non-whitespace characters (not tokens) remains the best call in the PR; the chemical-nomenclature and PDF-damage cases are pinned in both doctests and parametrized tests.
• Blank REMOVAL actions can't be auto-applied — cli/repair.py:388 gates on is_repaired, which is False here. Verified rather than assumed.

Items 1–3 are small and worth doing before merge; 4 is the one that decides whether users actually see the benefit.

--- · Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read PR diff)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at f8665a9. The third commit lands almost everything from the last round:

• ✅ Empty excerpts got their own EMPTY_EXCERPT action type and report section — the Similarity: 0% / Snippet: '...' misreporting is gone, _format_excerpt no longer fakes truncation on short quotes, and format_report output is pinned by four new tests.
• ✅ <br> → newline hoisted above the paragraph branch (html.py:56-57), with test_html_paragraph_treats_br_as_a_break covering the common path.
• ✅ The empty-query guard moved down into _substring_match rather than being deleted. That's the better call than what I suggested: _substring_match([], ...) was returning found=True, similarity=1.0, so the primitive itself was the vacuous pass. The doctest at supporting_text_validator.py:505 pins it.
• ✅ docs/troubleshooting.md updated for the new message and gained a cache-clearing section. I checked ReferenceFetcher._cache_path — the references_cache/PMID_30598549.md filename in the doc is correct.

Note: uv run pytest was denied approval in this environment for the third time, so everything below is static reading, not a test run.

Two things, one of them introduced by this commit.

---

1. The new action type silently drops empty excerpts out of the CLI exit code

cli/repair.py:181:

if report.removal_count > 0 or report.unverifiable_count > 0:
    typer.echo("\n⚠ Manual review required for some items")
    raise typer.Exit(1)
elif report.suggested_count > 0:
    ...

Before f8665a9, a blank excerpt produced a REMOVAL action, so removal_count > 0 and repair data exited 1 with "Manual review required". Now it produces EMPTY_EXCERPT, which is counted by neither removal_count nor unverifiable_count, and suggested_count requires RepairConfidence.MEDIUM while the action is VERY_LOW. So all three branches miss it and the command falls through to raise typer.Exit(0).

Concretely: a YAML file whose only defect is supporting_text: "" now exits 0 with no warning line. The report body still names the problem, but any CI job gating on the exit status goes green on exactly the defect this PR exists to catch — and it was red before this commit. Splitting the report section was right; splitting the exit code with it was not intended.

report.empty_excerpt_count > 0 belongs in that first condition.

Fix this →

2. PMIDSource re-encodes to UTF-8 without rewriting the encoding declaration

xml.py now hands bytes straight to BeautifulSoup(data, "xml") so the parser honours the in-document declaration — good, and test_non_utf8_encoded_article_is_extracted covers it. But pmid.py:425-426 does:

if isinstance(xml_content, str):
    xml_content = xml_content.encode("utf-8")

Bio.Entrez.efetch returns a text handle, so this is the production path. If the article's XML declares encoding="ISO-8859-1", the bytes we produce are UTF-8 while the declaration still says ISO-8859-1 — and lxml picks the encoding from the declaration when given bytes. That's mojibake in accented author names and unit symbols, and no test covers it: the new regression test builds ISO-8859-1 bytes directly and never goes through _fetch_pmc_xml.

The commit message frames the bytes change as fixing non-UTF-8 articles, but on the PMID path the encode step reintroduces the mismatch. Either encode using the declared encoding, or leave the str alone and widen Extractor.extract to Union[bytes, str] (bs4 handles str by re-encoding and fixing up the declaration, which is the case the manual encode gets wrong). A test with a mocked handle returning a str carrying a non-UTF-8 declaration would settle which way it should go.

Fix this →

3. Smaller items

• tests/test_empty_snippets.py:342 — assert "Similarity" not in output is a whole-report assertion that only holds because the batch contains a single empty excerpt. Add one fabricated quote to that fixture later and the test fails for a reason unrelated to what it's testing. Asserting on the empty-excerpt section (or on "Similarity" not in output.split("EMPTY EXCERPTS")[1].split("RECOMMENDED")[0]) would be sturdier. test_report_still_shows_fabricated_removals already covers the other side, so the risk is only a confusing future failure.
• models.py:245-283 — removal_count, unverifiable_count and empty_excerpt_count are now three copies of the same six-line loop. A single _count_results_with(action_type) helper would make the fourth one a one-liner.
• min_excerpt_length → EMPTY_EXCERPT — a "too short" excerpt (test_repair_single_flags_too_short_for_removal, line 626) reports under a name that says empty, and lands in a section headed "EMPTY EXCERPTS (nothing to verify)". It did quote something; it just quoted too little. result.message says "too short" so a reader isn't misled, but the section heading is wrong for that case. Off by default, so low impact.
• Broad "restricted" + 1000-char gate — still the right trade; residual risk that a genuinely short erratum using the word is dropped, made diagnosable by the info log.
• Still unmentioned in the PR description: the isinstance(text_value, str) guard at cli/repair.py:326-338 drops a non-string supporting_text where the old or chain passed it through; and the plugin now emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone). Both are improvements — they just want a release-note line.

---

What's good

• Moving the guard into _substring_match instead of deleting it is a better read of the situation than my suggestion was: the branch wasn't defensive scaffolding, it was masking a primitive that returned found=True for an empty part list. The commit message reasons out the count == 0 ⟺ query_parts == [] equivalence explicitly rather than asserting it.
• EMPTY_EXCERPT as its own category, with the rationale stated where it's easy to get wrong ("a removal recommendation is a verdict reached by comparison; nothing is compared here"). can_auto_fix returning False for it is belt-and-braces on top of the is_repaired gate.
• _format_excerpt fixes a small honesty bug that wasn't reported — the old f"'{result.original_text[:60]}...'" appended an ellipsis unconditionally, so every quote in the removals section looked truncated.
• Hoisting <br> above the region lookup rather than duplicating it in both branches, with the comment explaining why placement matters.
• Test docstrings that state the failure mode before asserting (test_html_fallback_without_paragraphs_keeps_symbol_boundaries explaining why it deliberately bypasses the matcher) are doing real work — that one would have passed on corrupted text in its earlier form.
• Doc example filenames verified against _cache_path, not guessed.

Item 1 is a genuine regression from this commit and worth fixing before merge; item 2 is narrow but on the production PMC path.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 1c44a60)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 1c44a60. Both blocking items from the last round are genuinely fixed:

• ✅ Exit code — report.insufficient_excerpt_count > 0 joins the condition at cli/repair.py:181-187, with tests on both sides. I checked the "clean file" test's fixture text ("Protein X functions in cell cycle regulation") against test_repair_text_already_valid:36, which already relies on it matching PMID:TEST001 — so the exit-0 assertion is sound, not accidentally passing.
• ✅ PMC encoding — the encode("utf-8") is gone and Extractor.extract widened to Union[bytes, str]. This is the right call and robust across bs4 versions: LXMLTreeBuilderForXML.prepare_markup yields the raw str first and, on failure, retries as UTF-8 bytes with an explicit parser_for("utf8"), which overrides the in-document declaration either way. The regression test going through _fetch_pmc_xml rather than building bytes directly is the part that makes it cover the production path.
• ✅ INSUFFICIENT_EXCERPT rename, _count_results_with, tighter report assertion — all landed. No stale EMPTY_EXCERPT identifiers remain in src/.

Note: uv run pytest was denied approval in this environment for the fourth time, so everything below is static reading, not a test run.

Five things, two of them regressions introduced by the last two commits.

---

1. The docs still tell curators these are removals

docs/how-to/repair-validation-errors.md:298 and :310:

Empty excerpts cannot be repaired automatically … They are flagged for removal so you can supply the real quote or drop the evidence item.

Like empty excerpts, these are flagged for removal rather than repaired

That was true at 1bd7cd5 and stopped being true at f8665a9, which is the commit that split them out of removals precisely because a removal is a verdict reached by comparison. The report now prints INSUFFICIENT EXCERPTS (nothing to verify), removal_count deliberately excludes them, and RepairActionType.REMOVAL is never emitted for this case. A curator who follows the doc and greps the report for REMOVAL — or scripts against removal_count — will not find them.

This page was added by this PR, so it's the PR's own documentation contradicting the PR's own behaviour. Both paragraphs need the section name and the action type updated.

Fix this →

2. The min_excerpt_length doc snippet is at a nesting level that silently does nothing

docs/concepts/how-it-works.md:62-65:

# .linkml-reference-validator.yaml
min_excerpt_length: 20

_extract_validation_config_data (cli/shared.py:131-147) checks for a validation: key first and returns that section if present; the top-level-keys fallback at line 143 only runs when the file has neither validation: nor reference_validation:.

Both existing config examples in the docs — docs/setup-guide.md:380 and docs/reference/cli.md:615 — show files with a validation: section. So a user who follows the setup guide and then pastes this snippet at top level gets the key silently ignored: no error, no warning, and the check they believe they enabled never runs. For an opt-in safety check that is the worst possible failure mode, and it looks like it's working right up until bad data ships.

validation: {min_excerpt_length: 20} works in every case and is what the snippet should show. Worth adding the key to the two config-reference pages too — neither mentions it, so there's nowhere a user could discover the correct nesting.

Fix this →

3. repair text still prints the misleading similarity percentage

cli/repair.py:249:

typer.echo(f"    Confidence: {action.confidence.value} ({action.similarity_score*100:.0f}%)")

RepairAction.similarity_score defaults to 0.0 (models.py:114) and the INSUFFICIENT_EXCERPT action never sets it, so repair text "" PMID:X prints:

    Suggestion: INSUFFICIENT_EXCERPT
    Confidence: VERY_LOW (0%)

That (0%) is exactly the "meaningless Similarity: 0%" the commit comment at repairer.py:510 says the new action type exists to avoid — removed from format_report, still live one command over. Not a crash and the message above it is clear, but the two commands now disagree about how the same defect is presented. Suppressing the percentage when action_type == INSUFFICIENT_EXCERPT would make them agree.

4. The report-scoping fix doesn't yet exclude the section it was scoped against

tests/test_empty_snippets.py:342:

section = output.split("INSUFFICIENT EXCERPTS")[1].split("Summary:")[0]

format_report emits sections in the order insufficient → removals → unverifiable → Summary: (repairer.py:672-711). So the slice still spans the removals block, and assert "Similarity" not in section still fails if a fabricated quote is ever added to that batch — the exact future-confusing failure the scoping was meant to prevent. The comment on line 346 claims otherwise. Splitting on "RECOMMENDED REMOVALS" as well, or slicing to the first blank line, would actually pin it.

5. Smaller items

• PDFExtractor rejects str with no test (pdf.py:66-71). The base contract now advertises Union[bytes, str] and its docstring promises str works; this override raises for half that domain. Safe in practice — every call site passes bytes, and reference_fetcher.py:378 wraps extraction in try/except — but it's a new raise on a public API with no coverage, which CLAUDE.md's tests-first rule would want. A three-line pytest.raises(TypeError) in tests/test_extractors.py alongside the existing PDFExtractor tests would close it.
• _fetch_pubmed_xml still hand-decodes (pmid.py:258-259: xml_content.decode("utf-8")), two functions above the one this commit fixed for exactly that reason, with a comment now explaining why not to do it. Bs4 fixes up the declaration for str, so there's no mojibake — but a PubMed record whose bytes aren't UTF-8 raises UnicodeDecodeError and takes the MeSH/publication-type metadata with it. Pre-existing and outside the stated scope; just newly inconsistent with its neighbour.
• Broad "restricted" + 1000-char gate — still the right trade. Residual: a genuinely short erratum using the word is dropped, made diagnosable by the info log at xml.py:115.
• Still unmentioned in the PR description: the isinstance(text_value, str) guard at cli/repair.py:329-338 now drops a non-string supporting_text (a list, say) where the old or chain passed it through — an improvement, since the old path would have thrown a TypeError inside _split_query; and the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone). Both want a release-note line.

---

What's good

• The encoding fix reverses course on a wrong intermediate step and says so in the commit message, with the measured before/after rather than a claim. Widening the base type was the honest resolution — the alternative (encode to the declared encoding) would have re-created the mismatch it was trying to avoid.
• The exit-code regression is the kind that only shows up if you actually run the command; catching it, naming CI gating as the consequence, and pinning both directions (non-zero for the defect, zero for a clean file) is the right treatment. A one-sided test here would have been almost worthless.
• _count_results_with collapses three copies into one, and its doctest exercises the empty-report case rather than asserting the obvious.
• INSUFFICIENT_EXCERPT over EMPTY_EXCERPT is a small rename doing real work: test_repair_single_flags_too_short_for_removal was landing a quote that did quote something under a heading that said "empty".
• _format_excerpt distinguishes (empty) from a real quote from a genuinely truncated one, and test_report_does_not_imply_truncation_of_short_text pins the case that was quietly wrong before anyone reported it.
• The cache-staleness item from round one is now documented in docs/troubleshooting.md with a correct references_cache/PMID_30598549.md path — a doc fix rather than a code fix, but it's the one that decides whether users see the benefit, and it's diagnosable now.
• LONG_BODY_FILLER at ~1.6 KB is comfortably past MAX_STUB_NOTICE_CHARS, so the fidelity tests aren't sitting on the gate boundary by luck.

Item 1 is the one I'd fix before merge — the PR's own docs describe behaviour the PR removed. Item 2 is smaller in scope but fails silently, which makes it worth the same attention.

--- · Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 6e57488)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 6e57488. Both items from the last round are fixed, and the smaller ones with them:

• ✅ repair-validation-errors.md now names INSUFFICIENT EXCERPTS and INSUFFICIENT_EXCERPT, says why they are not removals, and adds the detail that matters most for scripting — removal_count excludes them but repair data still exits non-zero. I checked that against cli/repair.py:181-187; it's accurate.
• ✅ min_excerpt_length is nested under validation: with a note on why placement matters, and documented in setup-guide.md:388 and reference/cli.md:625. I re-read _extract_validation_config_data (cli/shared.py:131-147) — the "silently ignored at top level when a validation: section exists" claim in the doc is exactly right.
• ✅ repair text no longer prints VERY_LOW (0%), with tests on both sides.
• ✅ The report test now truncates at RECOMMENDED REMOVALS, so it actually excludes the section it claimed to.
• ✅ PDFExtractor's str rejection has the test it should have shipped with.

Note: uv run pytest and uv run python were both denied approval in this environment for the fifth time, so everything below is static reading, not a test run.

One new regression, plus smaller items.

---

1. The PMC encoding fix landed on one of the two call sites — and the busier one still has the bug

etl/fulltext/pmc.py:100-108:

def _fetch_pmc_xml_bytes(self, pmcid, config) -> Optional[bytes]:
    ...
    xml_content = handle.read()
    if isinstance(xml_content, str):
        xml_content = xml_content.encode("utf-8")   # <-- the line 1c44a60 deleted from pmid.py
    return xml_content

This is the same encode("utf-8") that 1c44a60 removed from PMIDSource._fetch_pmc_xml for exactly this reason, and the comment left behind there (pmid.py:425-430) explains the failure it causes. The provider is the live path: ReferenceFetcher._enrich_with_full_text (reference_fetcher.py:212-227) resolves through FullTextProviderRegistry, and fetch_full_text is on by default — so PMCFullTextProvider.locate is where most PMC full text now comes from, more than PMIDSource._fetch_pmc_xml.

It's a regression introduced by this PR, not pre-existing. On origin/main, XMLExtractor.extract opened with data.decode("utf-8") if isinstance(data, bytes) else data, so this encode round-tripped straight back to str and bs4 fixed up the declaration. As of 1c44a60 the bytes go untouched to BeautifulSoup(data, "xml"), which honours the in-document declaration — the behaviour test_non_utf8_encoded_article_is_extracted pins. So an article declaring ISO-8859-1 now yields UTF-8 bytes read as ISO-8859-1: François → François in author names, µ → µ in unit symbols, silently, in cached full text.

The fix is the one already applied next door: drop the encode, return the str, and widen the annotation to Optional[Union[bytes, str]] — Extractor.extract accepts both now precisely so this works. test_pmc_xml_fetch_preserves_accents_from_non_utf8_article is the right shape to copy for it.

Fix this →

2. Extraction and write-back disagree about which key the quote came from

cli/repair.py:337-352 now prefers the first non-blank of ("supporting_text", "snippet"). But _apply_repairs_to_data (cli/repair.py:413-417) picks the key unconditionally:

text_key = "supporting_text" if "supporting_text" in d else "snippet"

For the exact dict the new test_extract_evidence_items_prefers_non_blank_key (test_empty_snippets.py:465) uses — supporting_text: "", snippet: "real text" — a --no-dry-run repair of the snippet writes the corrected text into supporting_text and leaves snippet wrong. The or-chain on main selected the same value, so the mismatch is pre-existing rather than introduced; what's new is that a test now pins the fixture that triggers it, which makes it look settled.

Two smaller consequences of the same preference, both worth a line in the PR description since they qualify its claims:

• With both keys present and one blank, the blank one is still silently dropped — "preserves blank snippets for reporting" holds only when a single excerpt key is present.
• test_extract_evidence_items_requires_reference pins that a blank excerpt with no reference is dropped by repair data, while the plugin reports it (reference_validation_plugin.py:193-209, comment: "it may have no reference at all"). Defensible — repair needs something to repair against — but the two layers disagree and only one of them says so.

3. Smaller items

• pmc.py:110-124 — _fetch_pmc_html still carries its own find_all("p") + "\n\n".join(p.get_text()), the same private copy this PR deleted from pmid.py in favour of the shared extractor. It happens not to have Bug 2 (no strip=True), but it also gets none of the fixes: no <br> handling, no block fallback, no <script>/<style> removal. Routing it through HTMLExtractor would close the last duplicate.
• docs/troubleshooting.md:318-341 — the new cache section tells users to rm references_cache/PMID_30598549.md. cache reference PMID:30598549 --force (cli/cache.py:120,146) already re-fetches in place and is the friendlier remedy; the rm is the fallback when you want to clear everything. Worth naming the command, since a user reading this page has no reason to know it exists.
• tests/test_cli_repair.py:604 — assert "%" in result.stdout passes on a % anywhere in the output and doesn't pin that it's the confidence line, or that the command took the "Could not repair" branch at all. assert "VERY_LOW (" in result.stdout (or asserting on the Confidence: line) would test what the docstring says it tests.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, two functions above the one fixed for that, now with a comment next door explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Pre-existing and out of the stated scope; just newly inconsistent with its neighbour.
• Broad "restricted" + 1000-char gate — still the right trade; residual risk that a short erratum using the word is dropped, made diagnosable by the info log at xml.py:110.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone by the isinstance guard). And validate() now rejects blank excerpts ahead of skip_prefixes, so a blank excerpt on a skipped prefix changes from INFO/valid to ERROR. Both are the point of the PR — they just want a release-note line.
• check_excerpt_content runs twice per validate() (directly, then inside find_text_in_reference), and _split_query now runs three times per validation. Two extra regex passes on a string that's about to be matched against a 155 KB document — noting it's redundant rather than accidental.

---

What's good

• The two doc fixes are the kind that only surface if you run the code rather than read it, and the commit message shows the measured min_excerpt_length -> 0 vs -> 20 rather than asserting the nesting matters. Documenting the key in setup-guide.md and reference/cli.md too is what makes the correct nesting discoverable — the doc fix alone would have left a reader with no way to check.
• Naming the failure mode precisely ("it looks enabled while never running") is why this was worth fixing at all: a silently-ignored opt-in safety check is worse than an absent one.
• repair text and format_report now agree about the percentage, and the test says why rather than just asserting "0%" not in output.
• test_pmc_xml_fetch_preserves_accents_from_non_utf8_article goes through _fetch_pmc_xml with a mocked handle returning str, which is what makes it cover the production path rather than the extractor in isolation — and it's the test that let me establish item 1 above is real.
• The PDFExtractor str test states the contract asymmetry (base accepts Union[bytes, str]; PDF is the one format where that's meaningless) instead of just asserting the raise.
• Across the whole PR, the layering has held up under four rounds of revision: check_excerpt_content() as the single source of truth, INSUFFICIENT_EXCERPT as its own category with its own report section and exit-code contribution, and the guard pushed down into _substring_match so the primitive can't pass vacuously if called from somewhere new.

Item 1 is the one I'd fix before merge — it's a regression this PR introduces on the default full-text path, and it corrupts cached content silently.

--- · Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 8e679bf)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 8e679bf. Both items from the last round are fixed, and correctly:

• ✅ _fetch_pmc_xml_bytes no longer re-encodes (etl/fulltext/pmc.py:101-115). The encode("utf-8") is gone, the return type is widened to Optional[Union[bytes, str]], and the docstring names the failure it avoids. test_pmc_provider_preserves_accents_from_non_utf8_article goes through the provider method with a mocked handle returning str, mirroring the PMIDSource test — so both call sites for the same XML are now pinned, not just the one that was reported.
• ✅ Extraction and write-back share _excerpt_key (cli/repair.py:315-345). One function, used by both _extract_evidence_items:372 and _apply_repairs_to_data:440, so the two can't diverge by construction rather than by convention. test_repair_writes_back_to_the_key_it_read_from pins the supporting_text: "" / snippet: "quote" case that previously wrote the fix into the wrong field, and its sibling pins the ordinary case so the fix can't be a blanket switch to snippet.
• ✅ Bonus: _fetch_pmc_html in the provider now routes through HTMLExtractor instead of its own find_all("p") walk — the last duplicate I flagged as a smaller item.

Note: uv run pytest was denied approval in this environment for the sixth time, so everything below is static reading, not a test run. CI (Build and test, run 32532087016) was still in progress when I finished.

One item, plus smaller notes.

---

1. The HTML consolidation landed on one of the two sites — and PMIDSource still has the private copy

etl/sources/pmid.py:461-465:

if article_body:
    paragraphs = article_body.find_all("p")
    if paragraphs:
        text = "\n\n".join(p.get_text() for p in paragraphs)
        return text

This is the same private paragraph walk that 8e679bf just replaced in PMCFullTextProvider._fetch_pmc_html, in the same module whose _fetch_pmc_xml this PR routed through the shared extractor with the comment "so body parsing and PMC placeholder detection cannot drift from the rest of the ETL layer" (pmid.py:430-433). It's live: pmid.py:360 calls it as the HTML fallback in PMIDSource's own full-text path.

It doesn't have Bug 2 (no strip=True), but it gets none of this PR's other HTML work: no <br> → newline, so <p>Line one<br>Line two</p> still yields "Line oneLine two"; no <script>/<style> removal, so JSON-LD and CSS land in cached full text; no block-boundary fallback when the region has no <p>. Pre-existing rather than a regression — but this is the third round where a fix landed on one PMC call site and not its twin, and the fix here is the one-liner already written next door: return HTMLExtractor().extract(str(article_body), content_type="text/html").

Fix this →

2. Smaller items

• pmc.py:60 — xml_bytes = self._fetch_pmc_xml_bytes(...). Both the variable and the method name still say bytes, and the whole point of the change is that it's a str on the production path. _fetch_pmc_xml / xml_content would stop the next reader from re-adding the encode to make the name true again.
• _fetch_pmc_html no-paragraph behaviour change — the old code returned None when the region held no <p>; HTMLExtractor now falls back to the region's block-separated text. Almost certainly an improvement (and _MIN_PMC_FULLTEXT_CHARS still gates it at locate()), but it's a silent widening of what counts as full text.
• HTMLExtractor re-scopes inside the region — str(article_body) is re-parsed and extract then does soup.find("article") or soup.find("main") (html.py:61). If a PMC div.article-body ever nests an <article>, the extractor narrows to it and drops the rest. Low likelihood; worth knowing the region selection happens twice.
• Only one excerpt key is ever validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second one silently unchecked and unrepaired. The plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187), so the two layers disagree. Pre-existing (the old or chain did the same), but the helper now makes it look deliberate.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, now with two neighbouring comments explaining why not to. No mojibake, but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of scope; just the last inconsistent one.
• Broad "restricted" + 1000-char gate — still the right trade; residual risk that a short erratum using the word is dropped, made diagnosable by the info log at xml.py:115.
• Still unmentioned in the PR description: the plugin now emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone by the isinstance guard); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so the change isn't a surprise on first run.

---

What's good

• _excerpt_key is the right shape for this fix: the bug was two call sites choosing independently, so the fix is one function they both call, not two matching edits. The docstring says that explicitly ("Used by both extraction and write-back so a repair cannot land in a different field than the quote came from"), and the doctests cover all four cases including the blank-only fallback that the reporting path depends on.
• Pinning both directions of the write-back — the non-blank-snippet case and the ordinary supporting_text case — is what stops the fix from being "always write to snippet". A one-sided test would have passed on a wrong fix.
• The encoding comment on _fetch_pmc_xml_bytes states the concrete corruption ("François" → "François") rather than a general warning about encodings, which is what makes it survive the next refactor.
• Fixing the HTML duplicate in the same commit, unprompted, is the part that actually reduces the surface — the two encoding sites diverged because nobody looked for the twin, and this one was found by looking.
• test_pmc_provider_html_preserves_inline_markup asserts both directions ("(GUSB, GRN)" in text and "Line oneLine two" not in text), so it covers the fix the delegation buys rather than just that delegation happened.
• Across six rounds the layering has held: check_excerpt_content() as the single source of truth, INSUFFICIENT_EXCERPT as its own category with its own report section and exit-code contribution, the vacuous-pass guard pushed down into _substring_match, and now a single excerpt-key resolver. Each round's fix has been at the shared level rather than at the reported site.

Item 1 is a leftover of the same shape as the last two rounds rather than a new defect, and it's a one-line change. Nothing here blocks merge.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 334aa8e)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 334aa8e. The one item from the last round is fixed, correctly and in the right place:

• ✅ PMIDSource._fetch_pmc_html delegates to HTMLExtractor (pmid.py:462-466). The region is still selected locally and only the text extraction is delegated, which is the same split _fetch_pmc_xml and the PMC provider use — so all three PMC paths now share one text-extraction implementation. test_pmid_source_html_fallback_preserves_inline_markup asserts both directions ("(GUSB, GRN)" in text, "Line oneLine two" not in text), mirroring the provider test.
• The commit message is honest about scope: it says plainly that nothing was broken here (this copy never had the strip=True welding bug) and that the change is about the other fixes it was missing. That's the right framing for a consolidation commit — it doesn't inflate a cleanup into a bug fix.

Note: uv run pytest and uv run python -m pytest were both denied approval in this environment for the seventh time, so everything below is static reading, not a test run. CI (Build and test, run 32543537707) was still in progress when I finished.

Nothing blocking. Findings this round are all structural follow-ons from the consolidation.

---

1. Three call sites now round-trip through str(), and re-run region selection inside the extractor

All three PMC paths now do the same thing:

HTMLExtractor().extract(str(article_body), content_type="text/html")   # pmid.py:466, pmc.py:131

Two consequences worth knowing about, neither of which the tests would catch:

• Double parse. article_body is already a parsed Tag. Serialising it back to markup and re-parsing costs a second full html.parser pass over the article body on every fetch — on a 155 KB paper that's not free, and it happens on the default full-text path.
• Region selection runs twice, and the second one can narrow. HTMLExtractor.extract opens with soup.find("article") or soup.find("main") (html.py:61). The caller already chose div.article-body; if that div ever nests an <article> or <main>, the extractor silently narrows to it and drops the surrounding body. Unlikely on today's PMC markup, but it's a scoping decision the caller believes it already made.

Both go away with a scope-taking entry point — HTMLExtractor().extract_scope(article_body) doing the <br>/paragraph/block work on the Tag it's handed, with extract() as the parse-then-delegate wrapper. That's the shape the three call sites are already reaching for.

Fix this →

2. The two delegation tests don't exercise the fallback their docstrings cite

test_pmid_source_html_fallback_preserves_inline_markup:483 and test_pmc_provider_html_preserves_inline_markup:456 both name the block-boundary fallback as something the delegation buys — but both fixtures are two <p> elements, so HTMLExtractor takes the paragraph branch at html.py:65 and returns before the fallback at line 82 ever runs. What's actually pinned is <br> handling and inline-markup spacing.

That's not a wrong test, just a docstring that claims more coverage than the assertions provide — and the fallback is exactly the path where the old code returned None and the new one returns text (see item 3). A third fixture with a <div>-only region, asserting the region's text comes back rather than None, would close both.

3. 1000 is now three independent constants, and the HTML paths lean on the coincidence

is_stub_notice is only ever applied on the XML path (xml.py:114). Neither HTML path has a placeholder check — they rely purely on a length gate:

Site	Value
xml.py:29	MAX_STUB_NOTICE_CHARS = 1000
pmc.py:32	_MIN_PMC_FULLTEXT_CHARS = 1000
pmid.py:358,362	bare literal 1000, twice

Today that works out exactly: a notice is_stub_notice would catch is by definition ≤ 1000 chars, and every HTML caller rejects text ≤ 1000 chars. But the coverage is a numeric coincidence across three uncoupled declarations, and the two in pmid.py aren't even named. Raise MAX_STUB_NOTICE_CHARS to catch a longer notice wording and the HTML paths silently start accepting stubs in the 1000–N band. Given this PR's own theme — the same logic in two places drifts — one shared constant (and ideally running is_stub_notice on the HTML text too) is the cheap version of the fix.

4. Smaller items

• _fetch_pmc_xml_bytes still says bytes (pmc.py:60,101) — carried unaddressed from the last round. The whole point of 8e679bf was that this returns a str on the production path; both the method name and the xml_bytes local now contradict the docstring directly above them. That's precisely the kind of name that invites the next reader to re-add the encode("utf-8") to make it true again. One rename.
• Silent widening of _fetch_pmc_html — both providers previously returned None when the region held no <p>; they now return the region's block-separated text. Almost certainly an improvement, gated by the ≥1000-char checks, but it's a change in what counts as full text that no test or note records.
• Only one excerpt key is ever validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked. The plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187), so the two layers still disagree. Pre-existing (the old or chain did the same), but the named helper now makes it look deliberate.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, now with two neighbouring comments explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Broad "restricted" + 1000-char gate — still the right trade; residual risk that a short erratum using the word is dropped, made diagnosable by the info log at xml.py:115.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone by the isinstance guard); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• The delegation keeps region selection at the call site and moves only text extraction. That's the correct seam: the region is a source-specific fact (div.article-body vs div.tsec is a PMC detail), while <br> handling and inline-markup spacing are not. A blunter consolidation would have pushed the PMC selectors into the extractor.
• Naming the change accurately — "It never had the welding bug, so nothing was broken; it simply received none of this PR's HTML work" — is what makes the commit reviewable. Over seven rounds the commit messages have consistently distinguished fixed a bug from closed a drift, and that distinction has held up every time I checked it against the code.
• Three rounds running, the fix has landed on the shared level rather than the reported site: check_excerpt_content() as the single source of truth for excerpt substance, _excerpt_key as the single resolver for read and write-back, and now one HTML text extractor for all three PMC paths. Each of the last three rounds surfaced a twin of the reported defect; the surface for that class of finding is now essentially closed.
• is_stub_notice's design — breadth in the phrase list, safety from the length gate — remains the right inversion of the original bug, and the doctest demonstrates both halves rather than asserting them.
• _substring_match's empty-parts guard is still the load-bearing invariant: it keeps the primitive honest even as new callers (the plugin, the repairer, repair text) were added around it over the course of the PR.

I'd take the _fetch_pmc_xml_bytes rename (item 4) since it's a one-liner guarding against a regression this PR already had to fix twice; the rest is optional follow-up. Nothing here blocks merge.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at dd2af3f)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at dd2af3f. Both items from the last round are addressed, and the seam is in the right place:

• ✅ extract_scope() takes an already-parsed tag, so the three PMC paths stop serialising and re-parsing the article body, and the <article>/<main> selection no longer re-runs inside a region the caller already chose. test_extract_scope_does_not_reselect_the_region pins the nested-<article> case that would have silently dropped everything around it.
• ✅ _fetch_pmc_xml_bytes → _fetch_pmc_xml_source, with the docstring saying why it deliberately returns whatever Entrez gave it. That's the rename I asked for; the old name was an invitation to re-add the encode() this PR had to remove twice.
• ✅ One 1000. _MIN_PMC_FULLTEXT_CHARS = MAX_STUB_NOTICE_CHARS and the two bare literals in pmid.py:358,362 now import it. The comment states the real reason (only the XML path runs is_stub_notice, so on HTML the length gate is the whole defence).
• ✅ The two delegation-test docstrings no longer claim fallback coverage their <p> fixtures never reach, and the fallback got its own test that also records the None → region-text widening.

Note: uv run pytest was denied approval in this environment for the eighth time, so everything below is static reading, not a test run. CI (Build and test, run 32576523278) was still in progress when I finished.

One regression introduced by this commit, plus smaller notes.

---

1. extract_scope drops <script>/<style> removal — and it's the only stub defence on the HTML paths

html.py:52-56 — the decompose stayed in extract(), above the delegation:

def extract(self, data, *, content_type=None):
    soup = BeautifulSoup(data, "html.parser")
    for tag in soup(["script", "style"]):
        tag.decompose()
    region = soup.find("article") or soup.find("main")
    return self.extract_scope(region if region is not None else soup)

def extract_scope(self, scope):      # no decompose here

All three PMC callers now enter at extract_scope (pmc.py:136, pmid.py:466), so none of them strip script or style any more. Until this commit they went through extract(str(article_body)) and did. Script/style removal was one of the fixes the last two rounds cited as the reason to consolidate onto the shared extractor — the consolidation is now complete and the fix is gone from every consolidated caller.

The paragraph branch mostly escapes (a <script> inside a <p> is rare), but the block fallback at html.py:106-110 runs scope.get_text() over everything, so inline JSON-LD, MathJax config and analytics land verbatim in cached full text. That fallback is exactly the path this commit newly made reachable.

The consequence is worse than noise, and it's the one the commit message itself sets up. pmc.py:36 says the length gate is the only thing standing between a placeholder page and the cache on HTML. A stripped PMC landing page carries little prose but plenty of inline script — so the script text counts toward len(html_text) > _MIN_PMC_FULLTEXT_CHARS, pushes a sub-1000-char stub over the line, and it gets cached as full text. Every subsequent snippet check runs against JavaScript.

test_extract_scope_matches_extract_for_a_plain_region:535 is the test that should have caught this, but its fixture is a single <div><p> with no script or style, so the two entry points agree there by construction. Moving the decompose into extract_scope fixes both paths, and adding a <script> to that equivalence fixture makes it load-bearing.

Fix this →

2. extract_scope mutates the tag it is handed

html.py:85-86 and :106-107 both edit the caller's tree in place — line_break.replace_with("\n"), block.insert_after("\n"). Under extract() that was invisible (the soup was local and discarded); as a public entry point taking a caller-owned tag, it isn't.

Two things follow. Calling it twice on the same region is not idempotent: the second call inserts a second "\n" after every block, so the fallback's output grows separators on each pass. And a caller that reads article_body after extraction — none do today, but pmid.py:457 and pmc.py:129 both keep the parent soup alive — sees a modified tree. Either say so in the docstring ("the tag is consumed") or work on copy.copy(scope). The docstring currently reads as a pure accessor.

3. The constants are coupled in the direction that helps, and also the one that hurts

_MIN_PMC_FULLTEXT_CHARS = MAX_STUB_NOTICE_CHARS makes raising the notice length safe, which is the case the comment reasons about. The reverse is the one to watch: lowering MAX_STUB_NOTICE_CHARS — the obvious response to the residual risk this PR has carried for five rounds, that a short erratum saying "restricted" is dropped — silently lowers the PMC HTML gate with it, admitting stubs in the new band. The two constants now move together but their safe directions are opposite, so the tie-together needs the second half of the argument written down next to the first.

pmid.py:358,362 also uses MAX_STUB_NOTICE_CHARS bare where pmc.py aliases it with three lines of rationale, so the same coupling reads as deliberate in one file and incidental in the other.

4. Smaller items

• extract_scope(scope: Any) (html.py:58) — Any disables checking on the one argument that has a real contract. bs4 is imported # type: ignore so Tag isn't usable as a checked annotation, but importing it for documentation value still beats Any; as written, extract_scope("<div>…</div>") type-checks and then fails at .find_all.
• HTMLExtractor class docstring (html.py:26) still says "from HTML bytes"; extract has accepted Union[bytes, str] since 1c44a60 and extract_scope takes neither.
• Only one excerpt key is ever validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, now with three neighbouring comments explaining why not to. No mojibake, but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• The split is the right one. extract() owns finding the region in a whole document; extract_scope() owns reading a region it's given. That keeps the PMC selectors (div.article-body, div.tsec) at the source, where they belong, while the <br>/paragraph/block handling has exactly one implementation. The three call sites were already reaching for this shape by passing str(tag).
• test_extract_scope_does_not_reselect_the_region tests the silent half of the bug: the double parse is a cost, the re-narrowing is a correctness hazard, and the fixture is built so a nested <article> would have eaten the surrounding paragraph. Asserting both paragraphs survive pins it precisely.
• Correcting the two test docstrings rather than leaving them is the part I'd have expected to slip. Both claimed fallback coverage their <p> fixtures never reach — a docstring that overstates coverage is worse than no docstring, because the next reader stops looking. The new fallback test also records the None → region-text widening that three rounds of review noted and nothing pinned.
• The commit message continues to distinguish fixed a bug from closed a drift, and the constants paragraph reasons from why the coincidence held rather than just declaring the dedup.
• Four rounds running, each fix has landed at the shared level rather than at the reported site: check_excerpt_content(), _excerpt_key, one HTML text extractor, now one length constant. The class of finding that dominated rounds five through seven — a fix applied to one PMC call site and not its twin — is closed.

Item 1 is a regression this commit introduces on the default full-text path and worth fixing before merge; 2 and 3 are cheap hardening on the API this commit just made public.

--- · Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 58735b0)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 58735b0. The blocking item from the last round is fixed, and the commit message is more honest than my report was:

• ✅ The decompose moved into extract_scope (html.py:84-85), so all three PMC callers get it. I checked the claim in the commit message rather than taking it: bs4's HTMLTreeBuilder.string_containers maps script/style to Script/Stylesheet, and get_text() filters on interesting_string_types = {NavigableString, CData} — so script and style text was indeed already excluded, and my "JSON-LD lands in cached full text" claim was wrong. The commit says so plainly and makes the case for the change on the correct grounds (a guarantee three call sites depend on shouldn't rest on a library default). That's the right disposition of a review finding that was half right.
• ✅ I also checked the move doesn't change extract(): the decompose used to run over the whole soup before region selection, and now runs inside the region. Script/style content can't create <article>/<main> tags (html.parser treats their contents as raw text) and text outside the region was never emitted, so the two are equivalent. test_extract_scope_drops_script_and_style pins the new entry point directly, which is better than the fixture change I suggested.

Note: uv run pytest was denied approval in this environment for the ninth time, so everything below is static reading, not a test run. CI (Build and test, run 32576833339) was still in progress when I finished.

Nothing blocking. Two carryovers and small notes.

---

1. extract_scope now destroys nodes in the caller's tree, and still documents itself as an accessor

Carried from the last round, and this commit sharpens it. Before 58735b0 the mutations were additive (replace_with("\n") at html.py:91, insert_after("\n") at :112); now tag.decompose() at :85 permanently removes nodes from a tree the caller owns and keeps alive (pmid.py:457, pmc.py:129 both hold the parent soup).

Two consequences, neither pinned:

• Not idempotent. A second extract_scope(region) inserts a second "\n" after every block, so the fallback's output grows separators on each pass. Nothing calls it twice today; nothing stops it either.
• The docstring reads as pure. "A BeautifulSoup tag or soup to take the text of" gives no hint that the tag comes back stripped. The natural next caller — one that extracts text and then reads article_body for figure captions or a DOI — gets a tree with pieces missing.

copy.copy(scope) (bs4 Tag supports it and it's far cheaper than the serialise-and-reparse this method replaced) makes it genuinely pure; a "the tag is consumed" line in the docstring is the zero-cost version.

Fix this →

2. The two constants are coupled in the direction that helps, and also the one that hurts

Also carried. _MIN_PMC_FULLTEXT_CHARS = MAX_STUB_NOTICE_CHARS (pmc.py:32) makes raising the notice length safe, which is what the comment reasons about. Lowering it is the hazard: that's the obvious response to the residual risk this PR has carried for six rounds — a short erratum saying "restricted" gets dropped — and it silently lowers the PMC HTML gate with it, admitting stubs in the new band. The two move together but their safe directions are opposite, and only one half of that is written down. pmid.py:358,362 uses MAX_STUB_NOTICE_CHARS bare, so the same coupling reads as deliberate in one file and incidental in the other.

3. Smaller items

• scope(["script", "style"]) then scope.find_all("br") (html.py:84,90) — the call shorthand and the explicit method, six lines apart, doing the same thing. find_all in both places reads better next to the find_all(BLOCK_LEVEL_TAGS) below.
• extract_scope(scope: Any) (html.py:54) — Any on the one argument with a real contract; extract_scope("<div>…</div>") type-checks and then fails at .find_all. bs4 is imported # type: ignore so Tag isn't checkable, but it still documents better than Any.
• HTMLExtractor class docstring (html.py:26) still says "from HTML bytes" — extract has taken Union[bytes, str] since 1c44a60, and extract_scope takes neither.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, now with three neighbouring comments explaining why not to. No mojibake, but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Verifying the reported claim against the installed bs4 and reporting that it was wrong — with the actual interesting_string_types value — is the part that makes this commit trustworthy. The easy version was to move the decompose, cite the review, and let "JSON-LD in cached full text" stand as the justification. Distinguishing this was broken from this held for a reason we don't control is the same discrimination the last four commit messages made, applied against the reviewer this time.
• Keeping the change anyway is the right call on the stated grounds: pmc.py:36 already leans on the length gate as the only stub defence on HTML, and adding a second dependency on a bs4 default that a builder swap or a future interesting_string_types change could silently drop is the kind of coupling that surfaces as corrupted cached text rather than as a test failure.
• The dedicated extract_scope test rather than a fixture tweak to the equivalence test: the equivalence assertion answers "do the two entry points agree", and loading it with a second concern would have made a future failure ambiguous about which property broke.
• Nine rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT as its own category with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor for all three PMC paths, one length constant. Every round's fix landed at the shared level rather than at the reported site, which is why the twin-call-site findings that dominated rounds five through seven stopped appearing.

Item 1 is worth the one-line docstring at minimum, since extract_scope is now a public entry point whose contract is stronger than its documentation. Nothing here blocks merge.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 8ad07e9)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 8ad07e9. Both carryovers from the last round are closed, and the smaller notes with them:

• ✅ extract_scope is pure. The copy fixes both halves — the caller's tree survives (test_extract_scope_leaves_the_caller_tree_alone) and repeated calls agree (test_extract_scope_is_idempotent, whose <div>-only fixture correctly lands in the block fallback where the growing separators came from). The commit message shows the measured 'One\nTwo' → 'One\n\nTwo' → 'One\n\n\nTwo' rather than asserting the bug.
• ✅ The equivalence fixture now has <script> and <style>. That's the right correction: the test asserted the two entry points agree while being built out of the one thing that distinguished them, so it would have passed through dd2af3f's regression. It's load-bearing now.
• ✅ Both halves of the constants argument are written down (pmc.py:36-42), and pmid.py:357-360 says why it gates on the notice length instead of leaving two bare literals.
• ✅ scope: Tag replaces Any, and the class docstring no longer says "from HTML bytes".

I also checked the copy's safety rather than assuming it: bs4's Tag.copy_self() passes interesting_string_types (and cdata_list_attributes, preserve_whitespace_tags) explicitly to the clone, so get_text()'s string filtering behaves identically on the copy — the copy doesn't quietly change what counts as text.

Note: uv run pytest was denied approval in this environment for the tenth time, and there's no venv to read bs4 out of, so everything below is static reading. CI (Build and test, run 32577184072) was still in progress when I finished.

Nothing blocking. Three notes, all follow-ons from this commit.

---

1. extract() now copies a document it exclusively owns

html.py:52-54 builds soup locally and throws it away; nothing else ever sees it. So the copy.copy(scope) at line 89 protects nobody on that path — it's a second full pass over the tree on every non-PMC HTML fetch, which is the cost dd2af3f set out to remove. The consolidation removed it for the three PMC callers and this commit reintroduces it for everyone else.

The whole-document fallback (no <article>/<main> — most publisher HTML) is where it bites hardest: copy.copy on a BeautifulSoup object copies the entire parsed document, scripts and all, immediately before decomposing them. Worth knowing that BeautifulSoup.__copy__ has historically been implemented as re-encode and re-parse rather than a tree walk; if 4.14 still does that, the fallback path is doing literally the serialise-and-reparse the previous commit measured at ~24ms. I couldn't verify against the installed version — bs4 isn't present in this checkout — so treat that as the pessimistic case, not a claim.

Either way the fix is the ordinary one: a private _extract_scope(scope) holding the body, extract_scope = self._extract_scope(copy.copy(scope)), and extract() calling _extract_scope(...) directly on its own soup. Purity stays where it's needed (the public entry point taking a caller-owned tag) and the owned path stops paying for it.

Fix this →

2. The floor the constants comment asks for exists in only one of the two places that need it

pmc.py:41 says "Lower the notice length only with a floor kept here." But there are two gates on MAX_STUB_NOTICE_CHARS, and only pmc.py has somewhere to keep a floor: pmid.py:361,365 compares against the imported constant directly, because _MIN_PMC_FULLTEXT_CHARS is private to the provider module. So someone who follows the instruction — pins a floor in pmc.py, lowers MAX_STUB_NOTICE_CHARS — leaves PMIDSource._fetch_pmc_fulltext silently following it down, and that's the path with the HTML fallback the comment calls "the only stub defence there is".

The comment is the right analysis; it just can't be acted on as written from pmid.py. Making the floor a public constant that both import (MIN_FULLTEXT_CHARS, in extract/xml.py next to MAX_STUB_NOTICE_CHARS, or in a shared module) gives the instruction one place to be true.

3. Smaller items

• test_extract_scope_leaves_the_caller_tree_alone:625 discards the return value. It pins that the tree survives but not that extraction still works on the copy — a version of extract_scope that returned early before touching anything would pass. One assert text == "One\nTwo" alongside covers both halves; the idempotence test has the same shape (it compares calls to each other, never to an expected string, so three identical wrong answers pass).
• scope(["script", "style"]) then scope.find_all("br") (html.py:97,103) — the call shorthand and the explicit method six lines apart doing the same thing, with find_all(BLOCK_LEVEL_TAGS) below making it three. Carried from last round.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, now with three neighbouring comments explaining why not to. No mojibake, but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Fixing the equivalence fixture is the part I'd have expected to be skipped. The test wasn't failing and the purity fix didn't require touching it — but it was passing by construction, and a test that can only agree is worse than none, because it occupies the slot where a real check would go. Noticing that while fixing something else is the harder half.
• Choosing the copy over a docstring note, and defending it with a measurement against the thing it replaced (~14ms vs ~24ms) rather than a general appeal to cheapness. The argument is also correctly scoped — it says the cost is noise beside the network fetch, which is the comparison that decides it.
• The constants comment now reasons in both directions and names the specific tempting mistake (lowering the notice length for the erratum case this PR has carried as a known residual since round three). A comment that anticipates the plausible wrong edit is worth more than one restating the current value.
• The purity docstring explains the two distinct failures — editing a document the caller holds, and a second call returning different text — instead of a generic "does not modify its argument".
• Ten rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length constant. Every round's fix landed at the shared level rather than at the reported site.

Item 1 is a small perf regression on a network-bound path and item 2 is a comment that can't be followed from one of its two sites; both are optional. Nothing here blocks merge.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 1a49d7c)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 1a49d7c. This commit is a single cosmetic line — scope(["script", "style"]) → scope.find_all(["script", "style"]) (html.py:97) — closing the last of the smaller items from the previous round. Tag.__call__ is an alias for find_all, so it's behaviour-identical, and it does what the commit message says: the four selections in extract_scope (script/style, br, p, BLOCK_LEVEL_TAGS) now read the same way in the same twenty lines.

Nothing new found. Two items carry over from the last round, both optional.

Note: uv run pytest was denied approval in this environment for the eleventh time, and bs4 isn't present in this checkout to read, so everything below is static reading. CI (Build and test, run 32577449798) was still in progress when I finished.

---

1. extract() still copies a document it exclusively owns

Carried unchanged. html.py:52-54 builds soup locally and discards it; nothing else ever holds it. So copy.copy(scope) at line 89 protects nobody on that path — it's a second full pass over the tree on every non-PMC HTML fetch, which is precisely the cost dd2af3f set out to remove. The consolidation removed it for the three PMC callers; 8ad07e9 reintroduced it for everyone else.

The whole-document fallback (no <article>/<main> — most publisher HTML) is where it bites hardest: copy.copy on a BeautifulSoup object copies the entire parsed document, scripts and all, immediately before decomposing them. Worth noting BeautifulSoup.__copy__ has historically been implemented as re-encode and re-parse rather than a tree walk; the lock pins beautifulsoup4==4.14.2, which I could not read here, so treat that as the pessimistic case rather than a claim.

The ordinary fix: a private _extract_scope(scope) holding the body, extract_scope = self._extract_scope(copy.copy(scope)), and extract() calling _extract_scope(...) directly on its own soup. Purity stays where it's needed — the public entry point taking a caller-owned tag — and the owned path stops paying for it.

Fix this →

2. The floor the constants comment asks for can only be kept in one of the two places that need it

Also carried. pmc.py:41-42 says "Lower the notice length only with a floor kept here." There are two gates on MAX_STUB_NOTICE_CHARS, and only pmc.py has somewhere to keep such a floor: pmid.py:359,363 compares against the imported constant directly, because _MIN_PMC_FULLTEXT_CHARS is private to the provider module. Someone who follows the instruction — pins a floor in pmc.py, lowers MAX_STUB_NOTICE_CHARS for the short-erratum case — leaves PMIDSource._fetch_pmc_fulltext silently following it down, and that's the path whose own comment (pmid.py:357-360) calls the length check "the only stub defence there is".

The analysis is right; it just can't be acted on from pmid.py as written. A public MIN_FULLTEXT_CHARS next to MAX_STUB_NOTICE_CHARS in extract/xml.py, imported by both, gives the instruction one place to be true.

3. Smaller items

• test_extract_scope_leaves_the_caller_tree_alone:631 discards the return value, and test_extract_scope_is_idempotent:611-614 compares calls only to each other. Both pin the property they name, but neither pins that extraction still works: a version returning early before touching anything passes the first, and three identical wrong answers pass the second. One assert text == "One\nTwo" in each covers both halves.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:258-259 — _fetch_pubmed_xml still hand-decodes UTF-8, now with three neighbouring comments explaining why not to. No mojibake, but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• The commit does one thing and says so without inflating it. "Same operation, same six lines" is the whole justification and it's the correct one — a cosmetic change that claimed a benefit it didn't have would be worse than not making it.
• Consistency here is worth more than usual: extract_scope is now four find_all selections in a row, three of which the previous rounds' fixes hang off. A reader scanning for "what does this method select" should not have to know that scope(...) is the same call.
• Eleven rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length constant. Every round's fix landed at the shared level rather than at the reported site.

Nothing in this commit blocks merge, and nothing outstanding does either — item 1 is a small perf regression on a network-bound path, item 2 a comment that can't be followed from one of its two sites.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 3ceb991)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 3ceb991. Both items from the last round are closed, and closed at the level where they were wrong rather than at the site where they were reported:

• ✅ The copy is charged only where it buys something. _extract_scope holds the body, extract_scope is that plus copy.copy, and extract() enters the private form directly (html.py:57) because the soup it parsed is the soup it discards. Purity stays on the public entry point that takes a caller-owned tag; the generic HTML path stops paying for it.
• ✅ The pessimistic case was checked rather than assumed. I raised BeautifulSoup.__copy__ as possibly a re-encode-and-reparse and said so as a maybe; the commit measured it on the pinned 4.14.2 (__copy__ → __deepcopy__({}), 13.4 ms vs 16.7 ms to parse) and reports the waste as a tree walk, not a reparse. That correctly deflates my framing while keeping the change, which stands on its own without the worse number.
• ✅ MIN_FULLTEXT_CHARS gives the instruction one place to be true. It sits in extract/xml.py beside MAX_STUB_NOTICE_CHARS and both gates import it — pmc.py:61,67 and pmid.py:361,365. I checked for leftovers: _MIN_PMC_FULLTEXT_CHARS survives only in docs/superpowers/plans/, which is a historical plan document. Values are unchanged at 1000 on both paths, so this is a pure refactor.
• ✅ Both purity tests are anchored. assert first == "One\nTwo" and assert text == "One\nTwo" close the two self-referential holes. I traced both fixtures through _extract_scope: the idempotence fixture is <div>-only so it lands in the block fallback where the growing separators came from (two insert_after("\n"), get_text() → "One\nTwo\n", stripped), and the caller-tree fixture goes through the <p> branch after <br> → "\n". Both expected strings are right.

Note: uv run pytest and python3 were both denied approval in this environment for the twelfth time, so everything below is static reading and hand arithmetic, not a test run. CI (Build and test, run 32577764135) was still in progress when I finished.

One finding, plus carryovers.

---

1. The floor three commits now reason about has no test, and the one fixture that crosses it clears it by 28 characters

MIN_FULLTEXT_CHARS carries twelve lines of comment in xml.py:32-43 and is cited again in pmc.py:58-60 and pmid.py:357-359 as "the only stub defence there is" on the HTML paths. Nothing asserts it.

Grepping the suite: test_extractor_fidelity.py imports MAX_STUB_NOTICE_CHARS but never MIN_FULLTEXT_CHARS, and test_fulltext_providers.py has exactly one test that crosses the gate — test_locate_returns_text_from_xml:161, which only pins the accept direction. No test anywhere pins that a sub-floor XML body or a sub-floor HTML page is rejected. Change > to >=, or delete either gate outright, and the suite stays green.

Worse, that one fixture sits almost exactly on the boundary. Its 40 paragraphs are "Sentence {i} of the body." — 23 chars for i<10, 24 for i≥10, so 230 + 720 = 950, plus 39 × "\n\n" = 78, for 1028 characters against a floor of 1000. Twenty-eight characters of margin on the only test that exercises the constant. Shorten the sentence, drop four paragraphs, or nudge the floor up for any reason and the test fails for a reason that has nothing to do with what it's testing.

The gap matters more after this commit than before it. The comment's whole argument is that lowering MAX_STUB_NOTICE_CHARS — the tempting fix for the short-erratum case this PR has carried as a known residual since round three — drags the floor down with it. That argument is now written down in one place and enforced in none. A rejection test on each path, plus a fixture built as "x" * (MIN_FULLTEXT_CHARS + 500) rather than an incidental 1028, would make the constant load-bearing instead of documented.

Fix this →

2. Smaller items

• MIN_FULLTEXT_CHARS lives in the XML extractor module but governs the HTML gates. pmid.py now reads from ...etl.extract.xml import MIN_FULLTEXT_CHARS on the line above a check applied to _fetch_pmc_html output. The comment justifies the placement (beside the constant it's derived from, which is the right argument), but the import line says "xml" on an HTML decision. A neutral home — or just # noqa-free re-export from etl/extract/__init__.py — would let the import read like what it does. Cosmetic; the reasoning is in the right place, only the address is odd.
• _extract_scope is the real implementation, extract_scope the guarded door. Any future subclass overriding the public method won't affect extract(), and any new internal caller reaching for the private one silently opts out of purity. Fine today — one caller each — worth a word in _extract_scope's docstring saying it is for callers that own the tree, which is nearly what it says already.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:259-260 — _fetch_pubmed_xml still hand-decodes UTF-8, now with three neighbouring comments explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Reporting the measurement that weakens the reviewer's framing is the part that makes the commit message worth reading. I flagged the reparse as a pessimistic case and hedged it; the easy move was to leave the hedge standing since it argued for the change. Measuring it, finding 13.4 ms against 16.7 ms, and saying "the waste was a tree walk, not a reparse — still waste" keeps the justification proportionate to the evidence. That's the third round running where a review claim was checked rather than accepted.
• The split lands the cost exactly where the benefit is. extract() owns its soup, so it takes the unguarded door; extract_scope takes a tag it doesn't own, so it copies. Both facts are stated at their own call sites rather than in one comment that a reader has to hold in mind at the other.
• The constants move fixes a comment that could not be followed. "Lower the notice length only with a floor kept here" was sound advice with nowhere in pmid.py to act on it — the floor was private to the provider module. Moving it beside MAX_STUB_NOTICE_CHARS and importing it from both gates makes the instruction executable, and the comment now names the specific tempting wrong edit rather than warning generically.
• Both purity tests were passing and neither was failing; noticing that one satisfied an early return and the other satisfied three identical wrong answers, and anchoring them, is the kind of fix that only happens if you read the tests as adversarially as the code.
• Twelve rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, and now one length floor for all four gates. Every round's fix landed at the shared level rather than at the reported site.

Nothing here blocks merge. Item 1 is a coverage gap rather than a defect — but it's a gap under the one constant this PR has now spent three commits explaining.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 3527e9e)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 3527e9e. This commit is tests-only — 110 lines in tests/test_fulltext_providers.py, no src/ change — and it closes the one item from the last round:

• ✅ The floor has tests. Five of them, covering rejection below the floor on the PMC XML path, the PMC HTML fallback and the PMID full-text path, plus an acceptance case so the rejections can't pass by rejecting everything. I checked each fixture actually lands where its docstring says: "Too little to be an article body." carries none of STUB_NOTICE_PHRASES, so test_pmc_locate_rejects_xml_below_the_floor:207 is rejected by the floor rather than by is_stub_notice — which is what makes it a test of the constant.
• ✅ The 28-character margin was real. I recomputed the old fixture by hand: 10 paragraphs at 23 chars + 30 at 24, plus 39 × "\n\n" = 1028 against a floor of 1000. The commit's measurement is right, and _pmc_article_xml(MIN_FULLTEXT_CHARS + 500) sizes the replacement from the constant, so raising the floor moves the fixture with it.
• ✅ The mutation results check out. Deleting the PMC HTML gate (pmc.py:67) fails both test_pmc_locate_rejects_html_below_the_floor and the first half of the boundary test — the 2 failures reported. Deleting any of the other three gates also fails a test, so no gate can now be removed silently.

Note: uv run pytest was denied approval in this environment for the thirteenth time, so everything below is static reading and hand arithmetic, not a test run. CI (Build and test, run 32578211004) was still in progress when I finished.

One finding, plus carryovers.

---

1. Deletion is pinned on all four gates; exclusivity is pinned on one

test_pmc_locate_accepts_text_at_the_floor_boundary:236 is the only test that touches the boundary, and it drives _fetch_pmc_html — so it pins > on pmc.py:67 alone. The other three gates keep fixtures far from 1000: pmc.py:61 sees a 33-character body and a 1524-character one, pmid.py:361,365 see "Too short." and "x" * 1500. Change any of those three to >= and the suite stays green.

That's consistent with the commit's own report — "> became >= -> 1 failure" is exactly what one mutated gate yields — but the summary reads as though the mutation was run against the constant rather than against one of its four uses, and a reader who trusts it will believe the boundary is settled everywhere. The gap is small in consequence (a document of exactly 1000 characters is an odd corner), but the boundary test exists precisely because that corner was worth stating, and stating it in one of four places is what the last three rounds' consolidation work was against.

Cheapest close: parametrize the boundary test over the two locate gates and the two _fetch_pmc_fulltext gates, so one test says "exclusive, everywhere" instead of "exclusive, on the HTML fallback".

Fix this →

2. Smaller items

• TestPMCProvider's config fixture is now a duplicate of the module-level one (:155 vs :19 — same cache_dir, same rate_limit_delay=0.0, no email in either). The new module fixture's docstring says "classes define their own", which is true for TestUnpaywallProvider/TestOpenAlexProvider (they set email) but no longer buys anything for the PMC class. Deleting the class fixture would make the module one apply unchanged — and would remove the trap where a future edit to one PMC fixture doesn't reach the other three PMC tests.
• The PMC floor tests sit outside TestPMCProvider while test_locate_returns_text_from_xml sits inside it, so one provider's locate behaviour is now described in two places. The section banner explains the grouping-by-constant intent, which is reasonable — just noting the class is no longer the place to look for what locate does.
• The acceptance fixture feeds bytes where production feeds str — _pmc_article_xml returns .encode(), but _fetch_pmc_xml_source's docstring (pmc.py:102-108) exists specifically because Entrez hands back str. The extractor takes both and test_pmc_provider_preserves_accents_from_non_utf8_article covers the str path, so nothing is untested — the floor tests just don't exercise the shape the production path produces.
• MIN_FULLTEXT_CHARS lives in the XML extractor module but governs the HTML gates — pmid.py:24 reads from ...etl.extract.xml import MIN_FULLTEXT_CHARS above a check applied to _fetch_pmc_html output, and the test file now imports it from there too. The comment justifies the placement (beside the constant it's derived from, which is the right argument); only the address reads oddly. Carried from last round.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:259-260 — _fetch_pubmed_xml still hand-decodes UTF-8, with three neighbouring comments now explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Verifying the tests by mutation rather than by writing them and watching them pass is the difference between coverage and protection. The gap this commit closes was exactly a suite that was green whether or not the code did anything — reporting which mutations were caught, with counts, is the only form of that claim a reviewer can check, and both counts hold up when traced.
• Sizing the fixture from MIN_FULLTEXT_CHARS rather than from a paragraph count fixes a test that was correct by coincidence. The 28-character margin wasn't a failure and nothing pointed at it; noticing that a passing test would have broken on an unrelated edit is the harder half, and the measurement is stated rather than estimated.
• test_pmid_fulltext_accepts_a_body_past_the_floor's docstring — "the rejection tests above must not pass by rejecting everything" — names the specific way a set of rejection tests goes wrong. Three of the five tests here would be satisfied by a gate that rejected unconditionally; that one is what stops it.
• The section banner records why the constant needed tests (it carries the reasoning for both thresholds and had none) rather than restating what the tests do, so the next person to touch MAX_STUB_NOTICE_CHARS meets the argument where they'd meet the tests.
• Thirteen rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length floor for all four gates — and now that floor is enforced rather than merely explained.

Nothing here blocks merge. Item 1 is a narrowing of a claim rather than a defect: the gates can no longer be deleted, only their exclusivity is unpinned on three of four.
· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 9d5899b)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 9d5899b. This commit is small — a re-export plus two docstring lines — and it closes the addressing nit from two rounds ago:

• ✅ MIN_FULLTEXT_CHARS now reads from the extraction layer. pmc.py:25 and pmid.py:24 import it from etl.extract rather than etl.extract.xml, so neither file has import ... xml ... sitting above a check applied to _fetch_pmc_html output. The constant stays defined beside MAX_STUB_NOTICE_CHARS, which is the right call — the twelve lines of coupled reasoning in xml.py:32-43 only make sense next to the constant they reason about, and both gate comments still point there by name (pmc.py:59-61, pmid.py:357-360), so a reader following the import lands on the argument.
• ✅ No new import cost. I checked rather than assumed: from ...etl.extract.html import HTMLExtractor on the line above already executes the package __init__, so both modules were paying for base/html/xml/pdf before this commit. pdf.py also has no third-party top-level import (io, logging, typing, base), so the package surface is cheap. Nobody should "optimize" this back to the submodule form.
• ✅ _extract_scope's docstring names its audience. "For callers that own the tree and do not mind it being edited … opting out of that protection deliberately" — that's the thing a future internal caller needs to read before picking the shorter name, and it's now in the first line rather than implied by the leading underscore.

Note: uv run pytest was denied approval in this environment for the fourteenth time, so everything below is static reading, not a test run. CI (Build and test) was still queued when I finished.

One finding, plus carryovers.

---

1. MAX_STUB_NOTICE_CHARS was promoted alongside it, and it's the constant the argument doesn't apply to

etl/extract/__init__.py:8,15 re-exports both. But the two are in genuinely different positions, and this PR spent three commits establishing the difference:

• MIN_FULLTEXT_CHARS gates four sites across two formats (pmc.py:62,68, pmid.py:362,366) — two of them HTML. Package-level is exactly right.
• MAX_STUB_NOTICE_CHARS is used by is_stub_notice and nothing else, and xml.py:40 says why in as many words: "Only the XML path runs is_stub_notice." Promoting it to the package surface asserts the opposite of the comment.

The practical results:

• Nothing imports it from the new address. tests/test_extractor_fidelity.py:25 still takes it from ...extract.xml, correctly, because it imports is_stub_notice from there on the next line. So the export is a new public name with zero users.
• It's exported without its function. is_stub_notice and STUB_NOTICE_PHRASES are not in __all__, so a caller who takes MAX_STUB_NOTICE_CHARS from etl.extract can't reach the thing that consumes it from the same place. The constant is meaningless on its own — it isn't a threshold anyone applies directly.
• Two addresses for one constant is the shape this PR has spent thirteen rounds removing. The last three fixes were _excerpt_key, one HTML extractor, one length floor; adding a second way to spell MAX_STUB_NOTICE_CHARS runs the other way.

Dropping it from the import and __all__ leaves MIN_FULLTEXT_CHARS as the one constant the extraction package genuinely owns, which is also the cleaner statement of what the commit set out to say.

Fix this →

2. Carryover: the floor's boundary is pinned on one of its four gates

Unchanged from last round. test_pmc_locate_accepts_text_at_the_floor_boundary:236 drives _fetch_pmc_html, so it pins > on pmc.py:68 alone. The other three gates keep fixtures far from 1000 — pmc.py:62 sees a 49-character body and a MIN_FULLTEXT_CHARS + 500 one, pmid.py:362,366 see "Too short." and "x" * (MIN_FULLTEXT_CHARS + 500). Change any of those three to >= and the suite stays green. Deletion is pinned everywhere; exclusivity isn't. Parametrizing the boundary test over the four gates is the cheap close.

3. Smaller items

• __all__ ordering (extract/__init__.py:14-22) — constants first, then Extractor/ExtractorRegistry, then the extractors in a different order than the imports above them (html, xml, pdf vs HTML, XML, PDF — actually matching, but the base pair moved). Cosmetic; ruff check won't flag it since RUF022 isn't in the default set.
• TestPMCProvider's config fixture (:154-156) is now byte-identical to the module-level one (:18-21) — same cache_dir, same rate_limit_delay, neither sets email. The module fixture's docstring says "classes define their own", which still earns its keep for TestUnpaywallProvider/TestOpenAlexProvider but no longer for this class. Deleting the class fixture removes the trap where editing one PMC fixture misses the four PMC tests that use the other.
• The floor tests sit outside TestPMCProvider while test_locate_returns_text_from_xml sits inside it, so one provider's locate behaviour is described in two places. The section banner explains the grouping-by-constant intent, which is reasonable — just noting the class is no longer where to look for what locate does.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:259-260 — _fetch_pubmed_xml still hand-decodes UTF-8, with three neighbouring comments now explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Splitting where a constant is defined from where it is addressed is the right resolution of the nit. The obvious move was to relocate MIN_FULLTEXT_CHARS to a neutral module, and that would have separated it from the twelve lines of coupled reasoning that are the whole reason the two thresholds can't drift — the reasoning has to live beside MAX_STUB_NOTICE_CHARS or it stops being read. Keeping the definition and moving only the import gets both.
• The gate comments already carried "see MIN_FULLTEXT_CHARS in extract/xml.py", so changing the import address didn't orphan the pointer. That's the detail that decides whether this refactor helps or hides the argument, and it was already in place.
• _extract_scope's new first line addresses the reader who's about to make the mistake, in the place they'll be standing. A private method's underscore says "internal"; it doesn't say "this one skips the copy on purpose."
• Fourteen rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length floor for all four gates, and that floor enforced by mutation-checked tests rather than merely explained.

Nothing here blocks merge. Item 1 is a two-line subtraction from a commit that's otherwise exactly right; item 2 is a narrowing of a claim rather than a defect.

---

· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 94e9b0b)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 94e9b0b. This commit is tests-only — 55 added lines in tests/test_fulltext_providers.py, no src/ change — and it closes the one item from the last round, along with a smaller one:

• ✅ Exclusivity is pinned on all four gates. test_every_floor_gate_is_exclusive parametrizes over _pmc_xml_gate / _pmc_html_gate / _pmid_xml_gate / _pmid_html_gate, each driven at exactly the floor and one character past it. I traced each helper to its gate: pmc.py:62, pmc.py:68, pmid.py:362, pmid.py:366 — all four reached, each with the other path in the same function patched to None so the assertion can only be answering for the gate under test.
• ✅ The pmc-xml case is answered by the floor, not by is_stub_notice. "x" * 1000 is ≤ MAX_STUB_NOTICE_CHARS so is_stub_notice runs, but carries none of STUB_NOTICE_PHRASES — so it returns False and the rejection at the boundary is genuinely the > comparison. That's the detail that decides whether this test measures what it names, and it holds.
• ✅ The overclaim is corrected in the commit message rather than quietly fixed. "Its message reported > became >= -> 1 failure, which reads as though the boundary had been verified for the constant. It had not" — with the measurement for the three unpinned gates (776 passed) and then each caught in turn after parametrizing.
• ✅ TestPMCProvider's duplicate config fixture is gone, with a comment saying why the class deliberately uses the module-level one.

Note: uv run pytest was denied approval in this environment for the fifteenth time, so everything below is static reading, not a test run. CI (Build and test, run 32579108581) was still in progress when I finished.

One finding, plus carryovers.

---

1. Every floor fixture is sized from the floor, so the value the comment defends is unpinned

xml.py:36-42 makes a specific argument:

They move together, but their safe directions are opposite. Raising the notice length to catch a longer wording is safe. LOWERING it — the tempting fix for a short erratum discarded for saying "restricted" — would drag this floor down too and start admitting stubs in the band it just vacated. Lower the notice length only by pinning this to a literal first.

That last sentence is an instruction to a human that nothing enforces, and this commit is the one that makes the gap visible — because it completes the relative half of the coverage. Every fixture in the suite is now expressed in terms of the constant: "x" * MIN_FULLTEXT_CHARS, MIN_FULLTEXT_CHARS + 1, MIN_FULLTEXT_CHARS + 500, and MAX_STUB_NOTICE_CHARS in test_extractor_fidelity.py:156. Sizing from the constant is right — it's what fixed the 28-character margin two commits ago — but it means the absolute value has no test at all.

Concretely: someone takes the erratum case this PR has carried as a known residual since round three, sets MAX_STUB_NOTICE_CHARS = 400, and doesn't notice the derived line below it. MIN_FULLTEXT_CHARS follows to 400. All four gates now admit 401–1000-character documents on the HTML paths, where the comment says this floor is "the sole defence" — the exact band it just vacated, exactly as predicted — and all 776 tests stay green, because each one re-derives its fixture size from the new value.

The enforceable version of the paragraph is one assertion:

def test_the_floor_does_not_follow_the_notice_length_below_1000():
    """xml.py's comment says to pin this to a literal before lowering
    MAX_STUB_NOTICE_CHARS. Nothing else in the suite would notice if it moved:
    every fixture is sized from the constant, so they all move with it."""
    assert MIN_FULLTEXT_CHARS >= 1000

Cheap, and it turns the one piece of reasoning this PR has spent four commits writing down into something that fails rather than something that has to be read.

Fix this →

2. Carryover: MAX_STUB_NOTICE_CHARS is re-exported from the package with no users

Unchanged — this commit touched no src/. etl/extract/__init__.py:8,15 exports it, but nothing imports it from there: test_extractor_fidelity.py:25 correctly takes it from ...extract.xml, beside the is_stub_notice it's meaningless without — and is_stub_notice/STUB_NOTICE_PHRASES aren't in __all__, so the package address can't be used for anything anyway. MIN_FULLTEXT_CHARS is the cross-format one (four gates, two of them HTML) and belongs there; MAX_STUB_NOTICE_CHARS is XML-only, as xml.py:40 states. Two spellings for one constant is the shape the last four rounds have been removing.

3. Smaller items

• The module config fixture's docstring (test_fulltext_providers.py:20) still says "classes define their own". That's true for TestUnpaywallProvider and TestOpenAlexProvider (both set email), but TestPMCProvider now deliberately relies on this fixture — the parenthetical is the reason the duplicate existed, and it's the line most likely to prompt someone to re-add it.
• test_pmc_locate_rejects_html_below_the_floor is now subsumed by the parametrized test in mutation terms: deleting pmc.py:68 fails both. Its XML sibling still earns its keep (its fixture demonstrates rejection by the floor on a body far below it, where is_stub_notice could plausibly be suspected instead), and test_pmid_fulltext_rejects_responses_below_the_floor uniquely pins content_type == "pmc_restricted". Just noting one of the five is now redundant rather than all four being load-bearing.
• The floor tests sit outside TestPMCProvider while test_locate_returns_text_from_xml sits inside it, so one provider's locate behaviour is described in two places. The section banner explains the grouping-by-constant intent; the class is simply no longer where to look for what locate does.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:259-260 — _fetch_pubmed_xml still hand-decodes UTF-8, with three neighbouring comments now explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Correcting a claim in a previous commit's message is unusual and worth naming. 3527e9e passed review; the mutation numbers it reported were real; nothing was failing. Going back, finding that "1 failure" answered for one of four uses rather than for the constant, and saying so in the commit that fixes it, is the same discipline the last several rounds applied to code — this time applied to the project's own record of itself.
• Parametrizing over drive functions rather than duplicating the test four times is the right shape here: the four gates differ in which object and which method they hang off, so a table of (id, driver) keeps the assertion — "the floor itself is not enough, one past it is" — written exactly once. The ids= labels are what make a failure say which gate broke.
• Each helper patches the sibling path to None. Without that, _pmc_xml_gate's rejection could be satisfied by the HTML fallback picking up the slack, and the test would pass while measuring the wrong gate. Easy to omit, and it's there in all four.
• The duplicate config fixture was deleted with a comment in its place explaining why the class now reaches outward. Deleting it silently would have invited the next person to re-add it the moment they wanted a PMC-specific setting.
• Fifteen rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length floor for all four gates — and that floor now enforced against both deletion and off-by-one on every use.

Nothing here blocks merge. Item 1 is the last gap under this constant: relative behaviour is fully pinned, the absolute value isn't, and it's the value the comment spends seven lines defending.

---

· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at 1376ffa)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at 1376ffa. This commit is a two-line subtraction from etl/extract/__init__.py, and it closes item 1 from the last round exactly as scoped:

• ✅ MAX_STUB_NOTICE_CHARS is off the package surface. I checked the claim rather than taking it: grep across src/, tests/ and docs/ finds it in three places only — the definition (xml.py:30), the derivation on the line below (xml.py:43), and its one use inside is_stub_notice (xml.py:74) — plus tests/test_extractor_fidelity.py:25, which imports it from ...extract.xml beside is_stub_notice and XMLExtractor. Nothing took it from the package address, so the removal can't break a caller.
• ✅ MIN_FULLTEXT_CHARS keeps the package spelling, and still earns it. Four gates, two of them HTML: pmc.py:62,68 and pmid.py:362,366, both importing from etl.extract. That's the asymmetry the commit message rests on and it holds.
• ✅ The commit message argues from the code's own comment. xml.py:40 says "Only the XML path runs is_stub_notice"; exporting the constant that gates it as though it were cross-format asserted the opposite. Reverting a two-line addition from three commits ago because it contradicted a comment written in the same PR is the right size of correction, and it's stated without inflating it into a bug fix.

Note: uv run pytest was denied approval in this environment for the sixteenth time, so everything below is static reading, not a test run. CI (Build and test) was still queued when I finished.

One finding, unchanged from last round, plus carryovers.

---

1. Carryover: the floor's absolute value is still unpinned, and this commit narrows the blast radius without closing it

xml.py:36-42 spends seven lines on one instruction:

Raising the notice length to catch a longer wording is safe. LOWERING it — the tempting fix for a short erratum discarded for saying "restricted" — would drag this floor down too and start admitting stubs in the band it just vacated. Lower the notice length only by pinning this to a literal first.

Nothing enforces that. Every fixture in the suite is sized from the constant — "x" * MIN_FULLTEXT_CHARS, MIN_FULLTEXT_CHARS + 1 (test_fulltext_providers.py:293-294), MIN_FULLTEXT_CHARS + 500 (:176,317), and "x" * MAX_STUB_NOTICE_CHARS (test_extractor_fidelity.py:156). Sizing from the constant is right — it's what fixed the 28-character margin four commits ago — but it means every test re-derives its threshold from whatever the value happens to be.

Set MAX_STUB_NOTICE_CHARS = 400 for the erratum case this PR has carried as a known residual since round three, and MIN_FULLTEXT_CHARS follows via xml.py:43. All four gates start admitting 401–1000-character documents on the paths where the comment calls this floor "the sole defence", and the whole suite stays green.

This commit makes that slightly more likely to happen, not less: with MAX_STUB_NOTICE_CHARS no longer on the package surface, the only place it's visible outside xml.py is one test import — so an editor working on the erratum case has even less chance of meeting a consumer that would object.

One assertion turns the paragraph into something that fails:

def test_the_floor_does_not_follow_the_notice_length_below_1000():
    """xml.py's comment says to pin this to a literal before lowering
    MAX_STUB_NOTICE_CHARS. Nothing else in the suite would notice if it moved:
    every fixture is sized from the constant, so they all move with it."""
    assert MIN_FULLTEXT_CHARS >= 1000

Fix this →

2. Smaller items

• __all__ vs import order (extract/__init__.py:7-20) — pdf is imported after xml while __all__ lists HTML, XML, PDF, and the constant leads the list ahead of Extractor/ExtractorRegistry. Pre-existing and cosmetic; ruff check won't flag it (RUF022 isn't in the default set).
• The module config fixture's docstring (test_fulltext_providers.py:20) still says "classes define their own". True for TestUnpaywallProvider/TestOpenAlexProvider (both set email), but TestPMCProvider now deliberately relies on this fixture — and that parenthetical is the line most likely to prompt someone to re-add the duplicate 94e9b0b deleted.
• test_pmc_locate_rejects_html_below_the_floor is subsumed by test_every_floor_gate_is_exclusive in mutation terms — deleting pmc.py:68 fails both. Its XML sibling still earns its keep (its fixture shows rejection by the floor where is_stub_notice could plausibly be suspected), and the PMID rejection test uniquely pins content_type == "pmc_restricted".
• The floor tests sit outside TestPMCProvider while test_locate_returns_text_from_xml sits inside it, so one provider's locate behaviour is described in two places. The section banner explains the grouping-by-constant intent; the class is simply no longer where to look for what locate does.
• Only one excerpt key is validated by repair data — _excerpt_key returns a single key, so an item with both supporting_text and snippet non-blank has the second silently unchecked, while the plugin iterates _find_excerpt_fields and checks each (reference_validation_plugin.py:187). Pre-existing; the named helper makes it look settled.
• pmid.py:259-260 — _fetch_pubmed_xml still hand-decodes UTF-8, with three neighbouring comments now explaining why not to. No mojibake (bs4 fixes up the declaration for str), but non-UTF-8 bytes raise UnicodeDecodeError and take the MeSH/publication-type metadata with them. Out of the stated scope; the last inconsistent one.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Removing something added three commits earlier, in its own commit, with the reason stated as a contradiction between two lines of the same file — rather than quietly folding it into the next change — keeps the history readable as a record of decisions rather than of edits.
• The asymmetry the commit rests on is the correct one and was worth spelling out: a constant that gates four sites across two formats belongs on the package surface; a constant used by exactly one function on one path does not. That distinction is what made MIN_FULLTEXT_CHARS's move right in 9d5899b and this one wrong, and both halves are now on the record.
• Checking that the export had no users before removing it — and noting that is_stub_notice was never exported either, so the package spelling was unusable regardless — is the difference between a safe subtraction and a hopeful one.
• Sixteen rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard keeping the primitive honest for new callers, _excerpt_key as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length floor for all four gates enforced against both deletion and off-by-one — and now one address per constant.

Nothing here blocks merge. Item 1 is the last open gap: the floor's relative behaviour is fully pinned, its absolute value isn't, and that value is what the seven-line comment above it exists to defend.

---

· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at c14d7bd)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at c14d7bd. This commit closes the last open item from round one — the one that decided whether users see the benefit of the PR at all — plus the three carryovers:

• ✅ Cache invalidation. Entries carry extractor_version in frontmatter (reference_fetcher.py:607); the validation read path treats a below-current stamp as absent (:746) so the reference is re-fetched and rewritten. Lazy and per-reference, nothing deleted, and a newer stamp is left alone so an older tool doesn't re-fetch on every run. Both exemptions are reasoned rather than incidental: iter_cached_references (export/enrichment walks the cache as a record of what was fetched) and the legacy .txt format.
• ✅ _fetch_pubmed_xml no longer hand-decodes. The last of the three sites, with the reason stated next to it and a test driving a non-UTF-8 record through Entrez.efetch.
• ✅ _excerpt_key → _excerpt_keys. Both layers now check every excerpt field, and the reported path carries the key (evidence[0].snippet), matching excerpt_path in the plugin (reference_validation_plugin.py:189) — which is also what keeps a two-excerpt repair landing in the right field. test_repair_writes_back_to_each_excerpt_key_independently pins it.
• ✅ The floor's absolute value is pinned. assert MIN_FULLTEXT_CHARS >= 1000, with a docstring saying why no other test would notice it moving.
• ✅ Troubleshooting doc rewritten from "delete these files" to "this is deliberate, here's the one-off", and points at cache reference --force.

Note: uv run pytest was denied approval in this environment for the seventeenth time, so everything below is static reading, not a test run.

Four things, the first of which is a behaviour regression on a workflow this tool documents.

---

1. A stale entry whose source no longer answers is discarded with nothing to fall back on

_load_from_disk returns None for a stale entry, so fetch() goes to the source (reference_fetcher.py:143-168). If that fetch yields nothing — offline, NCBI outage, a file: target since moved, a record withdrawn — content is None, _save_by_access never runs, and fetch() returns None. The good-enough copy is sitting on disk and is now unreachable: not deleted, but ignored on this run and every run after, since nothing rewrites it.

That matters more here than it would in most caches, because this one is explicitly a pre-population store. cache reference's own help says "Cache a reference for offline use … Downloads and caches the full text of a reference for offline validation" (cli/cache.py:123-126). Every entry a user pre-populated before this PR is unstamped. So the first offline run after upgrading reports every reference as not found — not "text was stale", not "couldn't refresh" — and the second run does the same.

The invalidation is right; the missing half is the fallback. Serving the stale entry when the refresh yields nothing (with a warning naming it as stale) keeps the whole benefit — every reachable reference still refreshes on first use — and costs only the case where the alternative is no reference at all. Neither behaviour is currently pinned by a test, so whichever you want is worth writing down.

Fix this →

2. The line that explains the re-fetch is invisible unless the user passes --verbose

reference_fetcher.py:747 logs at info, and setup_logging only calls basicConfig when verbose is set (cli/shared.py:57-66) — so on a default run the root logger stays at WARNING and nothing is printed.

docs/troubleshooting.md:328 presents those exact lines as the symptom to look for:

Ignoring cache entry for PMID:30598549 written by an older extractor;
it will be re-fetched and rewritten

A user who arrives at that page because their first post-upgrade run was slow will not find that string in their output, and will reasonably conclude this isn't their problem. Combined with item 1, the offline case has no visible explanation at all. Either log this one-off at warning (it is a one-time event per reference, not per-fetch chatter), or say in the doc that it needs -v.

3. The legacy exemption is implemented by sniffing content, not by which file was read

The comment promising it sits on the .txt branch at reference_fetcher.py:732-738 — but nothing there records that the file was legacy. The exemption actually falls out of content_text.startswith("---") at :745. A .txt entry whose first line happens to be --- routes into the Markdown branch, fails the stamp check, and is discarded — which is precisely the "discard someone's data to fix a problem they do not have" the comment forbids. Low likelihood, since the legacy writer emitted key: value lines; carrying a boolean from the branch would make the comment true by construction rather than by the format's habits.

4. Nothing beside the extractors points at the constant that has to be bumped

EXTRACTOR_CACHE_VERSION's docstring (reference_fetcher.py:47-57) says it is "bumped whenever an extraction change means previously cached text is wrong rather than merely older" — good definition, but it's addressed to someone editing etl/extract/xml.py or html.py, two packages away, and neither file mentions it. The next extraction fix in this codebase will be made by someone reading xml.py.

This PR set the pattern for exactly this: the MIN_FULLTEXT_CHARS comment names the tempting wrong edit where the tempted person will be standing. A one-line pointer in extract/__init__.py (or beside is_stub_notice and extract_scope, the two functions whose bugs motivated version 1) would do the same job here.

5. Smaller items

• Every reference type is invalidated, including ones the bugs never touched — Crossref abstracts, file:, GEO, ClinicalTrials. Defensible, since the stub bug could also have downgraded a real article to abstract_only, which a content_type filter would miss. But the doc calls the refresh "a one-off per reference" without saying the scope is the whole cache; a curator with thousands of entries re-fetches all of them under rate_limit_delay on the first run. Worth a sentence.
• _is_stale_cache_entry splits on "---" with maxsplit=2 (:801), so a frontmatter value containing --- would truncate the searched region and make a current entry read as stale. Safe today only because the stamp is written second, immediately after reference_id (:606-607) — an ordering dependency between writer and reader that isn't stated at either end.
• Repair paths changed shape — evidence[0] → evidence[0].supporting_text. That's the fix (it's what stops a repair landing in the wrong field, and it matches the plugin), but anything downstream parsing repair-report paths sees a different string. Release-note line.
• An item carrying the same quote under both keys is now fetched, validated and reported twice. Harmless with a warm cache; just no longer deduplicated.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise.

---

What's good

• Lazy per-reference refresh over a sweep or a delete is the right shape: it costs one fetch at the moment the entry is actually needed, and a user who never touches an affected paper never pays. "Nothing is deleted" is the property that makes this safe to ship without a migration step.
• Treating a newer stamp as fine is the half that's easy to omit. Without it, anyone running two versions against one shared cache — a CI job on the release and a developer on the branch — would re-fetch every reference on every run, and would blame the network.
• Both exemptions are argued from what the code is for rather than from convenience: iter_cached_references backs export, where dropping an entry loses it instead of refreshing it; the .txt format predates the extractors that had the bugs, so re-fetching it would trade someone's data for a fix they don't need. Either could have been waved through as "out of scope".
• Seven test fixtures were stamped rather than the check being weakened for tests — which also demonstrates, in-repo, exactly what an external user with a committed cache faces.
• The test set covers all four stamp states (absent, older, current, newer) plus the exemption and the end-to-end re-fetch-and-restamp, and test_stale_entry_is_refetched_and_restamped asserts the file was rewritten, not just that the fetch returned something.
• _excerpt_keys is the shared-level fix again: extraction and write-back still can't diverge, because the key now travels in the path both sides key on.
• Seventeen rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard, _excerpt_keys as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length floor enforced against deletion, off-by-one and its absolute value — and now the cache itself knows which extractor wrote it.

Item 1 is the one I'd want before merge: the invalidation is correct, but with no fallback it converts a stale-text problem into a no-reference problem for exactly the offline workflow this tool tells users to pre-populate a cache for. Item 2 is what makes that case undiagnosable.

---

· Branch: claude/empty-snippets-pgwe8z

[claude]

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

---

PR Review: Reject empty excerpts and fix content extraction bugs

• Gather context (read diff at dd9940d)
• Review source changes
• Review test coverage
• Check whether earlier review items were addressed
• Post review feedback

---

Re-reviewed at dd9940d. The blocking item from the last round is fixed, and the four smaller ones with it:

• ✅ Stale fallback. _stale_fallback serves the older entry when the source yields nothing, warns that its text may still carry the bugs, deliberately does not re-save it, and force_refresh opts out. I checked the reachability rather than assuming it: PMIDSource.fetch wraps its Entrez call in try/except and returns None (pmid.py:114-118), so a network failure lands on the fallback rather than propagating past it. Both the "source returns nothing" and "no source handles this ID" paths route through it.
• ✅ The warning is visible by default, and the doc's claim about it is true. setup_logging only calls basicConfig under -v (cli/shared.py:64-66), and nothing in src/ installs a NullHandler — so logging.lastResort (level WARNING) emits this to stderr on a plain run while the INFO re-fetch line stays hidden. docs/troubleshooting.md:376 says exactly that, including that the INFO line needs -v.
• ✅ Frontmatter ends at a line that is ---. _FRONTMATTER_DELIMITER is line-anchored and maxsplit=2 keeps body rules harmless. I traced the title case by hand: - is not in _quote_yaml_value's special_chars, so title: Before --- after is written unquoted and the old split("---", 2) really did truncate the block there and drop journal — test_horizontal_rule_in_a_title_does_not_truncate_the_frontmatter pins the field that was being lost, not just the split.
• ✅ The legacy exemption follows the file that was opened, with test_legacy_text_entry_is_exempt_even_if_it_starts_with_a_rule covering the sniffing case it replaces.
• ✅ EXTRACTOR_CACHE_VERSION is pointed at from extract/__init__.py, xml.py and html.py — where someone changing extraction is actually standing.

Note: uv run pytest was denied approval in this environment for the eighteenth time, so everything below is static reading, not a test run.

Two findings, plus carryovers.

---

1. cache reference now reports success, and exits 0, for a reference it did not cache

cli/cache.py:146-158 branches on truthiness alone:

reference = fetcher.fetch(reference_id, force_refresh=force)
if reference:
    typer.echo(f"Successfully cached {reference_id}")
    ...
    raise typer.Exit(0)

With the fallback in place, fetch() returns the stale entry — so cache reference PMID:X offline against an unstamped entry prints Fetching… / Successfully cached PMID:X and exits 0, having downloaded nothing and written nothing. The entry on disk is still the one the fixed extractors are meant to replace, and _stale_fallback deliberately left it that way. Before this commit that same invocation printed Failed to fetch and exited 1.

This is the one command whose entire job is the thing that didn't happen — its own help says "Downloads and caches the full text of a reference for offline validation. Useful for pre-populating the cache" (cache.py:123-126). A pre-population loop gated on exit status now goes green across a whole outage. The Could not re-fetch… warning does reach stderr, so a human watching sees the contradiction; a script does not.

The fallback is right for validate — that's the case it was added for. cache reference wants the other answer, and the two can't currently be told apart from the return value. Either give the caller a way to know the result came from the fallback, or have reference_command treat a stale-served result as failure. Worth a CLI test either way; nothing currently pins this command's behaviour when the source is unreachable.

Fix this →

2. A permanently-offline cache pays a failed fetch per reference on every run, not just the first

"The entry is deliberately not re-saved, so it stays stale and the next run that can reach the source still refreshes it" is the right call for correctness — but it means the refresh attempt is never retired for a user who never reaches the source. Each stale reference goes: memory miss → disk load returns None → full source fetch → network timeout → fallback. The in-process memo (_stale_fallback line 209) covers repeats of the same ID, so a 1,000-reference dataset offline costs 1,000 timeouts, and costs them again tomorrow.

Before this PR an offline run against a warm cache did no network work at all. docs/troubleshooting.md:362-376 tells the offline user their text is served, which is the important half, but not that every run re-attempts every reference — and that's the part they'll notice first, because it looks like a hang rather than a fallback.

Not a correctness problem and not obviously worth code, but the docs currently frame the refresh as "one-off per reference" (:349-353), which is true only for someone who gets through. A sentence in the offline section, or a per-run circuit breaker after N consecutive source failures, would close the gap between what the page promises and what an offline run does.

3. Smaller items

• The writer doesn't guarantee the invariant the reader now depends on. _split_frontmatter is correct given one line per value, but _quote_yaml_value (reference_fetcher.py:604) has no \n in special_chars and wouldn't escape it if it did — so a title carrying a newline is written across physical lines. If the continuation happens to be ---, the frontmatter ends early: YAML still parses (the stamp is written second, so staleness is unaffected), the title comes back truncated, and the dropped lines are prepended to the body. Exactly the failure this commit fixed, one layer up. Unlikely for Entrez titles; cheap to close by quoting-or-rejecting a value containing a newline.
• _stale_fallback skips _maybe_retry_full_text, which the ordinary cached path applies (:154). Defensible — the retry is a network call and we just established the network isn't there — just not stated.
• CRLF frontmatter isn't split. ^---[ \t]*$ doesn't match ---\r, so a hand-edited cache file saved on Windows reads as stale forever. The tool always writes \n, so this only bites hand-maintained entries; adding \r? costs nothing.
• Private cache entries are still invisible to _load_from_disk — it reads get_cache_path (public dir) only, while _save_by_access routes non-open content to the private dir. Pre-existing and out of scope; noting it because the fallback inherits the same blind spot.
• Still unmentioned in the PR description: the plugin emits an ERROR for any present-but-empty-string excerpt on any instance, so data using supporting_text: "" as a placeholder will light up (None is correctly left alone); and validate() rejects blank excerpts ahead of skip_prefixes/trusted_low_similarity, so a blank excerpt on a skipped or trusted reference changes from INFO/valid to ERROR. Both are the point of the PR — they want a release-note line so first-run output isn't a surprise. (Since drafting: the description now carries both, plus the cache and exit-code notes. Consider this closed.)

---

What's good

• The fallback is scoped by what the caller asked for rather than by a config knob: an implicit read falls back, force_refresh doesn't. "An explicit refresh that failed should report failure" is the right line to draw, and test_force_refresh_does_not_fall_back_to_the_stale_entry pins the side that's easy to lose.
• Not re-saving the served entry is the detail that keeps the invalidation intact. Writing it back would have converted "temporarily unreachable" into "permanently accepted", which is how a fallback usually rots — and the docstring says so where someone would otherwise add the save.
• test_missing_entry_still_returns_none_when_the_source_fails is the guard that stops the fallback from being a general excuse: no cache entry still means no result. Three of the five new tests would pass on a fallback that invented content; that one wouldn't.
• The frontmatter fix was found by following the reported concern past its reported site. The review raised maxsplit=2 as a hazard for _is_stale_cache_entry; the same split was also in _load_markdown_format, silently dropping fields, and the commit fixes both behind one helper rather than patching the one that was named.
• Both ----in-a-value tests assert the consequence rather than the mechanism — the id round-trips, journal survives — so they'd catch a re-regression in a different implementation.
• Eighteen rounds in, the layering has held without a rewrite: check_excerpt_content() as the single source of truth for excerpt substance, INSUFFICIENT_EXCERPT with its own report section and exit-code contribution, _substring_match's empty-parts guard, _excerpt_keys as one resolver for read and write-back, one HTML text extractor behind one entry point for all three PMC paths, one length floor enforced against deletion, off-by-one and its absolute value, one frontmatter splitter for both readers — and the cache now knows which extractor wrote it, with a defined answer for when it can't get a better one.

Item 1 is the one I'd fix before merge: it's small, it's a behaviour change this commit introduces, and it turns the one command dedicated to filling the cache into one that claims success without filling it.

---

· Branch: claude/empty-snippets-pgwe8z