Skip to content

Heading ids: every raw tag starting with br yields a space (#709) - #710

Open
philcunliffe wants to merge 9 commits into
masterfrom
fix/issue-709
Open

Heading ids: every raw tag starting with br yields a space (#709)#710
philcunliffe wants to merge 9 commits into
masterfrom
fix/issue-709

Conversation

@philcunliffe

@philcunliffe philcunliffe commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Collects the deferred findings from #705. One of the three is a real parity bug and is fixed here; the other two stay deferred and are now recorded in the doc instead of living only in the PR thread.

Root cause

headingId turns <br> into a space because pandoc's reader makes it a LineBreak and slugs it like any other whitespace token. The regex doing that was spelled <br(?:\s[^>]*)?\/?>, deliberately narrowed on a stated rationale (in a review record and in the code comment above headingId) that it must not be <br[^>]*> "so it does not swallow <brand>", presented as matching pandoc.

That rationale is false. pandoc 3.1.11's stringify keys on the raw inline's leading text, not on a parsed tag name, so any raw inline HTML token whose text starts with the case-sensitive prefix <br becomes a space. -t native shows the mechanism: <brand> and <BR> are both RawInline (Format "html"), and only the lowercase-prefixed one contributes a space. So <brand>, <bra>, <br-x> and <breakfast time> are all line breaks to pandoc, and the narrowed regex was minting one hyphen too few for every one of them.

The fix is the regex the old comment ruled out: /<br[^>]*>/g, case-sensitive. The comment is corrected either way, since it recorded a false rationale.

Widening is safe because the regex never sees a string marked did not already accept as a tag. marked escapes invalid tag syntax to entities before headingId runs (## A <br@> B arrives as A &lt;br@&gt; B), and pandoc likewise leaves it as text. Both sides mint a-br-b.

Verified against a real pandoc 3.1.11 binary

Every row measured one heading per document, so a de-dup counter drift cannot cascade and misreport parity. 36 cases, 11 fixed, 0 regressions, 36/36 parity after.

The br prefix family (fixed)

Heading pandoc before after
## A <brand> B a---b a--b a---b
## A <brand>x</brand> B a--x-b a-x-b a--x-b
## A <bra> B a---b a--b a---b
## A <br-x> B a---b a--b a---b
## A <br2> B a---b a--b a---b
## A <brand attr="y"> B a---b a--b a---b
## A <brand/> B a---b a--b a---b
## A <breakfast time> B a---b a--b a---b
## A <brand > B a---b a--b a---b
## A <brand>B a--b a-b a--b
## A<brand> B a--b a-b a--b

Only the open tag counts: </brand> starts </b, not <br, which is why <brand>x</brand> gains exactly one hyphen.

The exclusions (unchanged, and now pinned)

Heading pandoc before after
## A <BR> B a--b a--b a--b
## A <Br> B a--b a--b a--b
## A <bR> B a--b a--b a--b
## A </brand> B a--b a--b a--b
## A <b> B a--b a--b a--b
## A <bold> B a--b a--b a--b
## A <span> B a--b a--b a--b
## A <custom> B a--b a--b a--b
## A <q> B a--b a--b a--b
## A <br@> B a-br-b a-br-b a-br-b

No regressions in the existing cases

## A <br> B, ## A <br/> B, ## A <br /> B, ## A <br class="x"> B (all a---b), ## <br> (-), ## Line one<br/>Line two (line-one-line-two), ## A <br><br> B (a----b), ## A <span> </span> B (a---b), ## A<em>B</em>C (abc), ## A <span>a</span>b B (a-ab-b), ## A <brand C (a-brand-c), ## A <br (a-br), ## A <br="x"> B (a-brx-b), ## A &lt;brand&gt; B (a-brand-b) all match pandoc before and after.

Items fixed vs recorded

Item Disposition
1. Raw tags whose name starts with br Fixed
2. \s matching U+2028 / U+2029 / U+FEFF Recorded, still deferred
3. Image alt-text ids Recorded, still deferred

Items 2 and 3 stay deferred, and I re-measured both to confirm the deferral still holds:

  • Item 2. A headingId fix closes only the entity half (## A &#8232; B mints a---b against pandoc's a--b). The literal-authored spellings break further upstream, in marked's block parser, before any renderer override can see them: a literal U+2028 or U+2029 in a heading line splits the block so no heading and no id at all is produced, and a literal U+FEFF welds its neighbours. A half fix that made the entity spelling right while the literal spelling lost its heading outright would be worse than the honest gap.
  • Item 3. pandoc slugs a Markdown image's alt text in and contributes nothing for a raw-HTML <img> (x-alt-text-y vs x--y). marked emits byte-identical HTML for the two forms (X <img src="i.png" alt="alt text"> Y, measured a === b true), so no string-level discriminator exists for headingId to key on. A correct fix needs token-type work in the heading renderer.

LLP 0208 (item 4)

The doc promised heading ids "so existing in-page anchors keep resolving" without naming where that is inexact. It now carries a #heading-id-gaps consequence naming the two remaining classes with their measurements, and headingId carries a @ref LLP 0208#heading-id-gaps [constrained-by] pointing at it.

On LLP 0208. #705 has since squash-landed on master (12cb2f4), so 0208 is a settled Active record and its decided text is off limits. This PR therefore leaves the Decision paragraph exactly as landed and only adds the consequence section above. git diff origin/master -- llp/0208-... is additive only: no line of the settled doc is removed or reworded.

Tests

A new case in test/core/report-render.test.js covers the whole br-prefix family plus every exclusion, 19 assertions. Verified to fail against the pre-fix render.js:

not ok 1 - every raw tag whose name starts with `br` yields a space, matching pandoc 3.1.11
  error: '<brand> yields a space, exactly as <br> does'
  code: 'ERR_ASSERTION'
  expected: true
  actual: false

Assertion by assertion, pre-fix: all 10 br-prefix assertions FAIL, all 9 exclusion assertions already pass (they are the guard against the widened regex over-matching). Post-fix all 19 pass.

Gate

Check Result
npm test 3920 pass, 0 fail, 1 skipped
npm run typecheck clean
npm pack --dry-run 900 files, 1.8 MB
test/core/llp-ref-hygiene.test.js 11 pass, 0 fail

Fixes #709

bgmcmullen and others added 8 commits August 10, 2026 16:10
Supersedes LLP 0196 open question 1's keep-pandoc resolution through the escape
hatch that resolution named for itself: the only pandoc property the component
vocabulary relies on is gfm passing raw HTML through untouched, "so in-process
rendering stays available if the dependency ever becomes a problem." Server-side
generation (hypaware-server LLP 0112) made it one twice over: pandoc would be
that daemon's first non-npm binary, and execFileSync blocks the single thread
every customer org shares, measured at ~40ms per page.

The substitution was measured before it was made: all 94 files of a real
reports tree converted under both engines; 66 structurally identical, 28
differing only in pandoc's syntax-highlighting markup, 0 genuine differences.
After the swap the same tree renders to 94 pages with zero structural diffs
against the pandoc reference, and the style-before-theme link order holds on
every page.

- pandocPage becomes htmlPage: marked (pinned, zero transitive deps) plus an
  explicit standalone template doing what -s did, with assets/head.html inlined
  after the base stylesheet link so theme.css keeps loading last
  (LLP 0196#theme-layer).
- A renderer override reproduces pandoc's heading ids, -1 suffixes included, so
  existing in-page anchors keep resolving.
- hasPandoc, the CLI preflight, the help text, the CI apt-get step, and the
  skill's prerequisite all go. The render tests lose their skip guard and run
  everywhere: a renderer with no external dependency has no excuse for untested
  paths. 3892 tests pass, none skipped for pandoc.
- Syntax highlighting is the one visible change, light mode only: the
  stylesheet's token rules sit in its dark-mode block and flattened pandoc's
  spans to one colour anyway. language-* classes survive for later colour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…by tests

Round-1 review of #705. The pandoc-to-marked swap was measured structurally,
which is exactly the comparison that cannot see either of these: one changes an
attribute's spelling, the other changes text the browser never shows.

- headingId was fed marked's already-escaped inline HTML, so `&` reached the
  slug rule as `&amp;` and left its letters behind: `Cost & Usage` minted
  cost-amp-usage where pandoc mints cost--usage, and every authored
  [link](#whats-next) pointed at what39s-next. It now strips tags, unescapes,
  collapses whitespace runs before dropping punctuation (a dropped `&` leaves
  two spaces and pandoc emits both hyphens), and hyphenates per space rather
  than per run. Unicode letter and number classes keep `Café résumé` and
  `日本語` intact, as pandoc does. Re-verified 24/24 against pandoc 3.1.11
  `-f gfm -t html5`, `-1` repeat suffixes included.
- A tablecell override restates column alignment as pandoc's inline
  `style="text-align: ..."`. marked's built-in emits `align="right"`, a
  presentational hint the cascade ranks below assets/style.css's
  `th, td { text-align: left }`, so every right-aligned numeric column and
  every centred column silently rendered left, against tabular-nums.
- test/core/report-render.test.js gains a golden page over the authoring
  vocabulary (component block byte-for-byte, aligned table, fenced block with
  its language class, headings carrying `&`, `'` and `/`) plus an anchor
  integrity check: every href="#..." must match an id on the page. Both fixes
  above fail this test before they land.
- src/core/reports/README.md drops the stale "pandoc is still a hard
  dependency" rule and pandoc's `-H` for head.html. It ships in the package.
- assets/style.css suppresses the list marker on task lists, which pandoc's
  template did through a task-list class marked does not emit. Footnotes are a
  real loss (marked's gfm has none) and are accepted and recorded rather than
  extension-patched.
- LLP 0208 narrows its measurement claim to what is true of tables, and records
  the footnote and task-list consequences. LLP 0196 gains a header-level
  Superseded-in-part-by forward-ref.

Co-Authored-By: Claude <noreply@anthropic.com>
…k lists

Four residual round-2 findings on PR #705's pandoc-to-marked swap, each
verified against a real pandoc 3.1.11 binary.

Finding A: heading ids dropped combining marks (`[^\p{L}\p{N}_\s-]`
stripped `\p{M}`), mangling Indic, Thai, Arabic/Hebrew, Vietnamese and
decomposed Latin text unconditionally. headingId now NFC-normalizes
before lowercasing and keeps `\p{M}` in the retained class, matching
pandoc on decomposed "Café résumé", Turkish dotted-I, and Devanagari.

Finding B: unescapeHtml only covered the five entities marked itself
emits, so an author-written entity (`&rsquo;`, `&#x27;`, ...) leaked its
raw letters/digits into the slug and could dangle an in-page anchor.
Replaced the five-entity table with a general decoder: numeric
(`&#NNN;`, `&#xHH;`) plus a bounded, case-sensitive "HTML4" named-entity
table (Latin-1, Greek, typography), sourced from the WHATWG entity list
and stored as codepoints rather than literal characters. Case-sensitive
matching was chosen over the case-insensitive suggestion after measuring
pandoc itself: it decodes `&AMP;` (a real legacy dual-case alias) but not
`&MDASH;` or `&RSQUO;`, and a case-fold would wrongly collide distinct
entries like `&Alpha;`/`&alpha;`. Fixing this also surfaced a second bug:
whitespace-run collapsing had to move to before entity decoding, or a
decoded `&nbsp;` merged with real spaces around it into one hyphen
instead of pandoc's one-hyphen-per-token count (verified: `A  &nbsp;  B`
-> `a---b`).

Finding C: the `ul:has(> li > input[type="checkbox"])` rule only matched
"tight" task lists; a blank-line-separated ("loose") list wraps the
checkbox in `li > p > input`, so it kept its bullet. The selector now
matches both shapes. Also matched pandoc's cheap-to-copy indent
behavior: pandoc's template kept the list's normal indent and pulled the
checkbox left with a negative margin, rather than zeroing padding-left.
LLP 0208's task-list bullet is corrected to name both shapes instead of
just one.

Finding D: a heading that reduces to nothing (`## <emoji>`) minted
`id=""`; pandoc emits no id attribute. The heading renderer now omits
the attribute when the slug is empty, while still running pandoc's own
de-dup counter against the empty base for a repeat (`id="-1"`).

Each fix is pinned in test/core/report-render.test.js, verified to fail
against the pre-fix renderer before the change landed.

Co-Authored-By: Claude <noreply@anthropic.com>
Two heading-id findings from the round-2 entity decoder, both measured against
a real pandoc 3.1.11 binary.

An out-of-range numeric character reference in a heading crashed the whole
render. `unescapeHtml` called `String.fromCodePoint` behind only a
`Number.isFinite` guard, and that throws above U+10FFFF. The window is
reachable rather than theoretical: marked's escaper passes numeric references
of up to 7 decimal or 6 hex digits through untouched, so every value in
`&#x110000;`-`&#xFFFFFF;` and `&#1114112;`-`&#9999999;` arrived verbatim.
The blast radius is what made it worth fixing now: `renderReports` wipes
`html/` before building and builds in sorted slug order, so one bad heading in
one report destroyed the already-built pages of every report sorting at or
after it and left the landing page stale. Reports are model-authored, so the
input is not fully under a human's control. pandoc substitutes U+FFFD, which
its slug then strips, minting `a--b` for `## A &#x110000; B`, so the guard
substitutes U+FFFD too rather than returning the entity intact, which would
leak its digits into the id. Lone surrogates and U+10FFFF stay on the
`fromCodePoint` path, where they already matched pandoc.

`headingId` also trimmed after the punctuation strip, which pandoc does not
do. Any heading starting or ending with stripped punctuation next to a space
minted a different id: `## 🚀 Rollout plan` gave `rollout-plan` where pandoc
gives `-rollout-plan`, and `## end &` gave `end` where pandoc gives `end-`.
Emoji-led headings are ordinary in model-authored reports, so every pandoc-era
`#-rollout-plan` anchor dangled silently, contradicting LLP 0208's promise
that existing in-page anchors keep resolving. The trim is dropped, which
restores parity across the whole measured table, including the all-whitespace
case (pandoc mints `-` for `## ( )`, not no id) and the de-dup counters that
run off those degenerate bases. Headings that reduce to the truly empty string
still carry no id, and the reader has already trimmed authored outer
whitespace, so no stray leading hyphen appears.

Regression tests cover both, plus the amplifier: a malformed entity in one
report must not destroy the pages of the reports that sort after it.

Co-Authored-By: Claude <noreply@anthropic.com>
…pellings

Three heading-id divergences from the pandoc that master shelled out to. Each
one still renders the heading correctly, so the only visible symptom is a
pandoc-era `#anchor` that silently dangles. All measured against a real pandoc
3.1.11 binary with one heading per document, so no de-dup counter drift can
cascade between cases.

Uppercase `&#X...;` never decoded. The entity regex spelled the hex alternative
`#x[0-9a-fA-F]+`, which made the existing `body[1] === 'X'` guard unreachable:
marked's escaper passes the uppercase form through (its no-encode pattern
spells the prefix `#[Xx]`), so it arrived verbatim and leaked its digits.
`## A &#X41; B` minted `a-x41-b` against pandoc's `a-a-b`. This also left
`&#X110000;` inside the out-of-range window unsubstituted, leaking digits where
the guard's own comment says it must not, so that comment is corrected too.

`&apos;` was missing from NAMED_ENTITIES, so `## What&apos;s next` minted
`whataposs-next` against pandoc's `whats-next` - a single-entry hole in the
exact case the decoder's docstring uses to motivate itself, while `&#39;`,
`&#x27;` and `&rsquo;` were all already right. The rest of the HTML5
ASCII-punctuation block had the same hole and is added with it: all 27 names
were verified individually against pandoc rather than taken on trust.

`<br>` was deleted outright by the general tag strip where pandoc's reader
yields a space, welding `## Line one<br/>Line two` into `line-oneline-two`.
It now substitutes a space before the tag strip and after the whitespace
collapse, because pandoc counts that space as its own token: `## A <br /> B`
mints `a---b`, not `a-b`. The match is lowercase-only, since pandoc treats
`<BR>` as raw inline HTML contributing nothing. Every other tag still vanishes
without a trace; `<span>a</span>b` and `A<em>B</em>C` are pinned so a future
general tag-to-space rule cannot regress them. Moving the collapse ahead of the
tag strip also stops a tag's own inner whitespace being merged away, which
pandoc likewise counts as its own token. The heading renderer's comment claiming
pandoc emits no id for a `<br>`-only heading was wrong and is corrected: pandoc
mints `-`, and now so does this.

Two measured divergences are left alone deliberately. U+2028, U+2029 and U+FEFF
are in JS's `\s` but pandoc drops them, each minting one extra hyphen; the
literal-authored spellings break further upstream in marked's block parser
(no heading is emitted at all), so a character-class fix would only close half
the case. And pandoc slugs a Markdown image's alt text (`## ![img](x.png)` ->
`img`) but emits no id for a raw-HTML `<img alt="...">`, which this renderer
already matches; marked emits byte-identical HTML for both, so there is no
string-level discriminator and a fix belongs at the token level, not here.

LLP 0208's footnote consequence is tightened: it described only the multi-token
definition, which survives as literal text. A single-token one (`[^1]: notes.md`)
is parsed as a link reference definition instead, so the definition line
disappears and the reference becomes a live link that rewriteHrefs then rewrites.

Co-Authored-By: Claude <noreply@anthropic.com>
Deferred findings from PR #705, which this stacks on.

Item 1 (fixed). pandoc 3.1.11's stringify keys on a raw inline's LEADING
TEXT, not on a parsed tag name, so ANY raw inline HTML token whose text
starts with the case-sensitive prefix `<br` becomes a space. The regex
here was spelled `<br(?:\s[^>]*)?\/?>` on the stated rationale that it
must not "swallow `<brand>`"; pandoc swallows `<brand>` too, so that
rationale was false and the code comment recording it is corrected.
Widening to `/<br[^>]*>/g` closes 11 divergences with no regressions,
verified case by case against a real pandoc 3.1.11 binary, one heading
per document so no de-dup counter drift can read as parity:

  `## A <brand> B`          a--b  -> a---b    (pandoc a---b)
  `## A <brand>x</brand> B` a-x-b -> a--x-b   (only the open tag counts)
  `## A <bra> B`            a--b  -> a---b
  `## A <br-x> B`           a--b  -> a---b
  `## A <br2> B`            a--b  -> a---b
  `## A <brand attr="y"> B` a--b  -> a---b
  `## A <brand/> B`         a--b  -> a---b
  `## A <breakfast time> B` a--b  -> a---b
  `## A <brand   > B`       a--b  -> a---b
  `## A <brand>B`           a-b   -> a--b
  `## A<brand> B`           a-b   -> a--b

The exclusions stay excluded: `<BR>`, `<Br>` and `<bR>` miss the
lowercase prefix, `</brand>` starts `</b`, and `<b>`, `<bold>`,
`<span>`, `<custom>` and `<q>` are not `br` at all. Widening is safe
because the regex never sees a string marked did not already accept as
a tag: marked escapes invalid tag syntax to entities first, and pandoc
likewise leaves `<br@>` as text.

Items 2 and 3 (recorded, not fixed). Both stay deferred, with the
measurement behind them now in the doc instead of only the PR thread.

Item 4. LLP 0208 records the footnote, task-list and highlighting
consequences but promised heading ids "so existing in-page anchors keep
resolving" without naming where that is inexact. A new
`#heading-id-gaps` consequence names the two remaining classes, and
`headingId` carries a `@ref` to it.

Co-Authored-By: Claude <noreply@anthropic.com>
master carries PR #705 as the squash 12cb2f4, whose blobs for
llp/0208-report-renderer-drops-pandoc.decision.md, src/core/reports/render.js
and test/core/report-render.test.js are byte-identical to 3c6876e, this
branch's parent commit. The add/add and content conflicts were therefore
purely lineage, not intent: master had nothing to contribute to those three
files beyond what 3c6876e already held, so each resolves to this branch's
side, which is 3c6876e plus the #709 fix. The merged tree differs from master
by exactly the #709 commit's diff (verified byte for byte).
… gloss

LLP 0208 is Status: Active and merged to master, so its Decision paragraph
is a settled record; revert the added qualifier on the heading-id sentence
back to the master text and keep only the additive #heading-id-gaps
consequence section.

The @ref gloss in render.js overstated the constraint: the entity spellings
of the Unicode separators/BOM are decoded by unescapeHtml and are reachable
inside headingId, only the literal spellings break above this function in
marked's block parser. Reword the gloss to match the LLP section it cites.
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round 1 (head 7caee9e)

Verdict: findings. Three actionable, all now fixed. The substance of the PR (the regex widening and its regression test) is correct and was not changed; every finding was about accuracy of the surrounding record.

codex is not installed on this host, so this was a single-family review (careful reading plus empirical runs), not a dual review.

Findings

1. minor - llp/0208-report-renderer-drops-pandoc.decision.md:44 - edited a settled Active LLP.

The PR added a qualifier to the Decision paragraph's promise:

-`-1` suffixes on repeats included, so existing in-page anchors keep resolving;
+`-1` suffixes on repeats included, so existing in-page anchors keep resolving
+(exactly, but for the two narrow classes at [#heading-id-gaps](#heading-id-gaps));

The PR body justified this on the premise that "LLP 0208 is not on master (git cat-file -e origin/master:llp/0208-... fails)". That premise has since been falsified: #705 squash-landed as 12cb2f4, and master was merged into this branch at 7caee9e. 0208 is now a landed Status: Active record, so CLAUDE.md's rule binds, and adding a caveat to a promise the Decision made is not inside the typo/link/status carve-out.

Fixed. The Decision paragraph is reverted to byte-identical with origin/master. The additive #heading-id-gaps consequence section is kept (a new consequence is additive, not an edit to what was settled), and its <a id="heading-id-gaps"></a> anchor is intact so the code's @ref still resolves. git diff origin/master -- llp/0208-... is now additive only: no line of the settled doc is removed or reworded. The PR body's falsified justification was replaced with the accurate one.

2. minor - src/core/reports/render.js:278-281 - the @ref gloss stated something false.

The gloss claimed both deferred gaps "diverge ABOVE this function ... so neither is fixable by changing the rule below". That is wrong for half the separator class: the entity spellings (&#8232;, &#65279;) are decoded by unescapeHtml inside headingId, survive the retained \s branch, and are hyphenated by .replace(/\s/g, '-') at render.js:359. Measured on the PR head: <h2 id="a---b">A &#8232; B</h2> against pandoc's a--b. Only the literal spellings break upstream in marked's block parser. LLP 0208 itself says the correct thing ("Fixing the slug rule would close only that half"); the gloss collapsed it into a falsehood that would send a future reader away from the exact function producing the bug, which is the same comment-tells-you-the-wrong-thing failure this PR exists to fix.

Fixed. The gloss now matches the LLP: entity spellings are reachable here, literal spellings break in marked's block parser so a fix here closes only half the class, and image alt text needs token types this function only sees as rendered HTML. Comment only, no code touched.

3. nit - PR body carried a stale merge-ordering constraint.

The body still said "this branches from #705's head (3c6876e), not master ... Merge after #705". #705 is merged and the base is master with mergeable: MERGEABLE. Harmless mechanically but it reads as a live constraint to whoever merges.

Fixed. Callout removed.

What was verified (and found correct, unchanged)

  • No over-match in the widened regex. /<br[^>]*>/g cannot match a closing tag (</brand> starts < /), and does not match <b>, <bold>, <BR>, <Br>, <span>, <custom>, <q>. Malformed shapes (<br@>, <br="x">, <br/ >) arrive at headingId already escaped by marked, so the safety argument holds mechanically rather than by assertion.
  • The regression test is genuine. Reverting only the regex on the PR head makes not ok 20 - every raw tag whose name starts with 'br' yields a space fail while the other 22 tests pass. The 19 expected ids are pairwise distinct, so the de-dup counter cannot mask a divergence behind a -1 suffix.
  • Pre-existing limit, not actionable: [^>]* stops at the first >, so an attribute value containing > truncates the match. This is the same limit the generic tag strip already has, and the change moves the case toward pandoc, not away.
  • Gates, run in an isolated worktree at both the reviewed head and the fixed head: npm test 3935 pass / 0 fail / 1 skipped, npm run typecheck clean, node --test test/core/llp-ref-hygiene.test.js 11/11. Style checked: no U+2014, no semicolons added, no TS types or @typedef introduced.

Not verified

pandoc is not installed on this host, so every "matches pandoc 3.1.11" claim in the PR body, in issue #709, and in the new LLP consequence is taken on trust. What was checked is this renderer's own output for those shapes and its internal consistency, not parity against a real pandoc binary. A human who wants that parity independently confirmed should run the PR's case table against a real 3.1.11.


The head has moved to 653d5cf with the fixes above, so the next reconcile tick reviews that head as round 2.

Round 2 verified against marked 18.0.9 through the real renderReports path
that a literal U+FEFF survives inline parsing and reaches headingId intact
(`## K <BOM> L` renders `<h2 id="k-l">`), so its divergence comes from the
slug rule's own `\s` handling, not from marked's block parser. Only a
literal U+2028/U+2029 breaks upstream, splitting the block so no heading
is produced at all.

Reword the @ref gloss and the LLP 0208 #heading-id-gaps sub-bullet so the
BOM is no longer cited as evidence of upstream breakage, and record that a
slug-rule-only change would close the BOM case in full. Comment and doc
accuracy only; no executable code, regex, or test changes.

Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Review round 2 (head 653d5cf)

Verdict: findings. One minor, now fixed. Round 2 checked the round-1 fixes and found one of them still subtly wrong in the same way the original was.

Single-family review again (no codex on this host).

Finding

minor - src/core/reports/render.js:278-283 and llp/0208-…decision.md #heading-id-gaps: the BOM's divergence was misattributed to marked's block parser.

The round-1 rewording said the literal spellings of the separator/BOM class "break ABOVE this function, in marked's block parser, so fixing the rule below would close only half that class". True for U+2028 and U+2029. False for U+FEFF.

Measured through the real renderReports path on marked 18.0.9 (the pinned version), twice and independently, by the reviewer and again by the fixer before editing:

input result
literal U+2028 / U+2029 in a heading no <h2> at all - the block splits upstream, as documented
literal U+FEFF: ## K ␠BOM␠ L <h2 id="k-l">K  L</h2> - heading produced, BOM intact in the text
## A &#8232; B a---b
## A &#65279; B a---b

So the literal BOM is not stopped upstream: it reaches headingId and the weld is produced by this function's own rule, because JavaScript's \s matches U+FEFF and .replace(/\s/g, '-') hyphenates it. A slug-rule-only change treating U+FEFF as an ordinary symbol moves both BOM spellings to the pandoc-side value in one edit (entity a---b to a--b, literal k-l to k--l). For the BOM, "closes only half that class" is wrong: it closes the whole case.

No behavioural impact. The risk is precisely the one this PR exists to remove: a confident comment that would send a future maintainer chasing a dangling anchor away from the function actually producing it.

Fixed in f942c20. The gloss now splits the BOM out from the separators (BOM fully fixable here; literal U+2028/U+2029 genuinely upstream, no heading produced at all; image alt text needs token types this function only sees as rendered HTML). LLP 0208's #heading-id-gaps sub-bullet was corrected to match, including its parent clause, which had said "each is deferred because the fix does not live in the slug rule" - itself contradicted by the correction, since for the BOM the fix does live there. It now reads "neither is closed by a slug-rule change alone", true of both classes. The two documents agree.

Round-1 fixes: both verified sound

  • LLP settled-text revert. git diff origin/master -- llp/0208-… is a single hunk, 32 insertions, 0 deletions - additive only, no settled line removed or reworded, Decision paragraph byte-identical to master. The <a id="heading-id-gaps"></a> anchor exists and all three 0208# refs in the repo resolve. On whether the section is orphaned now that the Decision paragraph no longer links it: it is not. It is a top-level Consequences bullet with a bolded lead-in, structurally identical to the pre-existing #footnotes consequence, which is also anchored and also unlinked from the Decision. It follows the doc's own pattern, and additive-only is exactly what the extend-do-not-edit rule asks for.
  • Reworded @ref gloss. The entity-spelling half and the image-alt half were both verified true: ## X ![alt text](i.png) Y and ## X <img src="i.png" alt="alt text"> Y produce byte-identical marked output, so no string-level discriminator exists at this layer. Only the BOM clause was wrong, which is the finding above.
  • No behaviour change in the fix delta. Stripping comments from both revisions of render.js and diffing yields no difference: the delta is comment plus doc only. Confirmed again for 653d5cf..f942c20, which touches the same two files and no executable code.

Gates

npm test 3936 tests, 3935 pass, 0 fail, 1 skipped (pre-existing); npm run typecheck clean, exit 0. Both run in an isolated worktree at 653d5cf and again at f942c20. Style: no U+2014 anywhere in the diff, no semicolons added, no @typedef, no inline import('...') types, no TypeScript types.

Not re-litigated (settled in round 1, unchanged since): the /<br[^>]*>/g widening and its regression test.

Standing caveat for the human merging this

pandoc is not installed on this host. Every "against pandoc 3.1.11" figure in the code comment, in LLP 0208, in the PR body, and in the test expectations is taken on the author's stated measurement. What has been verified is this renderer's own output for those shapes and its internal consistency. If that parity claim is load-bearing for you, run the case table against a real pandoc 3.1.11 before merging. Note the finding above does not depend on it: it stands on the LLP's own assertion that pandoc drops the BOM as an ordinary symbol, plus the directly observed fact that a literal BOM reaches headingId.


Head is now f942c20. Review rounds are exhausted at the cap, so the next tick triages rather than opening round 3.

@philcunliffe

Copy link
Copy Markdown
Contributor Author

Triage (head f942c20): shippable, no follow-up issue filed

The review fix-loop used its 2 rounds. Triage re-read every finding from both rounds against the current tree (not against the review comments' own claims that they were fixed) and found none unresolved:

finding round state at f942c20
Edited the settled Decision paragraph of Active LLP 0208 1 resolved - diff vs master is 32 insertions, 0 deletions, Decision paragraph byte-identical to master
@ref gloss claimed neither deferred gap was fixable in headingId 1 resolved - reworded, no trace of the old claim
Stale "Merge after #705" note in the body 1 resolved - removed
BOM divergence misattributed to marked's block parser 2 resolved - gloss and LLP section both split the BOM out, and the underlying facts were re-verified independently against marked 18.0.9

No new production defect was found. Independently confirmed shippability:

  • The entire executable change vs master is one line. Stripping comments from the render.js diff leaves exactly -.replace(/<br(?:\s[^>]*)?\/?>/g, ' ') / +.replace(/<br[^>]*>/g, ' '). The other 145 diff lines are comment, the additive LLP section (32/0), and the new test file (86/0).
  • The regression test is genuine, re-checked a third time: reverting only the regex fails exactly one test (# 20 - every raw tag whose name starts with 'br' yields a space) with 22 passing; restored, 23/23.
  • Gates at this head: npm test 3935 pass / 0 fail / 1 skipped (pre-existing), npm run typecheck clean, node --test test/core/llp-ref-hygiene.test.js 11/11. No U+2014, no semicolons added.

Why no follow-up issue was opened

Triage normally defers non-blocking residue to a neutral:fix issue. There is no residue to defer here: every finding was fixed rather than waived, so an issue would have an empty body.

The two heading-id parity gaps named in LLP 0208 #heading-id-gaps (Unicode separators/BOM, and image alt text) are not deferred review findings, they are the deliberate subject of the decision this PR extends: 0208 records them as gaps left open on purpose. Filing them as neutral:fix would contradict a settled Active decision and send the issue-fix reconciler to churn against something the project chose not to change. They are documented in-repo, which is where LLP 0208 decided they should live.

One caveat for whoever merges

pandoc is not installed on this host, so every "matches pandoc 3.1.11" figure in the code comment, LLP 0208, the PR body's case table, and the test expectations is taken on the author's stated measurement and has not been checked against a real binary. Only the marked-side half of each claim was verified here.

Triage judged this a caveat rather than a blocker: if a parity figure were wrong, the failure mode is a wrong-but-stable slug in generated report anchors, self-consistent within this renderer, not a crash, data loss, or security issue; and the change strictly moves cases toward the claimed pandoc behaviour. But it is unverified, and this repo's own convention puts checks that need a real external binary in the human-run acceptance tier. If pandoc-anchor parity is load-bearing for you, run the PR body's case table against a real pandoc 3.1.11 before merging.

Nothing else is outstanding. The PR is being flipped out of draft and held for you to merge.

@philcunliffe philcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 11, 2026
@philcunliffe
philcunliffe marked this pull request as ready for review August 11, 2026 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up: deferred review findings from PR #705

2 participants