Cache url: HTML pages as readable text, so quotes through links and bold text match

[caufieldjh]

Fixes #102.

Since #92, URLSource cached sanitized markup. A quote could only match text inside a single text node, so a sentence that ran through a link, bold or emphasis failed even when copied exactly. Examples: the Flint press release, Wikipedia lead sentences, ZFIN's license line. A whole page also sat on one unreadable line.

Commits (best reviewed one at a time)

1. html_to_text (etl/extract/html.py). It flattens a whole page:

   • inline tags are unwrapped in place;
   • block-level tags and <br> start a new line;
   • a table row becomes one line, with its cells separated by |;
   • <pre> keeps its whitespace, and entities are unescaped;
   • non-content elements and comments are dropped exactly as sanitize_html drops them, through a shared helper.

   As the bot comment on url: pages: a quote that runs through a link or bold text fails; cache readable text instead of markup #102 suggested, it does not reuse HTMLExtractor. That extractor keeps only <p> text when a page has any, and narrows to <article>/<main>; either would lose text that matches today.

2. URLSource uses it, and URL_SOURCE_CACHE_VERSION goes 1 → 2. The title is still read from the raw page's citation_title first. Two old tests asserted that markup survives; they are replaced by one that checks the readable-text shape.

3. Docs: validate-urls.md and troubleshooting.md, including a migration note.

The invariant

"Every quote that matches today still matches." A property test checks this over the HTML fixtures (PLOS, DSpace, IRIS) and two synthetic pages: every text node of the old sanitized markup, normalized, appears in the normalized flattened text.

Open questions from #102

• Cell separator: |. Normalization drops punctuation, so a quote copied from a rendered row with spaces between cells still matches. There's a test for this.
• Migration: a troubleshooting note says the refresh rewrites every cached HTML page, which makes a large diff in a committed cache. It also re-fetches each page, so a page that has changed may stop supporting a quote. An unreachable page keeps its old entry, as before.

Verification

• The issue's repro, run against the live page: This press release was produced by the City of Flint. now gives Valid: True. The cache entry is 107 lines with no markup.
• New and updated tests, all doctests, mypy and ruff all pass.
• Full suite, one file at a time: 1,364 of 1,367 passed. The 3 failures are in test_pmid_network (2) and test_html_fulltext_acceptance (1). They pass when rerun alone, and the same files also fail on main on this machine (5 and 1 failures), from network and local-server timeouts.

🤖 Generated with Claude Code

[claude]

Claude finished @caufieldjh's task in 1m 13s —— View job

---

Review of PR #103: cache url: HTML as readable text

