fix(content): decode HTML entities in one pass - #394
Conversation
decodeHtmlEntities chained replaceAll calls and decoded "&" first. A single-escaped "&lt;" became "<", which the next call in the same chain decoded again into "<". Text that shows an entity to the reader, which pages about HTML syntax and double-encoding CMS feeds both produce, therefore had real markup injected into the extracted article text before the model saw it. Replace the chain with one regex pass and a lookup table, so each entity is decoded exactly once. Unknown entities are still left untouched.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed August 31, 2026, 1:04 AM ET / 05:04 UTC. ClawSweeper reviewWhat this changesThis PR replaces sequential HTML-entity substitutions in link-preview content cleaning with a one-pass lookup and adds regression coverage for escaped and unknown entities. Merge readinessKeep open for ordinary maintainer merge review: current main still has the sequential decoder, while this focused patch fixes the demonstrated double-decoding behavior with sufficient production-path evidence and no blocking finding. Priority: P2 Review scores
Verification
How this fits togetherSummarize's link-preview pipeline turns fetched page HTML into normalized article text for downstream summarization. The changed core helper decodes supported HTML entities after extraction while preserving text that intentionally displays an escaped entity. flowchart LR
A[Page HTML] --> B[Link preview extraction]
B --> C[Content cleaner]
C --> D{Recognized HTML entity}
D -->|Decode once| E[Normalized article text]
D -->|Leave unchanged| E
E --> F[Summarization input]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the narrow one-pass decoder and its regression tests so intentionally escaped markup remains literal text in raw HTML extraction. Do we have a high-confidence way to reproduce the issue? Yes—source-reproducible with high confidence: current main's ordered replacements directly turn < into <, and the PR supplies a retained regression failure plus an extraction-path comparison. Is this the best way to solve the issue? Yes. A single global replacement pass retains the existing supported entity set and removes the cascade without adding a second decoding policy or configuration surface. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 66202d92f055. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (24 earlier review cycles; latest 8 shown)
|
|
Went to get the extraction-path proof and it corrected the PR rather than confirming it. Running the real paths on
What the fix does change is every caller that hands the helper raw html: The description now lists which call sites are affected and which are not, with that measurement. |
clawsweeper
Bot
added
proof: sufficient
|
Triage recommendation: LAND. I reproduced the cascade on current main and verified this PR through the built core package's exported The sequential replacements consume text produced by the first Reviewed commit: Both content PRs also apply together cleanly on current main and pass all 12 cleaner regression tests. The baseline package build passed on Node 24.20.0, and each changed cleaner was compiled into the built core package for the integration checks. Local tests reused the installed dependency tree (Vitest 4.1.10); each original PR's exact-head CI is green. Codex autoreview was scoped-clean at the default P0 threshold. No source repair or branch rewrite was needed. No merge performed. Preserve Suggested landing changelog: “Content extraction: decode HTML entities once so deliberately escaped markup remains literal text (#394, thanks @devYRPauli).” |
2 participants
Problem
decodeHtmlEntitiesdecodes with a chain ofreplaceAllcalls, and&is decoded first:Each call runs over the output of the previous one, so a single-escaped
&lt;becomes<at step one and step two decodes it again into<. One level of escaping is unwrapped twice.Measured on the helper:
&lt;div&gt;<div><div>&quot;x&quot;"x""x"&amp;&&<p><p><p>Only entities after
&in the chain are affected. The last two rows show ordinary input is unaffected, so this is specific to the chain order.Which callers this actually affects
This matters only where the helper is handed RAW html, meaning entities are still in their source form. Callers that regex-strip tags and then decode:
content/browser-html.ts:61decodeHtmlEntities(withBreaks.replace(/<[^>]+>/g, " ")...)content/browser-html.ts:88decodeHtmlEntities(value.replace(/<[^>]+>/g, " "))link-preview/content/article.ts:161insideextractPlainTextlink-preview/content/firecrawl.ts:30callsextractPlainText(html)directlyarticle.ts:48and:106, inside the sanitize-htmltextFilterparsers.ts:75and:90, onelement.textContentThe last four are not affected because sanitize-html and the DOM have already decoded one level before the helper runs, so it only ever sees single-escaped text there.
I checked this by running the real extraction paths rather than reasoning about them:
extractArticleContentis unchanged, because it goes through sanitize-html.extractPlainTextis fixed. An earlier version of this description claimed the sanitize-html path was affected; that was wrong and the table above replaces it.Change
One regex pass with a lookup table, so each entity is decoded exactly once. The set of entities and their replacements is unchanged, including the case-sensitive
'and/. Unknown entities are still left untouched.Proof
Source change reverted, tests kept:
Full suite, unchanged from before this commit:
oxlintandoxfmt --checkon both changed files: exit 0.The existing test at
tests/cleaner.test.ts:27still passes. It did not catch this because its&is standalone and surrounded by spaces, so decoding it never produces a new entity for a later call to consume.