Preserve JATS tables and refresh legacy XML caches

[cmungall]

Change

JATS tables were missing from cached full text, preventing validation of findings stated only in a table. Append deterministic pipe-delimited rows with labels/captions and footnotes from the whole main article, including floats-only articles. Preserve body prose and source-managed abstracts; prevent duplicated captions/cells and exclude reviewer/reply tables in sub-article/response elements.

Keep the first 200 source rows per table (headers included), with an explicit truncation notice. Annotate spans on their source cells rather than propagate values; these are source rows, not reconstructed grids. Nested tables own their rows and get a generic heading when they have no separate wrapper.

Add xml_extraction_version: 1 for targeted legacy XML refresh, preserving current PDF/HTML and #80 HTML semantics. Only fresh source/provider acquisitions certify XML; metadata rewrites preserve missing, old, and future stamps. Offline legacy XML remains stale with the existing warning.

Validation

• Tests written first: extraction/migration failures reproduced, followed by failing-then-passing regressions for review findings.
• Real PMC5593426 / PMID28530713 Table 1 excerpt, identifiers, permissions, and provenance. Full manuscript removed from branch history; original table text/attributes retained.
• Real loopback HTTP provider acquisition, fixture-backed warm source refresh, directly validated immunoglobulin rows, and stale/version round trips.
• just test: 963 passed, mypy/ruff passed before final review fixes.
• Final review fixes: 192 affected tests (33 table/migration tests), mypy/ruff, and just doctest (202 passed) pass. Final commit 314bc44f308a115dd3f9825e6c27d73b047c44e4 passes Python 3.10–3.13 CI and the automatic review check; the final review confirms prior blocking findings are fixed.

Review disposition and limits

Fixed footnote loss, image/empty-table caption loss, hidden comment/PI text, reviewer-response body/table attribution, nested heading reuse, docstring/version guidance, and reduced the fixture to a relevant excerpt. Existing attribution/alternative body paragraphs survive unless already emitted by the table renderer. Strict XML integer stamps deliberately exclude YAML booleans; adjacent HTML semantics remain unchanged.

Bare tables without JATS wrappers, image tables, and alternate table encodings remain outside scope. The cap is per table, not per document. Provider retry/downgrade redesign and shared version-helper refactoring are deferred: existing successful abstract-only refresh can replace older full text, now documented. Spanned cells require consulting the original table to interpret column relationships. Existing superscript/subscript flattening remains documented and tested; numeric exponent interpretation is not redesigned.

Final non-blocking review disposition

Keep the requested prose-only stub check: counting appended table text could hide a restricted placeholder. A short legitimate body containing a stub phrase can still be rejected; changing that heuristic requires a separate fidelity decision. Keep document-wide main-article table discovery (including front/back) as requested, rather than narrowing it to body/floats.

Retain physical table blocks and notes in source order, without inferred associations for sibling tables or nested notes. An over-cap table with empty initial rows can yield only a truncation notice. Additional boolean-stamp tests, document-level limits, helper refactors and minor test/docstring cleanup are deferred hardening; none were identified as blocking in the final review. Existing HTML stamp semantics and provider downgrade behavior remain outside this issue.

Closes #68

[claude]

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

---

Review: Preserve JATS tables and refresh legacy XML caches

• Gather context (diff, changed files)
• Review etl/extract/xml.py table extraction
• Review etl/reference_fetcher.py version migration
• Review tests & fixtures
• Post review feedback

---

