Repository navigation
fix(lab-ops-hub): Worker-fallback extractor parity with container Extractor (#569) - #3918
Conversation
…ractor (#569) Add extractHtml to src/lib.ts mirroring the container Extractor (HTMLParser) semantics: every <a> start tag counts (bare/self-closing included), anchors in script/style/comments do not, title is raw character data with entities decoded and never leaks into text, script/style content is skipped (unclosed swallows the rest), text chunks are space-joined and whitespace-collapsed. crawlFallback now uses it; status codes and output limits unchanged. One shared fixture matrix (test/extractor-fixtures.json) drives both the TS and Python parity tests.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ed061c3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Self-closing detection now parses attributes: a slash ending an unquoted attribute value (src=/x/) is part of the value, so script/ style raw-text mode still engages, matching HTMLParser. - Numeric character references get HTML normalization: C1 controls 0x80-0x9F remap (e.g. € -> euro), NUL/surrogates/out-of-range become U+FFFD, matching html.unescape. - Comment scanner accepts --!> and the abrupt <!--> / <!---> forms instead of swallowing the rest of the document. - Six paired fixtures added to extractor-fixtures.json; TS suite 79/79 and Python parity suite 65/65 pass on the laptop.
|
@codex review Fix round 741b85a pushed for the three P2 findings on 50ed061 (all threads replied + resolved): attribute-aware self-closing detection, HTML numeric character-reference normalization (C1 remap + U+FFFD), and --!> / comment terminators — six new paired fixtures; TS suite 79/79 and Python parity suite 65/65 green on the laptop. Please re-review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741b85ac21
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (quote !== "") { | ||
| if (ch === quote) quote = ""; | ||
| } else if (ch === '"' || ch === "'") quote = ch; | ||
| else if (ch === ">") break; |
There was a problem hiding this comment.
Track attribute boundaries before opening quote mode
When an unquoted attribute value contains an apostrophe, the scanner treats it as the start of a quoted value and can consume the remaining document instead of recognizing subsequent tags. For example, <a href=/it's>link</a><p>body</p> returns the entire HTML as text with zero links, while the container Extractor under Python 3.13.13 returns link body with one link. The previous fallback also stripped this markup correctly. Only enter quote mode when beginning a quoted attribute value, rather than whenever a quote character appears.
Useful? React with 👍 / 👎.
| const code = Number.parseInt(body.slice(hex ? 2 : 1), hex ? 16 : 10); | ||
| return String.fromCodePoint(normalizeNumericRef(code)); | ||
| } | ||
| return NAMED_ENTITIES[body] ?? whole; |
There was a problem hiding this comment.
Restrict named-entity lookup to own properties
For unknown entity references whose names match inherited object properties, this lookup substitutes JavaScript functions into the extracted content. For example, <p>&constructor; &toString;</p> becomes function Object() { [native code] } function toString() { [native code] }, whereas both the previous fallback and the container Extractor under Python 3.13.13 preserve the literal references. Use an own-property check or a null-prototype entity map so unknown names remain unchanged.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
| if (next === "/") { | ||
| const m = /^<\/([a-zA-Z][^\s/>]*)\s*\/?>/.exec(html.slice(i)); |
There was a problem hiding this comment.
Accept recoverable end tags before leaving raw-text mode
When a script or style closing tag contains trailing attributes, the raw-text scanner recognizes the closing name, but this stricter regex rejects the tag, so handleEndTag never clears skipDepth and the remaining article text is discarded. For example, <script>x</script foo><p>body</p> returns empty text here, whereas the container Extractor under Python 3.13.13 returns body; the previous fallback also preserved the body. Parse and consume these recoverable end tags consistently with the container so they actually terminate the skipped region.
Useful? React with 👍 / 👎.
Three parity divergences from the container Extractor, each with a shared fixture verified against the Python suite first: (1) tag scanner enters quote mode only where a quoted attribute value begins (right after =), so an apostrophe inside an unquoted value (<a href=/it-s>) no longer swallows the document; (2) named-entity decoding uses an own-property lookup, so &constructor; and &toString; stay literal instead of resolving to Object prototype functions; (3) raw-text mode consumes recoverable end tags with trailing attributes (</script foo>), so skipDepth clears and following text is kept. Both parity suites green: node 82/82, python 65 OK.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17347f07b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while (k < len) { | ||
| while (k < len && /\s/.test(inner[k])) k += 1; | ||
| if (k >= len) return false; | ||
| if (inner[k] === "/") return /^\s*$/.test(inner.slice(k + 1)); |
There was a problem hiding this comment.
Require the self-closing slash to immediately precede
>
For a page containing <script / >var s='<a href=x>fake</a>';</script><p>real</p>, accepting whitespace after the slash incorrectly makes the script self-closing. The Worker consequently exposes script source in the extracted text and counts the fake anchor. Checked against the container's Extractor under Python 3.13.13, the result is instead text real with zero links; the previous Worker fallback also excluded this script source. Require the slash to be the final character before > rather than accepting trailing whitespace.
Useful? React with 👍 / 👎.
| continue; | ||
| } | ||
| if (next === "/") { | ||
| const m = /^<\/([a-zA-Z][^\s/>]*)\s*\/?>/.exec(html.slice(i)); |
There was a problem hiding this comment.
Consume recoverable end tags outside raw-text mode
For HTML containing trailing attributes on an ordinary closing tag, such as <p>a</p foo><p>b</p>, this strict pattern rejects the closing tag and sends its markup into the extracted article text. The Worker now returns a < /p foo> b, whereas the container's Extractor under Python 3.13.13 and the previous Worker fallback both return a b. Apply tolerant closing-tag consumption in the normal parsing path as well, so recoverable markup does not contaminate stored text and excerpts.
Useful? React with 👍 / 👎.
Self-closing slash must immediately precede the closing bracket: isSelfClosingTag now requires the slash to be the final character of the tag, so a script tag with a spaced slash stays an open raw-text element. Recoverable end tags are consumed in the normal parsing path too, not only in raw-text mode: trailing attributes on an end tag no longer leak into extracted text. Two shared fixtures added, expected values verified against the container Extractor first; node 84/84, python 65 OK.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a179aba170
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // End tags may carry (invalid but recoverable) attributes, which | ||
| // HTMLParser consumes rather than emitting as text: </p foo> | ||
| // still closes the paragraph. | ||
| const m = /^<\/([a-zA-Z][^\s/>]*)[^>]*>/.exec(html.slice(i)); |
There was a problem hiding this comment.
Avoid quadratic backtracking when parsing incomplete end tags
For an untrusted page ending with </p followed by a long string of letters and no >, the overlapping [^\s/>]* and [^>]* repetitions cause quadratic backtracking. An input containing just 20,000 trailing letters took approximately 434 ms in the new extractor, versus 0.05 ms in the previous fallback; larger inputs remain well below the 2 MB response cap but can exhaust Worker CPU. Check for a terminating > before matching, or parse the tag name and remainder without overlapping repetitions.
Useful? React with 👍 / 👎.
| export function decodeHtmlEntities(data: string): string { | ||
| return data.replace(/&(#(?:[xX][0-9a-fA-F]+|\d+)|[a-zA-Z][a-zA-Z0-9]*);/g, (whole, body: string) => { |
There was a problem hiding this comment.
Decode the complete set of HTML character references
For pages using standard entities outside the small lookup table or references without semicolons, the new extractor does not match the container. Verified against the container Extractor under Python 3.13.13: <title>Α &</title><p>→ A & B</p> produces title Α & and text → A & B, while the Worker preserves every reference literally. This makes stored titles and article text depend on which crawling path runs. Use complete HTML named-reference data and support the semicolon-omission rules rather than requiring every reference to match this restricted pattern.
Useful? React with 👍 / 👎.
| const handleStartTag = (tag: string, selfClosing: boolean) => { | ||
| if (tag === "title") { | ||
| if (!selfClosing) cdataTag = "title"; | ||
| } else if (tag === "a") links += 1; | ||
| else if (tag === "script" || tag === "style") { |
There was a problem hiding this comment.
Honor raw-text modes for textarea and iframe elements
For pages containing literal markup inside textarea, iframe, xmp, noembed, or noframes, the scanner incorrectly interprets that content as ordinary HTML. For example, <textarea><script></textarea><p>Article body</p> returns empty text because the literal <script> activates script skipping and swallows the remaining document. Verified against the container Extractor under Python 3.13.13, the expected text is <script> Article body; the previous Worker fallback also retained the article body. Implement the container parser's additional RCDATA/raw-text modes so literal content cannot activate nested tags or discard subsequent article text.
Useful? React with 👍 / 👎.
…#569) Codex round-4 P1: the tolerant end-tag pattern (name class plus remainder class, both unbounded) backtracked quadratically on an incomplete end tag at EOF -- 20k trailing letters took ~434ms vs 0.05ms for the previous fallback, on untrusted page input. The end-tag match is now bounded at the next closing bracket before matching the name (0.72ms on the same input, output unchanged), and a letter-led incomplete end tag at EOF is dropped, matching HTMLParser. Fixture added for the EOF case; node 85/85, python 65 OK.
Fixes #569 (queue task 4868).
The Worker fallback (
crawlFallback) and the container crawler disagreed on extraction: the fallback's regexes missed bare<a>, counted anchors inside script strings and comments, returned title entities raw, and leaked style/title into text.src/lib.ts: newextractHtmlmirroring the containerExtractor(PythonHTMLParser) semantics — every<a>start tag counts (bare and self-closing included); script/style are CDATA-skipped (an unclosed one swallows the rest);<title>is raw character data with entities decoded and never leaks into text; comments/declarations are not markup; text chunks are space-joined and whitespace-collapsed exactly asapp.pydoes.src/index.ts:crawlFallbackusesextractHtml. No status-code or output-limit change on either door.test/extractor-fixtures.json(17 cases: bare anchors, script/comment markup, entities, titles, styles, unclosed script/style, whitespace, truncation-adjacent chunking, quoted>in attributes), driven by bothtest/extractor.test.tsandtest/test_extractor_parity.pyso the two doors cannot drift again.Tests (laptop):
npm run test:ts73 pass;npm run test:py65 pass.Need help on this PR? Tag
@codesmith-botwith what you need.