Raise or parse correctly where the docs Markdown parser silently mangled input - #534
Raise or parse correctly where the docs Markdown parser silently mangled input#534dormouse-bot wants to merge 3 commits into
Conversation
… mangled The parser's contract is that anything outside its CommonMark subset is a hard error, not a silent degradation. Four constructs slipped through it: - a fence closed on a shorter run than its opener, so a ````-fenced block ended at the first ``` inside it and spilled its body into the page as prose -- the nesting form AGENTS.md prescribes; - a link destination stopped at the first `)`, truncating a URL carrying a balanced pair and still emitting a live anchor; - a blockquote's lazy continuation swallowed the heading, list, or fence that followed it, which also dropped that heading from the page's TOC; - a setext underline joined the paragraph and shipped as literal text. Generated docs data is byte-identical before and after, so nothing published today changes.
Deploying mouseterm with
|
| Latest commit: |
eedff36
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://451b9733.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-docs-parser-silent-mangl.mouseterm.pages.dev |
dormouse-bot
left a comment
There was a problem hiding this comment.
Two gaps in the same family as the four cases fixed here, both in the code this diff touches. Verified by running the parser at this head; the suggestions below are one coherent change (all three go together), and I confirmed they keep generate-docs.js output byte-identical across all four docs.*.json, same as the PR's own check.
A thematic break that interrupts a paragraph is still silently mangled. interruptsParagraph has no thematic-break arm, so a *** or ___ line directly under paragraph text is folded into the paragraph, and parseInline then eats the run as an empty emphasis span:
input: Some text parsed as: paragraph[ text("Some text "),
*** em(children: []),
Next text("* Next") ]
GitHub renders that as paragraph / <hr> / paragraph. Two characters vanish and a horizontal rule silently disappears, with a green build — the exact shape case 4 exists to eliminate, one marker character away. --- is already covered because SETEXT_UNDERLINE catches it first, which is also why the new guard has to stay ahead of the thematic-break arm: adding the break to interruptsParagraph instead would make Title\n--- a paragraph plus a rule, silently diverging from the <h2> GitHub renders.
The same missing arm also lands on case 3. > quoted followed by --- now raises the "setext heading underline — use an ATX # heading" error for a line the author wrote as a thematic break, so the error names the wrong construct; per CommonMark a thematic break is not paragraph continuation text, so the quote should just end there. The third suggestion fixes that.
interruptsParagraph's docstring overclaims, and the blockquote fix inherits the gap. "Lines that begin a new block" is four of the seven block starts — tables, thematic breaks, standalone <img>, and block-level raw HTML are all missing. Concretely, a table directly under a > line is still swallowed whole:
> quoted
| a | b |
|---|---|
| 1 | 2 |
parses to a single blockquote whose only child is the paragraph quoted | a | b | |---|---| | 1 | 2 |. Table detection needs lookahead to the delimiter row, so it doesn't fit the one-line predicate signature — but either the predicate takes the lookahead or the docstring (and the PR body's "ending the quote at a line that starts a new block") should say which starts it covers, so the next editor doesn't read completeness into it.
One smaller note on the spec line: Headings are ATX only is enforced in paragraphs and blockquote bodies but not inside list items, which parseList assembles by joining continuation lines straight into parseInline — - item / Title / ===== still ships as the literal text item Title =====. Not worth chasing on its own, but the spec sentence reads as global.
Also checked, nothing to report
matchDestination— balanced-paren scan,\skip, and"title toggle all behave;[a](/x "the (title)"),.png), and[a](/x?q="hi") tailall parse correctly. An unbalanced(or an odd"in the destination now yields literal text rather than a truncated href, which is the sameclose === -1path the old code had and a visible output either way.closesFence— correct for~as well as backtick, for a closing run longer than the opener, and the empty-line case is guarded by the length test.- The extracted
interruptsParagraphpredicate is character-identical to the inline condition it replaced, so the paragraph loop's existing behavior is unchanged. - Line numbers in an error raised inside a blockquote are relative to the quote body, not the document (
setextat document line 9 reports line 3). Pre-existing for every error type the nested parse can raise, not introduced here. - Byte-identical
docs.{guide,selfhost,cli,skill}.jsonbefore and after this PR's parser: confirmed independently. pnpm lint:specspasses at this head.
A thematic break interrupting a paragraph was folded into it, and parseInline then ate the run as an empty emphasis span -- `***` shipped as a dropped rule and two lost characters, the same shape as the setext case. The paragraph loop now breaks on it, after the setext throw so `---` keeps raising rather than becoming a rule, and the blockquote loop ends the quote there so `> quoted` followed by `---` no longer raises an error naming a construct the author did not write. interruptsParagraph covered four of the seven block starts while its docstring claimed all of them, so a table or a block-level raw HTML tag directly under a `> ` line was still swallowed whole. It now takes one line of lookahead -- a `|` row is a table only when a delimiter row follows -- and covers six; a standalone `<img>` stays out because it is inline-level, and the thematic break stays out because it must be tested after the setext underline. `Headings are ATX only` was enforced in paragraphs and blockquote bodies but not in list-item continuations, which joined straight into parseInline, so `- item` / `Title` / `=====` shipped as literal text. The same two guards now apply there. Generated docs data stays byte-identical.
dormouse-bot
left a comment
There was a problem hiding this comment.
One gap, and it is a regression this commit introduced rather than a leftover: the new table lookahead turns a construct the previous head raised on back into a silent mangle.
isDelimiterRow matches a pipeless ---, so the lookahead reads a setext heading as a table. interruptsParagraph's new arm is line.includes('|') && isDelimiterRow(next ?? ''), and isDelimiterRow's regex — /^\s*\|?\s*:?-{1,}:?\s*(\|\s*:?-{1,}:?\s*)*\|?\s*$/ — makes \|? optional throughout, so a bare --- (and :---) passes. A paragraph line that merely contains a pipe, followed by a setext underline, now breaks the paragraph and gets consumed by the table branch:
input: Intro fe1e6b1: THROW setext heading underline (line 3)
a | b cea9b88: paragraph("Intro"), table(header=[a, b], align=[null], rows=[])
--- GitHub: <h2>Intro a | b</h2>
GFM requires the delimiter row's cell count to match the header's, so a | b over --- is 2-vs-1 and not a table there either — GitHub reads the --- as a setext underline. The parser now emits a two-column header with a one-element align, zero rows, and no error. That is the exact shape this PR exists to eliminate, and the same two lines raised correctly at fe1e6b1, so it is the commit that reopened it.
The narrow fix is to make isDelimiterRow require the pipe GFM requires — it also closes the block-level table branch, which has the same hole from main (a | b / --- at the start of a block parses as a table today):
function isDelimiterRow(row) {
return row.includes('|') && /^\s*\|?\s*:?-{1,}:?\s*(\|\s*:?-{1,}:?\s*)*\|?\s*$/.test(row);
}Verified at this head with that one line applied: both Intro\na | b\n--- and the bare a | b\n--- raise the setext error instead of mangling; > quoted / table, Intro / table, and the escaped-pipe shortcut table all still parse identically; and all four canonical sources plus AGENTS.md, CHANGELOG.md, and docs/specs/website-docs.md produce byte-identical block trees against main's parser, so the byte-identical claim in the PR body survives it. No existing test uses a pipeless delimiter row.
Also checked, nothing to report
- The three findings from the previous review are all applied and correct at
cea9b88, verified by running the parser rather than by reading the diff:Some text/***/Nextgives paragraph / thematicBreak / paragraph,Title\n---still raises,> quoted\n---gives blockquote / thematicBreak,> quoted+ table gives blockquote / table,> quoted\n<div>raisesraw HTML <div>, and- item/Title/=====raises with a document-relative line 3. - Guard ordering in
parseListmatches the paragraph loop (setext throw, then thematic break), and the continuation loop's!LIST_ITEMtest means- - -never reaches either guard — it exits the loop and lands on the block-level thematic break, which is the same answer. interruptsParagraph's raw-HTML arm only changes where the existing rejection fires:Some text\n<kbd>…threw at line 1 before and line 2 now, which is the more accurate line.<img>is correctly excluded,</img>is not.closesFenceis equivalent to the two regexes it replaced apart from the length test; the empty-line case is covered bytrimmed.length >= length.matchDestination's\skip and"toggle behave;[Foo](https://x.test/wiki/Foo_(bar))and the nested ````/``` fence both parse correctly at this head.next ?? ''covers the last-line case; none of the new regexes carriesg, so there is nolastIndexstate.- A thematic break in a list-item continuation ends the list and emits a top-level rule where GitHub nests it in the item — a structural divergence rather than lost content, and strictly better than the dropped rule it replaces. Not worth chasing.
parseListstill joins continuation lines straight intoparseInline, so a table, fence, or ATX heading indented under a list item is still swallowed as literal text. Pre-existing and out of this PR's scope; noting it because the two guards added here are the shallow form of that fix.
| || LIST_ITEM.test(line) | ||
| || /^\s*>\s?/.test(line) | ||
| || /^(\s*)(`{3,}|~{3,})/.test(line) | ||
| || (line.includes('|') && isDelimiterRow(next ?? '')) |
There was a problem hiding this comment.
isDelimiterRow's regex makes every | optional, so a bare --- or :--- satisfies this lookahead. Intro / a | b / --- therefore breaks the paragraph here and gets consumed by the table branch as a two-column header with a one-element align and no rows — where fe1e6b1 raised the setext error and GitHub renders an <h2>. Fixing it at the arm would leave the block-level table branch's identical hole open, so the fix belongs in isDelimiterRow (return row.includes('|') && /^\s*\|?…/.test(row)), per the review body.
There was a problem hiding this comment.
Fixed in eedff36 — applied at isDelimiterRow as suggested, so the block-level table branch's identical hole (the bare a | b / --- case, which mangled the same way on main) closes with it.
Both shapes now raise setext heading underline — use an ATX \#` heading, at line 3 and line 2 respectively. Confirmed the regression first by running the three parser revisions against the same input: 0ba3d54folded it into one paragraph,fe1e6b1raised,cea9b88 produced the two-column/one-align` table you describe.
website/ is green (18 files, 166 tests — the one added case pins both shapes), as are lint:specs, its self-test, and lint:public-docs. Regenerating docs.{guide,selfhost,cli,skill}.json with this parser and with main's gives byte-identical files, so the PR body's claim survives.
No spec change: the governing rule is already in docs/specs/website-docs.md -> "Markdown parsing" as Headings are ATX only, and the reason the pipe is required constrains this one module, so it lives as a docstring at isDelimiterRow alongside the THEMATIC_BREAK ordering note.
The align-length case one step further out — a delimiter row that has a pipe but whose cell count disagrees with the header (a | b over | ---), which GFM also says is not a table — is untouched and still mangles. It carries no setext ambiguity, so it is not the shape this PR set out to close; noting it rather than widening the diff.
…s a setext underline `isDelimiterRow` made every `|` in its pattern optional, so a bare `---` (and `:---`) passed as a one-column delimiter row. The table lookahead added in cea9b88 therefore broke a paragraph at any line containing a pipe when a setext underline followed it, and the table branch consumed both: `Intro` / `a | b` / `---` parsed as a two-column header with a one-element `align` and no rows, where fe1e6b1 raised the setext error and GitHub renders an `<h2>`. GFM requires the delimiter row to carry a pipe, so requiring one at the source closes the block-level table branch's identical hole too — the bare `a | b` / `---` case, which mangled the same way on main. Both cases now raise the setext error. Generated docs.{guide,selfhost,cli,skill} .json stay byte-identical to main's parser.
website/scripts/docs-parser.jsopens by promising that anything outside its CommonMark subset is a hard error, "rather than degrading silently the way a general parser would" — anddocs/specs/website-docs.md-> "Markdown parsing" states the same rule. Seven constructs slip through that guarantee today and silently mangle instead. None is reachable from the current sources (I checked README.md, SELF_HOST.md, vscode-ext/README.md, and dor/skill.md), but all seven are shapes a routine edit to a hand-authored canonical doc can introduce, and the failure is a wrong page with a green build.Each is fixed to either parse correctly or raise. Verified end-to-end: running
generate-docs.jsagainst the real sources before and after produces byte-identicaldocs.{guide,selfhost,cli,skill}.json, so nothing published today changes.The seven cases, with the observed output
1. A fence closed by a shorter run. The closing matcher was
/^\s*`{3,}\s*$/regardless of how long the opener was, so a ````-fenced block closed at the first ``` inside it. This is the exact form AGENTS.md prescribes for nesting one code block inside another, so it is the likeliest of the seven to be hit.The block's body escaped the code block and rendered as prose. Fixed by requiring the closing run to be at least as long as the opener's, per CommonMark.
2. A
)inside a link destination.matchLinktooktext.indexOf(')'), so[Foo](https://x.test/wiki/Foo_(bar))yieldedhref: "https://x.test/wiki/Foo_(bar"plus a stray)in the prose — a live anchor pointing somewhere else. Fixed with a balanced-paren scan that treats a"-quoted title as literal.3. A blockquote swallowing the next block. The lazy-continuation loop consumed any non-blank line once the quote had a body, so
> Note\n# Headingput the heading inside the blockquote. GitHub renders it as a real heading, and because the nested parse'sheadingsare not merged upward, the heading also vanished from the page's table of contents. Fixed by ending the quote at a line that starts a new block — the same predicate the paragraph loop already used, now factored intointerruptsParagraphand shared.4. Setext headings.
Title\n=====has no block in this parser, so the underline joined the paragraph and shipped as the literal textTitle =====. Rejected withUnsupportedMarkdownErrorrather than supported:#is the only heading form the sources use, and an error names the line.5. A thematic break interrupting a paragraph.
Some text/***/Nextfolded the rule into the paragraph, andparseInlinethen ate the run as an empty emphasis span: the rule disappeared and two characters vanished, exactly case 4's shape one marker character away. The paragraph loop now breaks on it — after the setext throw, so---keeps raising rather than silently becoming a rule where GitHub renders an<h2>. The blockquote loop ends the quote there too, since per CommonMark a thematic break is not paragraph continuation text; without that,> quotedfollowed by---raised a setext error naming a construct the author did not write.6. A table or a raw HTML block under a
>line.interruptsParagraphcovered four of the seven block starts while its docstring claimed all of them, so> quotedfollowed by a table parsed as one blockquote whose only child was the paragraphquoted | a | b | |---|---| | 1 | 2 |. It now takes one line of lookahead — a|row is a table only when a delimiter row follows it — and covers six. A standalone<img>stays out deliberately (it is inline-level, so GitHub keeps it in the paragraph); the thematic break stays out because it has to be tested after the setext underline, and the docstring now says so instead of claiming completeness.7. A setext underline inside a list item.
parseListjoins continuation lines straight intoparseInline, so- item/Title/=====shipped as the literal textitem Title =====— the spec line "headings are ATX only" was enforced in paragraphs and blockquote bodies but not here. The same two guards now apply in the continuation loop.Tests
Fifteen cases in
docs-parser.test.js. Thirteen fail onmainand pass here; two (still folds a lazy continuation line into the blockquote,keeps a standalone <img> line inside the paragraph it follows) pass both ways and guard cases 3 and 6 against over-correcting.pnpm testinwebsite/is green (18 files, 166 tests), as arelint:specs, its self-test, andlint:public-docs.Spec
docs/specs/website-docs.md-> "Markdown parsing" gains two facts that would let an editor reintroduce cases 1 and 4 if deleted: headings are ATX only, and a fence closes only on a run at least as long as the opener's. The rest are correctness within constructs the spec already claims, and the one ordering constraint that binds a single module — the setext test has to precede the thematic-break test — lives as a comment atTHEMATIC_BREAK. The section stayed inside its word budget — no--ratchet.An eighth case, a regression the second commit introduced and the review caught: the table lookahead's
isDelimiterRowleft every|optional, so a bare---read as a one-column delimiter row andIntro/a | b/---became a two-column header with a one-elementalignand no rows. Requiring the pipe GFM requires closes it, and the block-level table branch's identical hole frommainwith it.Surfaced by the nightly code-quality sweep.