Overall this is a well-shaped change. The decisions I most agree with: annotating spans on their source cell rather than propagating values (a reconstructed grid would manufacture patient values that aren't in the paper — exactly the failure mode a supporting-text validator must not have), checking is_stub_notice before appending tables, escaping \ before |, and introducing a format-scoped XML_EXTRACTION_CACHE_VERSION instead of bumping EXTRACTOR_CACHE_VERSION and invalidating every PDF/HTML entry. The real-article fixture with a directly-quoted row is the right kind of test.

A few things I'd want addressed, the first one especially.

Note on verification: I could not execute uv/pytest in this environment (bash approval restrictions), so everything below is from reading the code. All findings are static; I have not reproduced them at runtime.

1. Table footnotes are now dropped entirely (net text loss vs. main)

xml.py:191 adds not p.find_parent("table-wrap") to the body-paragraph filter. That correctly stops captions from appearing twice — but <table-wrap-foot> / <fn> paragraphs live under table-wrap too, and _tables_as_text never emits them. On main, for a table-wrap sitting inline in <body> (the common PMC layout), those footnote paragraphs were in the cached text. After this PR they are in neither the prose nor the table section.

This matters precisely because of what the PR adds. The fixture's own Table 1 footnote is:

IvIg, intravenous immunoglobulin; EBV, Epstein-Barr virus; RhF, rheumatoid factor, dsDNA, double-stranded DNA; ANCA, anti-neutrophil cytoplasmic antibody; ...

and tests/test_jats_tables.py:32 asserts the row IgG | Low | Low | High* — whose * is defined only in that footnote. (This fixture's table-wrap is in floats-group, so it loses nothing relative to main; body-embedded tables do.) The abbreviation key and significance markers are the text that makes a table row interpretable, and since this ships behind a cache-version bump, legacy entries get rewritten with strictly less text than they had.

docs/troubleshooting.md states "Table footnotes outside cells are not appended by this extraction pass" — but it reads as a scope limit, not as "content that used to be cached is now removed." I'd emit the footnote text after the rows rather than document the loss.

Fix this →

2. A <table> with no <table-wrap> ancestor is silently skipped

xml.py:117-119 does continue when find_parent("table-wrap") is None. The wrap is only needed for the label/caption, and heading already has a "Table" fallback at line 128 — so requiring the wrap costs table content for no gain. This extractor is registered for xml generally and _materialize routes any sniffed-XML download to it (reference_fetcher.py:465-473), so publisher XML that isn't JATS loses every table, silently. Dropping the continue and letting wrap = None fall through to the "Table" heading would be a couple of lines.

Fix this →

3. Table search spans <sub-article>; prose does not

soup.find_all("table") (xml.py:115) walks the whole document, while prose comes from soup.find("body") — the first body, i.e. the main article's. For PMC records carrying <sub-article article-type="reply"> or peer-review material (eLife and others), the sub-article's prose is correctly excluded but its tables get appended to the main article's text with no marker. A finding "validated" against such a row would be sourced from a reviewer response, not the paper. Worth scoping the search to the main article (exclude any table with a sub-article/response ancestor), or at minimum labelling it.

Fix this →

4. XMLExtractor docstring is now inaccurate

xml.py:162 still says "Returns None when there is no body content" — but the whole point of handling floats-group is that a body-less, table-bearing article now returns text. tests/test_jats_tables.py:43 parametrizes exactly that case. The sentence should say it returns None only when neither body prose nor tables yield anything (plus the stub case).

Relatedly, etl/extract/__init__.py:3-9 still instructs readers to bump EXTRACTOR_CACHE_VERSION, while xml.py:4-6 now forwards them there for "cache versions" plural. The new format-scoped stamp should be described where the docstring sends people.

5. type(x) is int vs isinstance(x, int), and three copies of the stamping rule

reference_fetcher.py:727 and :1037 use type(xml_version) is int; the HTML stamp two lines above each uses isinstance(...). I assume the difference is deliberate — isinstance(True, int) is True, so a YAML xml_extraction_version: yes would be accepted and written back as True on the HTML path but rejected as stale on the XML path. That's the better behaviour, but nothing says so, and the next reader is likely to "fix" the inconsistency in the wrong direction. A one-line comment, or a shared _version_stamp(metadata, key) helper, would lock in the intent.

The same stamping rule now also appears three times (:190, :414, :727) with a fourth site reading it back at :1037. A {content_type: (metadata_key, version)} mapping would keep the next format from adding four more branches.

6. Migration risk worth stating explicitly

The bump invalidates every cached full_text_xml entry. On refresh, if the source still resolves but full text is no longer obtainable (PMC 404, stub notice, provider returning nothing without raising), fetch() saves the new abstract-only content at :201-202 — overwriting cached full text with an abstract, and _enrich_with_full_text sets full_text_attempted = True when nothing raised, making that downgrade sticky. _stale_fallback doesn't help, because the source did return something.

This is pre-existing machinery — #80's HTML bump has the same shape — but the blast radius here is every XML entry in a user's cache at once. A guard against downgrading full_text_* to abstract_only on refresh would make version bumps routine rather than risky. At minimum I'd note it in the troubleshooting section.

7. Test coverage

Strong where it counts: the real unmodified fixture, the row asserted end-to-end through SupportingTextValidator, the 200/201 boundary, and test_stub_is_checked_before_tables (that one is a genuinely good adversarial case). Gaps I'd fill:

• Backslash escaping — docs promise it, _table_text implements it, only | is tested (test_structural_tables:50).
• rowspan="1" — the != "1" branch at xml.py:143 is untested; a regression there would annotate every ordinary cell.
• <table> without a wrap and table footnotes — currently undocumented-by-test behaviours (see 1 and 2).
• No total-document cap. 200 rows is per table; a floats-only article with many tables can still add a lot of text with nothing bounding the whole. Probably fine in practice, but nothing tests or documents an upper bound.

One nit: MAX_TABLE_ROWS (xml.py:87) is a bare assignment in a module where every other constant carries a #: block explaining why that value and which direction is safe to move it. Given the cap silently truncates evidence, it deserves the same treatment.

---

None of the above is a correctness bug in the row rendering itself — the escaping, nesting, and span handling all look right to me, and the nested-table ownership test is convincing. Item 1 is the one I'd treat as blocking, since it removes text that was previously cached.
· branch cmungall/p2-68-jats-tables

[claude]

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

---

Review: Preserve JATS tables and refresh legacy XML caches

• Gather context (diff, changed files)
• Review etl/extract/xml.py table extraction
• Review etl/reference_fetcher.py version migration
• Review tests & fixtures
• Post review feedback

Solid piece of work overall. The version stamping is scoped exactly right — only fresh source/provider acquisitions certify, metadata-only round trips preserve missing/old/future stamps, and PDF/HTML entries are untouched. Output is deterministic, escaping is ordered correctly (\ before |), spans are annotated rather than invented, and the doctests + real-fixture tests are far better than mocks. Docs in troubleshooting.md are unusually thorough.

Findings below, most significant first.

---

1. <table-wrap-foot> prose is now silently dropped (content regression)

src/linkml_reference_validator/etl/extract/xml.py:192-196 excludes every <p> with a table-wrap ancestor:

if not p.find_parent("table-wrap") and p.get_text().strip()

That correctly de-duplicates <caption> and cell paragraphs, since both are re-emitted by _tables_as_text. But <table-wrap-foot> footnote paragraphs are in neither place: they're filtered out of prose and _tables_as_text only renders label, caption and rows. Before this PR that text was in the cached body, so the version bump will rewrite every XML entry with strictly less text than it had.

This bites hardest on exactly the content the PR is about. The PR's own example row is IgG | Low | Low | High* — the asterisk is now quotable, but the footnote defining it is not, as is the usual IvIg, intravenous immunoglobulin expansion line. docs/troubleshooting.md says footnotes "are not appended by this extraction pass", which reads as "not newly added" rather than "removed from where they used to be".

Suggest either narrowing the filter to caption/table descendants, or appending table-wrap-foot text as a trailing line of the table section. Fix this →

2. Tables are collected document-wide; prose is main-article-only

xml.py:116 walks the whole tree:

for table in soup.find_all("table"):

while prose comes from soup.find("body") — the first body. So tables inside <sub-article> / <response> (peer-review reports and author responses, routine in eLife, F1000Research and BMJ XML) get appended to the main article's cached text with no marker distinguishing them. A reviewer's table then becomes quotable evidence attributed to the paper. The floats-group case you're targeting doesn't require document-wide scope; scoping to tables whose nearest article/sub-article ancestor is the main article would cover it. Fix this →

3. <table> without a <table-wrap> ancestor is dropped entirely

xml.py:117-119:

wrap = table.find_parent("table-wrap")
if wrap is None:
    continue

Fine for canonical JATS, but this extractor also handles whatever a provider returns for format_hint="xml" via apply_full_text_location — including XHTML-ish full text where <table> stands alone. heading already falls back to "Table", so the wrap requirement looks like it's guarding against something specific (layout tables?). Whatever the reason, it deserves a comment, and it's currently untested in either direction.

4. Nested table without its own wrap reuses the ancestor heading

test_nested_tables_and_wraps_once covers the case where the inner table has its own <table-wrap>. When it doesn't — <td>…<table>…</table></td>, which is how continuation tables are sometimes marked up — find_parent("table-wrap") resolves to the outer wrap, so you get two sections both titled ## Table 1 …, reading as a duplicated table with contradictory rows. Minor, but worth a test and probably a suppressed or suffixed heading.

5. Two different int checks for the same kind of field

reference_fetcher.py:727 and :1037 use type(x) is int, while the adjacent HTML checks at :724 and :1030 use isinstance(x, int). The strict form is arguably the better one here (YAML true loads as a bool that isinstance would accept as 1), but having both forms side by side in the same if block invites a future cleanup in the wrong direction. Pick one and record the reason in a comment.

6. Tests and fixtures

Good: real unmodified PMC XML, a real loopback HTTP acquisition rather than a mocked one, provenance and licensing recorded in tests/fixtures/jats/README.md, and version round-trips parametrized over None/0/1/99.

• No coverage for findings 1, 3 or 4 above.
• tests/test_jats_tables.py:35-40 rebuilds ReferenceContent inside the for row loop; hoist it out.
• tests/test_jats_tables.py:245 — thread.join() after server.shutdown() has no timeout, so a wedged handler hangs CI rather than failing it. join(timeout=...) is cheap insurance.
• The 162 KB fixture is reasonable for a regression corpus; just confirm the linked Nature terms cover redistribution in a public repo, not only local text mining, since the README frames it as "subject to the linked Nature terms".

Note on verification

I reviewed statically — running uv run pytest / just test was not permitted in this environment, so I could not execute the new tests or confirm the full suite the PR description says is still running. Findings 1 and 4 are read off the code paths rather than observed, though both follow directly from p.find_parent("table-wrap") and table.find_parent("table-wrap") semantics.
· branch cmungall/p2-68-jats-tables

[claude]

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

---

Review: Preserve JATS tables and refresh legacy XML caches

• Gather context (diff, changed files, prior review threads)
• Review etl/extract/xml.py table + footnote extraction
• Review etl/reference_fetcher.py version migration
• Review tests & fixtures
• Post review feedback

---

The three commits have moved this a long way. Things I checked and think are now right:

• Footnotes are back (xml.py:117,124-128) — iterating ["table", "table-wrap-foot"] in one find_all keeps document order, so a table's notes land immediately after its rows, and each node is visited exactly once so nested wrappers can't double-emit.
• sub-article/response scoping (xml.py:118-119) — reviewer tables and reviewer notes can no longer be quoted as the paper's own evidence.
• Nested table without its own wrap (xml.py:138-139) now gets a Nested table heading instead of silently repeating ## Table 1.
• test_hidden_xml_nodes_are_not_quotable pins a genuinely subtle invariant: type(child) in (NavigableString, CData) at xml.py:98 is an exact-type check precisely because Comment/ProcessingInstruction/Declaration subclass NavigableString. Without that test a future isinstance "cleanup" would make XML comments quotable as evidence.
• Fixture shrank from 162 KB to 8.4 KB with a SHA-256 of the full response, retrieval date, and an explicit statement that the excerpt is not relicensed. That addresses the redistribution question raised earlier.
• Stamping comment at reference_fetcher.py:727 records why type(...) is int rather than isinstance — the intent is now locked in.

Remaining findings, most significant first.

---

1. A table-wrap with no renderable <table> still loses its label and caption

This is the same shape as the footnote regression, in the one case the footnote fix doesn't reach.

extract() at xml.py:206 excludes every <p> under a table-wrap from body prose. _tables_as_text iterates only table and table-wrap-foot nodes (xml.py:117), and appends a section only if rendered: (xml.py:161). So for a table-wrap that carries a <graphic> instead of a <table> — how PMC serves scanned and image-only tables, common in older deposits — the loop never visits the wrap at all, and the caption is in neither the prose nor a table section.

<table-wrap><label>Table 2</label><caption><p>Serum immunoglobulin levels by visit.</p></caption>
<graphic xlink:href="ni.3753-T2.jpg"/></table-wrap>

→ Table 2 Serum immunoglobulin levels by visit. was in the cached body text before this PR; after it, nothing. The same applies to a <table> whose rows all render empty (rendered == []).

docs/troubleshooting.md says "Images and non-HTML table encodings are not transcribed", which is true of the image, but the caption isn't a transcription of the image — it's prose that used to be cached, and the version bump rewrites those entries. Emitting a heading-only section for wraps with no renderable table would cover it in a few lines.

Fix this →

2. Prose is still not scoped to the main article

Tables are now scoped to the main article, but xml.py:201 is unchanged:

body = soup.find("body")

find returns the first <body> in document order. With a main body present that's correct. But this PR's own headline feature is that a body-less, floats-only article now produces output — and for a floats-only article that also carries <sub-article article-type="reply"> or peer-review material (eLife, F1000Research), the first <body> in the document is the reviewer's. The result is reviewer prose concatenated with main-article tables, which is the exact attribution failure finding #2 in the earlier review was about, arriving through the prose door instead of the table door.

It also affects is_stub_notice, which is judged on that same text. Selecting the body whose parent is the root article would make the two halves agree.

Fix this →

3. Nested-wrapper footnote placement is ambiguous, and the test doesn't check it

test_nested_wrapper_footnotes_are_not_duplicated (tests/test_jats_tables.py:272-279) asserts only count(...) == 1 for each note. Document order for that fixture is outer <table>, inner <table>, inner foot, outer foot — so the rendered output is:

## Table … (outer rows)

## Nested/Inner … (inner rows)

Inner note.

Outer note.

Outer note. ends up two sections away from the table it annotates, directly after the inner table's note, with nothing distinguishing them. Counting occurrences is exactly the assertion that passes either way. Since the PR's stated invariant is "nested tables own their rows", asserting the full expected string here (as the other tests do) would pin ownership for notes too — and if the placement is considered wrong, emitting the foot as part of its wrap's section rather than as a free-floating node would fix it.

4. Docstrings drifted behind the last two commits

• xml.py:173-175: "Returns None when neither body paragraphs nor table rows exist, and when the body holds one of PMC's placeholder notices" — these are independent conditions, so it should read or; as written it describes a conjunction that isn't the code. The clause also no longer covers reality: a table-wrap-foot with no table rows produces output, so "neither body paragraphs nor table rows" isn't the None condition any more. (The line also wraps oddly mid-sentence.)
• _tables_as_text (xml.py:111-115) still describes only tables; it now also renders footnotes, and the loop variable is named table while sometimes holding a table-wrap-foot (xml.py:117,124). Renaming to node and adding a sentence would save the next reader the double-take.
• _table_text's inner walk (xml.py:95) says "Render descendants without mutating the parsed document", which doesn't explain the exact-type check on line 98 — the reason (Comment subclasses NavigableString) lives only in a test name.
• MAX_TABLE_ROWS (xml.py:87) got a comment, but unlike MAX_STUB_NOTICE_CHARS / MIN_FULLTEXT_CHARS above it, it still doesn't say why 200 or which direction is safe to move it. Given it silently truncates evidence, that asymmetry is worth closing.

5. isinstance vs type(...) is — the comment landed on one of the two sites

reference_fetcher.py:727 now explains the strict check. _is_stale_cache_entry repeats the same rule at :1035 with no comment, and the HTML check immediately above it (:1029-1034) still uses isinstance. That combination is live: html_full_text_version: true parses as True, isinstance(True, int) is True, and True < 1 is False — so a bogus stamp certifies stale HTML, while the identical XML stamp is correctly rejected. Pre-existing from #80 rather than introduced here, but both forms are now four lines apart in one function, which is where someone will "unify" them in the wrong direction. Worth either fixing HTML to match or carrying the comment across.

The stamping rule itself is now written three times (:190, :414, :727) and read once (:1035). A {content_type: (metadata_key, version)} mapping would stop the next format adding four more branches.

6. Test coverage

Genuinely strong: real unmodified PMC XML, a real loopback HTTP acquisition, version round-trips over None/0/1/99, the 200/201 boundary, test_stub_is_checked_before_tables, and the new hidden-node and sub-article tests. Gaps I'd still fill:

• Backslash escaping — xml.py:152 escapes \ before | and the docs promise it, but only | is exercised (test_structural_tables:55). A cell like A \| B in the source would regress silently.
• rowspan="1" — the != "1" guard at xml.py:154 is untested; a regression there annotates every ordinary cell in every table.
• The real fixture's own footnote — test_real_clinical_rows asserts IgG | Low | Low | High* but not the note defining *, even though the fixture contains IvIg, intravenous immunoglobulin…. That's a one-line assertion that would have caught the original regression against real markup rather than a synthetic one.
• No document-level cap — 200 rows is per table; a floats-only article with 30 tables is unbounded. Nothing tests or documents an upper bound (the docs do at least now say the cap is per table, which is honest).

Nits: tests/test_jats_tables.py:245 — thread.join() with no timeout after server.shutdown() hangs CI rather than failing it if a handler wedges; join(timeout=5) is cheap. tests/test_jats_tables.py:35-40 still rebuilds ReferenceContent inside the for row loop.

7. Migration risk — documented rather than guarded (fine by me, noting it)

The troubleshooting section now states plainly that a successful source refresh returning only an abstract can replace existing full text, and that _stale_fallback doesn't cover that case. Documenting it is a reasonable call for this PR; the guard against downgrading full_text_* to abstract_only is a separate change and would make future version bumps routine.

---

Note on verification: everything above is from reading the code. uv run pytest and just test were both refused in this environment (bash approval restrictions), so I could not execute the new tests or the doctests. Findings 1–3 follow from find/find_all/find_parent semantics and the if rendered: guard rather than from an observed run; if you want them confirmed at runtime, the fastest check for #1 is extracting a table-wrap containing only <label>, <caption> and <graphic>.

Item 1 is the one I'd still treat as blocking, for the same reason the footnote fix was: it removes text that was previously cached, behind a version bump that rewrites those entries.
· branch cmungall/p2-68-jats-tables

[claude]

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

---

Review: Preserve JATS tables and refresh legacy XML caches

• Gather context (diff, changed files)
• Review etl/extract/xml.py table extraction
• Review etl/reference_fetcher.py version migration
• Review tests & fixtures
• Post review feedback

---

Reviewing the branch as it stands after the latest push. Most of what the two earlier passes raised is now resolved in the code rather than only in prose: table-wrap-foot text is re-emitted in document order (xml.py:117,124-128), sub-article/response tables are excluded (:118), nested physical tables get their own Nested table heading instead of repeating the ancestor's label (:138-139), the wrap requirement now carries a comment, the type(...) is int choice is explained at :727, the class docstring no longer claims body-only, extract/__init__.py points at the format-scoped stamps, and the abstract-downgrade-on-refresh risk is written down in troubleshooting.md. The fixture also shrank from 162 KB to 8.4 KB with clear provenance in tests/fixtures/jats/README.md. That's a good response.

Three things I'd still look at, plus nits.

Verification note: uv sync was not approved in this environment, and there's no .venv, so I could not execute the new tests or just test. Everything below is read off the code.

---

1. A table-wrap with no renderable <table> now loses its caption entirely

This is the same shape as the footnote regression that was just fixed, one step over. extract() still drops every <p> under a table-wrap from body prose (xml.py:206), but _tables_as_text emits the label/caption only inside if rendered: (xml.py:161-162). So when a wrap has no <table> at all — the JATS content model allows graphic/media/preformat in its place, which is how scanned and image-only tables are marked up — nothing is emitted, and the caption text that was in the cached body on main is now in neither place:

<body><table-wrap><label>Table 1</label>
  <caption><p>Baseline characteristics of the 42 patients.</p></caption>
  <graphic xlink:href="T1.jpg"/></table-wrap></body>

main cached "Baseline characteristics of the 42 patients."; this branch caches nothing for that wrap. The same applies to a <table> whose rows produce no cells. docs/troubleshooting.md says "Images and non-HTML table encodings are not transcribed", which is true of the image but doesn't say the surrounding caption stopped being cached — and the version bump rewrites every XML entry, so this lands on existing caches.

Emitting the heading when a wrap yields no rows (or narrowing the prose filter to caption/table descendants) closes it. Fix this →

2. Superscripts flatten, which changes numeric meaning in exactly the cells tables carry

_table_text concatenates inline children with no marker (xml.py:98-105), so 10<sup>9</sup> renders as 109 and 10<sup>−8</sup> as 10−8. A lymphocyte count cell 1.2 × 10<sup>9</sup>/L becomes 1.2 × 109/L, and a p-value P < 10<sup>−8</sup> becomes P < 10−8.

p.get_text() on main does the same thing to prose, so this isn't new behaviour — but tables are where exponent notation concentrates, and this PR exists so those rows can be quoted as evidence. A validator matching against 109/L is matching a number the paper does not state. The fix isn't obvious (10^9? Unicode superscripts?), so I wouldn't block on it, but given the docs already carefully explain that spans are annotations and rows aren't a reconstructed grid, this limitation belongs in the same paragraph. Fix this →

3. Tables are scoped to the main article; prose still isn't

_tables_as_text now skips anything under sub-article/response, but prose still comes from soup.find("body") — the first <body> in the document, whichever article it belongs to. test_other_article_tables_are_not_attributed_to_main_paper only covers the case where the main article has a body of its own.

Supporting floats-group is what makes the gap reachable: for a main article with no <body> (the floats-only case this PR deliberately handles) that carries a peer-review <sub-article>, find("body") returns the reviewer's body and its prose is cached as the paper's text — while the reviewer's table beside it is correctly excluded. Pre-existing, but the guard added here is half of it; scoping find("body") to a body whose nearest article ancestor is the root would complete it, and it's one more parametrization of the test you already wrote. Fix this →

---

Smaller points

• Truncation-notice-only section (xml.py:159-162): if the first 200 rows all render zero cells but the table has more than 200 rows, rendered is just ["[Table truncated after 200 rows.]"] and a heading gets emitted for a table with no content. Cheap to guard by checking that a row was actually rendered.
• Two isinstance(metadata, dict) checks (reference_fetcher.py:1028,1036) could share one branch; and the "YAML booleans are not extraction versions" comment lives at :727 while the type(...) is not int that depends on it is at :1038. A pointer in the second spot would keep them from drifting.
• Asymmetry is now deliberate but only half-documented: the XML path rejects a bool stamp, the HTML path at :724/:1030 still accepts True as 1 and writes it back. Fine as a scoped decision — worth saying so where the HTML check lives, or the next reader will align them the other way.
• MAX_TABLE_ROWS (xml.py:88) still gets a one-line comment where MAX_STUB_NOTICE_CHARS and MIN_FULLTEXT_CHARS each explain why that number and which direction is safe. Since this one silently truncates evidence, the same treatment would help.
• Class docstring (xml.py:170) says "tables from the whole document", which is no longer quite true after the sub-article exclusion, and doesn't mention footnotes at all despite them being half the commit.
• Stub check now discards more: because table-wrap paragraphs are filtered out before is_stub_notice, a short body that trips a stub phrase now also discards every table in the document, including floats-group. The trade-off is the documented one and test_stub_is_checked_before_tables pins the behaviour deliberately — just noting the cost went up with this change.

Test coverage

The suite is strong on the things that matter: real unmodified PMC XML, a row asserted end-to-end through SupportingTextValidator, a real loopback HTTP acquisition, the 200/201 boundary, nested wrapper ownership, hidden-node handling, and version round-trips over None/0/1/99. Remaining gaps:

• The real fixture's footnote is never asserted. tests/test_jats_tables.py:32 asserts IgG | Low | Low | High* and the fixture does contain table-wrap-foot, so one more assertion on the IvIg, intravenous immunoglobulin line would prove the footnote fix against real markup, not only the synthetic case at :259.
• Backslash escaping — implemented at xml.py:152 and promised in the docs; only | is exercised (:55).
• rowspan="1" / colspan="1" — the != "1" guard at xml.py:154 is untested; a regression there annotates every ordinary cell.
• A <table> with no wrap — now a documented, commented decision, but nothing pins it.
• tests/test_jats_tables.py:35-40 still rebuilds ReferenceContent inside the for row loop.
• tests/test_jats_tables.py:245 — thread.join() with no timeout hangs CI rather than failing it if a handler wedges.

---

Item 1 is the one I'd treat as blocking, for the same reason the footnote one was: it removes text that was previously cached, and the version bump applies it to every existing XML entry. Items 2 and 3 are fidelity/scoping concerns rather than breakage. The row rendering itself — escaping order, nesting ownership, span annotation, footnote placement — reads correct to me.
· branch cmungall/p2-68-jats-tables

[claude]

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

---

Review: Preserve JATS tables and refresh legacy XML caches

• Gather context (diff, changed files, prior review threads)
• Review etl/extract/xml.py table/footnote/caption extraction
• Review etl/reference_fetcher.py version migration
• Review tests & fixtures
• Post review feedback

---

Reviewing the branch as pushed (2 commits). The two blocking items from the earlier passes are now closed in code, not prose, and I checked each:

• Footnotes — xml.py:130 iterates ["table-wrap", "table", "table-wrap-foot"] in one find_all, so document order puts a table's notes directly after its rows and each node is visited exactly once.
• Non-renderable captions — the newest commit emits _table_heading(wrap) at the wrap's own document position (:133-137), independent of whether any row renders. test_unrenderable_table_keeps_caption parametrizes graphic / <table/> / <table><tr/></table>, which is exactly the three shapes that were losing text.
• Prose scoping — extract() now selects the first <body> not under sub-article/response (:215-222), so the floats-only + peer-review case no longer caches reviewer prose as the paper. test_floats_only_article_does_not_take_reviewer_body pins it.
• Attribution prose — the second text join (:253-261) keeps <attrib>/<long-desc> paragraphs that no renderer emits, while excluding exactly the four containers that are re-emitted. I traced every <p> position I could think of under table-wrap; I did not find one that is both retained as prose and rendered by _tables_as_text, nor one that is dropped by both.
• Duplicate-heading avoidance — heading = "" if _table_heading(wrap) else "Table" at :148 pairs correctly with the wrap-position emission, and the find_parent(["table", "table-wrap"]) is not wrap test at :149 still catches the unwrapped nested table.
• test_cell_boundary_contracts closes the backslash and rowspan="1" gaps from earlier rounds.

Verification note: there is no .venv and uv run pytest was not approved in this environment, so I could not execute the tests or just test. Everything below is read off the code.

Remaining findings, most significant first. None are blocking in my view.

---

1. The stub gate now sees less text, so a short article with a big table can be discarded whole

is_stub_notice is judged on the first text join (xml.py:232-241), which excludes every <p> under a table-wrap. On main that text included caption paragraphs and <td><p> cell paragraphs — which is how PMC marks up most cells, and the fixture's own table is no exception.

MAX_STUB_NOTICE_CHARS is a length gate whose comment explains at length that length is the only thing making the phrase list safe to match. Removing table paragraphs from the measured text shortens it, which moves articles into the matchable band rather than out of it. A 900-character erratum or brief communication that is mostly one table, and that happens to say "restricted", now trips the gate where it previously cleared 1000 characters on cell text alone — and because the check precedes table rendering, the return is None for the entire document, floats-group included.

test_stub_is_checked_before_tables deliberately pins the direction you want (a table cannot mask a stub). The converse direction is new and untested. The blast radius is limited — pmid.py:423 falls through to the HTML path when XML yields nothing — but the reasoning recorded on MAX_STUB_NOTICE_CHARS (xml.py:31-49) now describes a text that no longer exists, and that comment is the thing a future reader will trust. Worth a sentence there and a test for a short table-heavy body that must survive. Fix this →

2. Table discovery still spans <front> and <back>; prose is main-<body> only

_tables_as_text walks soup.find_all(...) over the whole tree and excludes only sub-article/response (xml.py:130-131). Prose comes from the main <body>. Two asymmetries fall out:

• A table-wrap inside <abstract> (structured clinical abstracts do carry them) is rendered into full text, even though the class docstring and test_structural_tables establish that abstract content is the source's job and apply_full_text_location prepends the source's abstract separately.
• <back> appendix tables are rendered while the appendix prose around them is not, so a row can be quoted with no surrounding context and no marker saying it came from an appendix.

Neither produces wrong attribution the way the sub-article case did, and appendix tables are arguably wanted. But "tables are found throughout the document" is now the only rule, and the class docstring at xml.py:184 says "main-article body paragraphs followed by labeled tables", which does not describe front/back matter. Scoping discovery to body + floats-group (+ back, explicitly) would make the two halves agree and is a one-line predicate next to the existing exclusion. Fix this →

3. Two rendering edges in _tables_as_text

• Notice-only section (xml.py:170-175): when a table has more than 200 <tr> but none of the first 200 yields a cell (<tr/> rows, or rows holding only nested-table cells), rendered is exactly ["[Table truncated after 200 rows.]"] and a section — with a ## Table heading when the wrap is unlabelled — is emitted for a table with no content at all. Guarding on "at least one row rendered" before appending the notice is two lines.
• Sibling tables under one wrap (xml.py:148): when a wrap holds two <table> elements (a multi-part table, or <alternatives> carrying two encodings rather than graphic+table), the first gets heading = "" and so does the second, because find_parent(["table", "table-wrap"]) is the same wrap for both. The output is two unlabelled row blocks separated by a blank line under one caption, which reads as one table with a gap. The Nested table mechanism doesn't fire here since neither is nested. Low frequency, but it's the one duplication shape the nesting tests don't cover.

4. Nested-wrapper footnote placement is still asserted only by count

test_nested_wrapper_footnotes_are_not_duplicated (tests/test_jats_tables.py:273-280) checks count(...) == 1 for each note. For that fixture the document order is outer table → inner table → inner foot → outer foot, so Outer note. renders two sections away from its own table, immediately after the inner table's note, with nothing distinguishing them. Counting passes either way. Every other test in this file asserts the full expected string; doing the same here would pin note ownership the way test_nested_tables_and_wraps_once pins row ownership — and would make it visible if the placement is considered wrong.

5. type(...) is int — intent recorded at one of the four sites, pinned by none

The comment at reference_fetcher.py:726 explains the strict check on the write path. The identical rule at :1038 has no comment, and the HTML check four lines above it (:1030-1032) still uses isinstance, so html_full_text_version: true loads as True, passes isinstance(True, int), and True < 1 is False — a bogus stamp certifies stale HTML while the same stamp is correctly rejected for XML. Pre-existing from #80, but both forms now sit in one function, which is where someone unifies them in the wrong direction.

More to the point: nothing tests the bool rejection. test_xml_version_roundtrip parametrizes None/0/1/99; adding true as a case would make _is_stale_cache_entry return True and the stamp vanish on rewrite, and would turn the comment into an executable invariant. That's the cheapest insurance against the "cleanup" the comment is worried about.

The stamping rule itself is now written three times (:190-193, :414-417, :726-729) and read once (:1036-1039). A {content_type: (metadata_key, version)} mapping would stop the next format adding four more branches.

6. Docstrings and small stuff

• xml.py:187-189: "Returns None when neither body paragraphs nor table content exists, and when the body holds one of PMC's placeholder notices" — these are independent conditions, so it should read or; as written it describes a conjunction the code doesn't implement. The sentence also wraps mid-phrase.
• xml.py:184 says "main-article body paragraphs followed by labeled tables and notes" — accurate about sub-articles now, but see finding 2 about front/back.
• _tables_as_text's loop variable is still named table while holding a table-wrap or table-wrap-foot roughly half the time (:130-146). node would save the next reader a double-take, since the first two branches continue before it ever means "a table".
• _table_heading(wrap) is computed twice per table — once at :134 for emission, once at :148 only to test emptiness — and each call does wrap.find_all(name) across the whole wrap subtree, i.e. the entire 200-row table, then filters by parent. Since JATS puts label/caption as direct children of table-wrap, find_all(name, recursive=False) would be both cheaper and equivalent; or just hoist the value.
• MAX_TABLE_ROWS (xml.py:87-88) now has a one-line comment, but MAX_STUB_NOTICE_CHARS and MIN_FULLTEXT_CHARS directly above each explain why that number and which direction is safe to move it. Since this one silently truncates evidence, the asymmetry is still worth closing.

7. Tests and fixtures

Genuinely strong, and better than the last round: real unmodified PMC markup with the footnote now asserted (test_real_clinical_rows:29), a real loopback HTTP acquisition rather than a mock, a row validated end-to-end through SupportingTextValidator, the 200/201 boundary, hidden-node handling, nested ownership, sub-article exclusion both for tables and for body selection, and version round-trips over None/0/1/99. The fixture at 8.4 KB with a SHA-256 of the full response, a retrieval date and an explicit "not relicensed" settles the redistribution question from the earlier pass.

Gaps I'd fill, in order:

• A bool version stamp (finding 5) — the one that protects a comment with a test.
• Short-body-plus-table stub survival (finding 1).
• Notice-only table and two tables under one wrap (finding 3).
• No document-level cap. 200 rows is per table; a floats-only article with 30 tables is unbounded. The docs are honest that the cap is per table, but nothing bounds or tests the whole.
• tests/test_jats_tables.py:36-40 still rebuilds ReferenceContent inside the for row loop — hoist it above the loop.
• tests/test_jats_tables.py:246 — thread.join() with no timeout after server.shutdown() hangs CI rather than failing it if a handler wedges; join(timeout=5) is cheap.

8. Migration risk — documented, not guarded (noting, not objecting)

docs/troubleshooting.md now states plainly that a successful source refresh returning only an abstract can replace existing full text, and that _stale_fallback covers "source returned nothing", not "source returned an abstract". Documenting rather than guarding is a reasonable call for this PR; the guard against downgrading full_text_* to abstract_only is a separate change that would make future version bumps routine. The stamping placement itself is right: fetch():190 sits after source.fetch and apply_full_text_location:414 after a successful acquisition, so no load/save round trip can certify legacy text — test_xml_version_roundtrip and test_xml_offline_stays_stale both pin that.

---

The row rendering — escaping order, span annotation, nested ownership, footnote emission, caption preservation — reads correct to me, and the migration is scoped as tightly as it should be. Finding 1 is the one I'd want a test for before merge; the rest are hardening and documentation.
· branch cmungall/p2-68-jats-tables