• Read the PR diff
• Check how html_to_text fits with the existing code
• Probe edge cases (I couldn't run tests: this job has no permission for uv run pytest, so everything below comes from reading the code)
• Post the review

Overall: the change is well focused and the design holds up. The PR rightly does not reuse HTMLExtractor. Moving the drop step into _without_non_content means caching and flattening can't drift apart. The property test (every text node of the old sanitized markup still appears in the flattened text) checks the right invariant. Raising the cache version to 2 and adding the stale-entry test makes old caches refresh correctly. I found no blocking problems, only a few edge cases and small fixes.

Possible bugs and edge cases

1. Text in a <tr> that isn't inside a <td>/<th> is lost (html.py, the tr branch of _flatten). The branch reads only child.find_all(["td", "th"], recursive=False). Any other child is dropped: stray text, or a wrapper element. Older layout pages often use <tr><form><td>…</td></form></tr>, and there every cell is lost. The old sanitized markup kept all of that text, so this breaks the stated invariant. The property test misses it only because no fixture contains it.
   One fix is to walk child.children and use _cell_text for cells and _lines(...) for everything else, then join the parts with CELL_SEPARATOR. A test with <table><tr><form><td>a</td><td>b</td></form></tr></table> would cover it.
   Fix this →

2. A <br> inside <pre> joins the words on either side. child.get_text() ignores <br>, so <pre>a<br>b</pre> becomes "ab". The invariant still holds, because each old text node is a substring, but a quote of a b won't match. Normalization keeps word boundaries, so this matters. It's rare, but some code and log viewers do put <br> inside <pre>. Replacing each br in the pre with "\n" before calling get_text() would fix it.

3. <pre> output doesn't match _lines' docstring. splitlines() keeps blank lines and trailing whitespace from inside <pre>, but _lines says it returns "the non-blank lines". Keeping them is probably intended, since it preserves the block's whitespace. If so, update the docstring. If not, filter the blank lines.

4. Deep nesting can hit the recursion limit. _flatten uses one Python stack frame per nesting level. A page nested more than about 990 tags deep would raise RecursionError from URLSource.fetch, and the page would fail instead of being cached. Some generated or broken pages do nest that deeply, but it's uncommon. Converting to an explicit stack is optional; at least a short note in the docstring would help.

Code quality

• TEXT_BREAK_TAGS is a frozenset built from the BLOCK_LEVEL_TAGS tuple. That's fine, but td/th now appear in both the break set and the special tr path. That's correct: a cell outside a <tr> breaks a line. A short comment saying so would help readers.
• Dropping empty cells (if cell) means a row's columns no longer line up across rows. That's fine for matching, but worth noting in the CELL_SEPARATOR docstring.
• Skipping PreformattedString but keeping CData is correct, and the type(text) is NavigableString filter in the property test is a good touch.
• Docstrings and doctests follow the repo conventions (CLAUDE.md), with no unneeded try/except.

Performance

The page is parsed once (before, sanitizing also re-serialized it), and the walk is linear in the page size. I see no problems. The cached entries are also much smaller than the old markup.

Security

This improves on the old cache, which kept markup. Nothing from scripts, styles, comments or attributes reaches the cache. Entities are unescaped into plain text, so the result is safe as long as nothing renders the cached content as HTML. I found nothing that does.

Test coverage

Coverage is good: the three repros from #102, quotes copied from table rows, pre, entities, the stale cache version, and the title still being read from the raw page. Gaps worth adding:

• A <tr> with a non-cell child (item 1).
• <br> inside <pre> (item 2).
• A cell containing a nested table, to pin down how _cell_text flattens it.

On the open questions

• A | separator looks right, and it is tested with a quote that has no separators.
• The migration note is good. You could also suggest a separate commit for the cache refresh, so its large diff doesn't hide content changes in review.

[claude]

Claude finished @caufieldjh's task in 47s —— View job

---

Re-review of PR #103 after the latest push

• Read the commits since the last review (11546fa..39c4d42)
• Check each earlier review item against the new code and tests
• Look for new edge cases
• Post the review

I couldn't run tests in this job, because uv run pytest and uv run python need approval here. Everything below comes from reading the code. To let me run them, add Bash(uv run:*) to --allowedTools.

Overall: every item from the last review is fixed, each in its own commit and each with a test. I see no blocking problems, so this looks ready to merge from my side.

Earlier review items

#	Item	Status
1	Text in a <tr> outside its cells was lost	✅ Fixed. _row_cells now walks every child of the row and steps into wrappers that contain cells. Tests cover cells inside a <form>, stray text and a stray element.
2	<br> inside <pre> joined the words on either side	✅ Fixed. _pre_text turns each <br> into \n. Covered by a doctest and test_br_inside_pre_breaks_a_line.
3	_lines docstring didn't match how <pre> is handled	✅ The docstring now says what is kept, and test_blank_lines_are_kept_only_inside_pre checks it.
4	Deep nesting could hit the recursion limit	✅ _lines now walks the page with its own stack, using a _BLOCK_END marker to end a block's last line. A test checks 5,000 nested <div>s.
—	Comment explaining why td/th are in TEXT_BREAK_TAGS	✅ Added
—	Note in CELL_SEPARATOR's docstring that empty cells are dropped	✅ Added
—	Test for a table nested inside a table cell	✅ test_a_table_nested_in_a_cell_folds_into_that_cell
—	Advise committing the cache refresh on its own	✅ Added to troubleshooting.md, including that entries refresh only as validations use them

Remaining notes (none blocking)

1. _row_cells still uses recursion. It calls itself for wrappers inside a row (html.py, the child.find(["td", "th"]) branch). A table inside a cell also recurses, through _cell_text → _lines → _row_cells. So one of these could still raise RecursionError:

   • a row whose cells sit under hundreds of nested wrappers;
   • tables nested hundreds of levels deep.

   The _lines docstring already says only nested tables recurse. Real pages almost never nest like this, so I'd leave the code as it is. You could add "or wrappers inside a row" to that sentence so it stays accurate.

2. find(["td", "th"]) also finds cells of a nested table. In <tr><div><table><tr><td>x</td><td>y</td></tr></table></div></tr>, the <div> is treated as a wrapper, so x and y become cells of the outer row (x | y). This doesn't break matching, because normalization drops |. It also follows the same idea as the nested-in-a-cell case. I'm mentioning it so the behaviour is a known choice.

3. A <pre> inside a table cell loses its line breaks. _cell_text joins the cell's lines with a space, so the <pre> collapses to one line, though spaces inside each line stay. This is fine for matching. The html_to_text docstring says "<pre> keeps its whitespace" without exceptions; you could add "except in a table cell".

4. Small point: _lines turns each text node's whitespace runs into one space, and _end_line collapses runs of spaces again. Both steps are needed, because separate nodes such as "x " + " y" produce a double space. It's correct, just worth knowing it's on purpose.

Code quality, performance, security

• The switch from recursion to a loop is clean. Pushing children in reverse and putting _BLOCK_END beneath a block's children is easy to follow, and the comment explains it.
• _row_cells calls child.find(...) on each non-cell child of a row. That search is linear in the child's size and happens only once per child, so the cost doesn't grow in a worrying way.
• Security is unchanged since my last review: nothing from scripts, styles, comments or attributes reaches the cache.
• The docstrings and doctests follow CLAUDE.md, and there is no try/except.

Test coverage

Coverage now includes every edge case I raised, alongside the property test over the fixtures. One optional addition: a test for a <pre> inside a <td>, to pin down the behaviour in note 3.