Skip to content

Skip decorative-bullet normalization inside fenced code blocks - #5787

Merged
jurgenwerk merged 3 commits into
mainfrom
decorative-bullets-skip-code-fences
Aug 20, 2026
Merged

Skip decorative-bullet normalization inside fenced code blocks#5787
jurgenwerk merged 3 commits into
mainfrom
decorative-bullets-skip-code-fences

Conversation

@jurgenwerk

@jurgenwerk jurgenwerk commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

There was a case where ai assistant code patches were failing over and over even though it re-read the file multiple times. Here's an example:

image

Claude's summary:

The AI assistant's code patches could fail forever with "search pattern not found" even though the search block matched the target file exactly. The cause is in markdownToHtml: it prefixes emoji/star-led lines with a * list marker so marked renders them as lists, and it applied that rewrite to the entire message — including the content of fenced code blocks. The host extracts search/replace patches from the rendered HTML, so a patch containing a line that starts with an emoji was mutated before matching:

bot message (markdown)                 file on disk
  ```gts                                 🚧 SITE UNDER ...
  ╔═══ SEARCH ════╗
  🚧 SITE UNDER ...
    │ markdownToHtml inserts "* " before 🚧
    ▼
  • 🚧 SITE UNDER ... ──match──✗──► 🚧 SITE UNDER ...

The bot re-reads the file and resends a correct patch, the renderer corrupts it again, and the loop never converges. The same rewrite also corrupted patches that did apply: REPLACE content with emoji-led lines was written to the file with stray `* ` markers injected.

The normalization now walks the markdown line by line with code-fence tracking (``` and ~~~ fences) and leaves fenced content verbatim; lines outside fences are normalized exactly as before.

markdownToHtml prefixes emoji/star-led lines with a list marker so marked
renders them as lists, but it applied that rewrite to the whole message,
including fenced code block content. Code patches are extracted from the
rendered HTML, so a search/replace block containing a line that starts
with an emoji was mutated before matching: the inserted marker made the
search pattern never match the target file, and applied patches wrote the
inserted marker into the file. The normalization now walks lines with
fence tracking and leaves fenced content verbatim.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files  ±0      1 suites  ±0   2h 25m 46s ⏱️ + 2m 20s
4 202 tests +8  4 188 ✅ +8  14 💤 ±0  0 ❌ ±0 
4 221 runs  +8  4 207 ✅ +8  14 💤 ±0  0 ❌ ±0 

Results for commit 8341f99. ± Comparison against earlier commit 52318dd.

Realm Server Test Results

1 files  ±    0  0 suites   - 1   0s ⏱️ - 18m 42s
0 tests  - 2 174  0 ✅  - 2 174  0 💤 ±0  0 ❌ ±0 
0 runs   - 2 254  0 ✅  - 2 254  0 💤 ±0  0 ❌ ±0 

Results for commit 5d743df. ± Comparison against earlier commit 8341f99.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a markdown rendering edge case where markdownToHtml normalized “decorative bullet” lines (emoji/star-led) inside fenced code blocks, which could corrupt search/replace patch text extracted from rendered HTML and cause patch application to fail or loop indefinitely.

Changes:

  • Reworked decorative-bullet normalization to be line-based with fenced-code-block tracking, leaving fenced content unchanged.
  • Updated the decorative bullet regex to operate on single lines (start-of-line) to support the new normalization approach.
  • Added unit tests to confirm normalization still produces list items outside fences and does not mutate fenced code content.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/runtime-common/marked-sync.ts Adds fence-aware normalization so decorative bullets are only prefixed outside fenced code blocks.
packages/host/tests/unit/marked-sync-test.ts Adds coverage for decorative bullet normalization behavior both outside and inside fenced code blocks.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jurgenwerk
jurgenwerk requested a review from a team August 14, 2026 10:27
@jurgenwerk
jurgenwerk marked this pull request as ready for review August 14, 2026 10:28

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 52318dda79

ℹ️ 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".

Comment thread packages/runtime-common/marked-sync.ts Outdated
/(^|\n)(\s*)([\p{Extended_Pictographic}★•▪●❖✦✧◉◦◾◽⬢⬡☑✔☑️➤➔➜➡→])(\s+)/gu;
/^(\s*)([\p{Extended_Pictographic}★•▪●❖✦✧◉◦◾◽⬢⬡☑✔☑️➤➔➜➡→])(\s+)/u;

const CODE_FENCE_PATTERN = /^(\s*)(`{3,}|~{3,})(.*)$/;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle fenced code blocks nested in list items

When Markdown nests a fenced block directly in a list item, such as - ```gts followed by 🚧 ..., Marked treats it as fenced code, but this pattern only recognizes fences preceded by whitespace. The code line is therefore rewritten to * 🚧 ..., and the closing fence is mistaken for a new opener, which both corrupts the fenced content and disables decorative-bullet normalization for the remainder of the message.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 8341f99: openers now also match with a leading list marker (- ```gts, 1. ```gts), while closers stay whitespace-only, so the closing fence can no longer be mistaken for an opener. Covered by the new list-nested-fence test, which also asserts normalization still applies after the fence closes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved, and I checked the parity half rather than just the content half. Rendering 'Steps:\n\n- ```gts\n 🚧 SITE UNDER CONSTRUCTION 🚧\n ```\n\n🌟 after\n' through marked 12.0.2 with this branch's normalization gives the fenced line verbatim inside the <pre> and <li>🌟 after</li> afterwards. On the previous commit the same input put * inside the <pre> and dropped the trailing bullet out of its list — so both the corruption and the state inversion it caused are gone.

Splitting openers from closers is the right call: keeping LIST_PREFIXED_CODE_FENCE_PATTERN out of the in-fence branch means a closer still has to be whitespace-only, which is what CommonMark requires and what stops a `- ```` line inside a block from closing it.

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] I reviewed this as a fence-detection correctness problem rather than a markdown-rendering one, because that's what the failure mode demands: the rewrite is cosmetic when it's wrong about prose and destructive when it's wrong about code, so the only question that matters is whether the tracker's idea of "inside a fence" can ever disagree with marked's. I ran every shape below against marked 12.0.2 (inside the workspace catalog's ^12.0.1) with the same code renderer this module registers, comparing normalized output and rendered HTML on main and on this branch.

Bottom line: the diagnosis is right and the approach is the right one, but the fence tracker has two holes that still let a corrupted patch through — one of which reproduces the original bug byte-for-byte. Both are fixable with a single edit to CODE_FENCE_PATTERN; I verified the replacement.

What lands right. Moving from a global regex to line-wise scanning with fence state is the correct shape — the rewrite has to happen before block lexing (turning a line into a list item is a block-level decision), so a pre-pass is unavoidable and it has to carry its own fence model. The closer test is more careful than it looks: the marker.length >= fenceLength clause is exactly what keeps a model patching a .md file from having its inner ``` close the outer ```` block, and the empty-info-string clause matches CommonMark's rule so a content line like ``` trailing doesn't end the block early. I verified both against marked and left a confirmation thread so a later simplification pass doesn't collapse them. Streaming also improves: a message cut mid-fence now protects everything after the opener, where main corrupted it.

On the Codex bot's finding — confirmed, and it understates the consequence. It's right that a fence opened on a list-marker line (- ```gts) is invisible to ^(\s*) and its content gets rewritten. What it missed is what the closing fence then does: it matches, inFence is still false, so it registers as an opener and the tracker is inverted against marked from there on. While inverted, the next real fence opener with no info string satisfies the closer test and switches normalization back on inside a genuine code block. I reproduced a message where that puts * into a SEARCH pattern in the rendered <pre>, identical to main's output. Detail and repro in the thread on normalizeDecorativeBullets.

Recommendations, most severe first:

  1. Blocking — allow a leading list marker on the fence pattern so opener and closer pair up; without it a list-nested patch still corrupts and the tracker's parity can invert. See the thread on normalizeDecorativeBullets (lines 91–106) for the repro and the verified replacement pattern.
  2. Blocking-adjacent, one character — the $ anchor makes the guard a total no-op on CRLF input, because JS treats \r as a line terminator that . won't match and $ won't skip. See the thread on CODE_FENCE_PATTERN. Bot messages are LF, so the patch path is safe today; markdown files rendered through the same helper are not.
  3. Add a round-trip test — render a real SEARCH/REPLACE block and assert extractCodeData returns it unchanged. Both findings above pass the current assertions. Thread on the new test in marked-sync-test.ts.
  4. Narrow the doc comment above normalizeDecorativeBullets: only fenced blocks are protected, and 4-space indented code blocks still get a marker injected. Thread on lines 79–83.
  5. Consider pinning the (improved) behavior change where a decorative bullet alone on a line is no longer promoted to a list item. Thread on DECORATIVE_BULLET_PATTERN.

If you'd rather close the whole class than keep patching the pattern, the durable version is to let marked locate the code: lex once over markdown.replace(/\r\n|\r/g, '\n') — matching what Lexer.lex does, so raw lengths map to source offsets — collect the ranges of code tokens, and rewrite only lines outside them. That is a bigger change than this PR needs; the pattern edit is a reasonable place to stop, and the round-trip test is what makes stopping there safe.

Adjacent, out of scope. escapeHtmlOutsideCodeBlocks in packages/runtime-common/helpers/html.ts runs immediately before this normalization on bot messages and carries a third, much cruder notion of "code block" — /`[\s\S]*?`/g, i.e. any backtick to the next backtick, with no fence awareness at all. It happens not to interact badly with this change (it only rewrites < and >), but that's now three independent models of where code starts and ends in one pipeline. Worth consolidating for whoever touches it next.


Generated by Claude Code

Comment thread packages/runtime-common/marked-sync.ts Outdated
/(^|\n)(\s*)([\p{Extended_Pictographic}★•▪●❖✦✧◉◦◾◽⬢⬡☑✔☑️➤➔➜➡→])(\s+)/gu;
/^(\s*)([\p{Extended_Pictographic}★•▪●❖✦✧◉◦◾◽⬢⬡☑✔☑️➤➔➜➡→])(\s+)/u;

const CODE_FENCE_PATTERN = /^(\s*)(`{3,}|~{3,})(.*)$/;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The $ anchor makes this guard a complete no-op on CRLF input — every fence line fails to match, inFence never becomes true, and the corruption this PR fixes comes back verbatim.

Mechanism. JS classifies \r (U+000D) as a line terminator, so . never matches it, and $ without the m flag anchors only to true end-of-string (JS has no Perl-style "also before a final newline"). markdown.split('\n') leaves the \r sitting at the end of every line, so on ```gts\r the (.*) group stops at gts and $ then fails with \r unconsumed. No backtracking rescues it.

Verified.

> /^(\s*)(`{3,}|~{3,})(.*)$/.test('```gts\r')   // false
> /^(\s*)(`{3,}|~{3,})(.*)$/.test('```gts')     // true

End to end, with ['```gts','file.gts','🚧 SITE UNDER CONSTRUCTION 🚧','```'].join('\r\n'):

<pre data-code-language="gts">file.gts
* 🚧 SITE UNDER CONSTRUCTION 🚧</pre>

versus the same content joined with \n:

<pre data-code-language="gts">file.gts
🚧 SITE UNDER CONSTRUCTION 🚧</pre>

Why it's silent. Lexer.lex in marked does src.replace(/\r\n|\r/g, '\n') before tokenizing, so marked parses CRLF markdown perfectly well — only this pre-pass is blind to it. Nothing downstream complains; the marker just lands in the <pre>.

The fix — tolerate the CR, and drop the .-vs-line-terminator dependency while you're in here:

const CODE_FENCE_PATTERN = /^(\s*)(`{3,}|~{3,})([^\r\n]*)\r?$/;

(See the thread on normalizeDecorativeBullets below — one pattern edit can carry this and the list-marker case together.)

Scope: regression against the PR's own contract, non-blocking on the reported path but a one-character fix. Bot messages arriving over Matrix are LF in practice, so the patch-corruption path is unlikely to hit this. markdownToHtml is also reached from packages/base/markdown-file-def.gts and packages/base/default-templates/markdown.gts with file contents, where a Windows-authored .md is entirely ordinary — and there the guard is simply off.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 8341f99: the fence patterns now end in (.*)\r?$ — the greedy group still stops before the CR (. cannot match it) and the explicit \r? lets $ anchor. Added a CRLF round-trip test that renders and re-extracts a patch from \r\n input.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved. (.*)\r?$ is the right shape and for the right reason — the greedy group still stops before the CR because . cannot match a line terminator, and the explicit \r? gives $ something to consume, so the pattern matches CRLF and LF identically without the m flag.

Verified end to end rather than at the regex: a patch authored with \r\n now round-trips through escapeHtmlOutsideCodeBlocksmarkdownToHtmlparseHtmlContent with the emoji-led search line unmutated, where the previous commit emitted * 🚧 into the <pre>. The new CRLF round-trip test covers exactly that.

One note on the closer path, which stays correct: closeMatch[3].trim() === '' tolerates the trailing \r because trim() strips it, so a CRLF closing fence still registers.

Comment thread packages/runtime-common/marked-sync.ts Outdated
Comment on lines +91 to +106
let fenceMatch = line.match(CODE_FENCE_PATTERN);
if (fenceMatch) {
let marker = fenceMatch[2];
if (!inFence) {
inFence = true;
fenceChar = marker[0];
fenceLength = marker.length;
} else if (
marker[0] === fenceChar &&
marker.length >= fenceLength &&
fenceMatch[3].trim() === ''
) {
inFence = false;
}
return line;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] inFence can invert relative to marked's own parse, and once inverted a genuine fenced patch gets normalized again — the exact corruption this PR exists to stop reproduces on this branch. The Codex bot flagged the first step of this; I verified it and it goes one step further than the bot described.

The mechanism, in three moves.

  1. CODE_FENCE_PATTERN anchors at ^(\s*), so a fence opened on the same line as a list marker — - ```gts or 1. ```gts — never matches. inFence stays false and the fenced content is rewritten. marked does treat that as a fenced code block, so parseHtmlContent hands it to extractCodeData as a patch.
  2. The closing fence of that same block (``` indented to the list content column) does match ^(\s*), and because inFence is still false it is read as an opener. The tracker's state is now inverted against marked's.
  3. While inverted, the next real fence opener carrying no info string (a bare ```) satisfies the closer test — same char, length ≥, empty info — and flips inFence back to false. Everything inside that code block is now normalized.

Verified. Move 1 alone, ['Steps:','','- ```gts',' 🚧 SITE UNDER CONSTRUCTION 🚧',' ```','','🌟 after'].join('\n') renders:

<ul>
<li><pre data-code-language="gts">* 🚧 SITE UNDER CONSTRUCTION 🚧</pre></li>
</ul>
<p>🌟 after</p>

The marker is inside the <pre> (patch corrupted), and 🌟 after is no longer a list item (move 2's side effect — normalization is off for the rest of the message).

Moves 1–3 together, with a bare-fence patch block after a list-nested one:

- ```gts
  const x = 1;
file.gts
🚧 SEARCH LINE

renders the patch block as:

```html
<pre data-code-language="">file.gts
* 🚧 SEARCH LINE</pre>

Byte-identical to what main produces. extractCodeData reads preElement.innerHTML verbatim into parseSearchReplace, so that * goes straight into the search pattern.

How much of this is new. The nested-fence content corruption (move 1) is pre-existing — main corrupts it too — so this is a hole in the fix rather than a fresh bug. What is new is move 2's side effect: after a phantom opener, decorative bullets silently stop rendering as lists for the remainder of the message.

The fix. Let the opener and its closer be seen as a pair by allowing a leading list marker; combined with the CR fix from the CODE_FENCE_PATTERN thread:

const CODE_FENCE_PATTERN =
  /^(\s*(?:(?:[-*+]|\d{1,9}[.)])\s+)?)(`{3,}|~{3,})([^\r\n]*)\r?$/;

I ran that against eight shapes: it turns all four failing ones green (list-nested fence, numbered-list-nested fence, the parity inversion above, CRLF) and leaves both new tests plus the nested-4-backtick and trailing-info-string cases byte-identical.

Residual, and the durable alternative. Hand-rolled fence detection will keep leaking: a line beginning with an inline triple-backtick span (```code``` is inline) still registers as a fence, CommonMark's 3-space indentation ceiling for fences is unmodelled, and so is "backtick fences may not carry a backtick in the info string". None of those is likely from a model writing patches, so the pattern edit is a reasonable place to stop. If you'd rather close the class, let marked decide instead of re-deriving it: lex once with bfmMarked.lexer(markdown.replace(/\r\n|\r/g, '\n')) — that's the same CR normalization Lexer.lex performs, so token raw lengths line up with source offsets — walk the block tokens recording the ranges of code tokens, and rewrite only lines outside them. Costs one extra lex pass and can never disagree with the parser that renders the result.

Scope: blocking. The current shape can still emit a corrupted patch, which is the failure mode this PR is meant to eliminate.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 8341f99. Openers and closers are now matched separately: openers additionally accept a leading list marker, closers require a same-char, at-least-as-long marker with nothing after it. The inversion case is covered by the new test asserting that content after a list-nested fence still normalizes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved — I reproduced all three moves on the previous commit and confirmed each is gone.

The opener is now seen (move 1), so the closer pairs with it instead of registering as a phantom opener (move 2), so the tracker can no longer be inverted and a later bare-fence block can no longer flip normalization back on inside real code (move 3). The three-shape input I used before — a list-nested fence followed by a bare-fence patch block — now renders the patch block's search line verbatim, where the previous commit produced output byte-identical to main.

Keeping the list-marker alternative on openers only is the detail that makes this hold: a closing fence in CommonMark carries neither an info string nor a list marker, so admitting one on the close would have re-opened the same class from the other side.

Comment thread packages/runtime-common/marked-sync.ts Outdated
Comment on lines +98 to +104
} else if (
marker[0] === fenceChar &&
marker.length >= fenceLength &&
fenceMatch[3].trim() === ''
) {
inFence = false;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Confirmation — this three-part closer test is correct and each clause is load-bearing. Recording why, because all three read like defensive noise and none of them is.

  • marker[0] === fenceChar stops a ~~~ line from closing a ``` block. CommonMark requires the closing fence to use the same character.
  • marker.length >= fenceLength is what makes nested fences work, and this is the case that matters most here: when the model patches a markdown file, it wraps the SEARCH/REPLACE block in ```` and the file content itself contains ```. Without the length comparison the inner fence would close the outer block and every subsequent line would be treated as prose.
  • fenceMatch[3].trim() === '' matches CommonMark's rule that a closing fence carries no info string. Without it, a content line like ``` trailing would end the block early.

Verified against marked 12.0.2 (inside the workspace catalog's ^12.0.1). The nested case — outer ````md, inner ```js around an emoji-led line — comes back with the inner fences intact and no marker injected:

<pre data-code-language="md">file.md
&lt;&lt;&lt;&lt;&lt;&lt;&lt; SEARCH
```js
🚧 x

...


And `` ``` trailing `` stays inside the block in both this code's tracking and marked's own parse, so the two agree.

Scope: confirmation, nothing to change. Worth keeping as-is if a later pass is tempted to collapse this into a plain `marker === fence` equality.

---
_Generated by [Claude Code](https://claude.ai/code)_

Comment thread packages/runtime-common/marked-sync.ts Outdated
Comment on lines +73 to +75
const DECORATIVE_BULLET_PATTERN =
// eslint-disable-next-line no-misleading-character-class -- match pictographic symbols plus a few geometric glyphs not covered by the Unicode class
/(^|\n)(\s*)([\p{Extended_Pictographic}★•▪●❖✦✧◉◦◾◽⬢⬡☑✔☑️➤➔➜➡→])(\s+)/gu;
/^(\s*)([\p{Extended_Pictographic}★•▪●❖✦✧◉◦◾◽⬢⬡☑✔☑️➤➔➜➡→])(\s+)/u;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Unflagged behavior change: a decorative bullet alone on its own line is no longer turned into a list item.

Mechanism. The old pattern was (^|\n)(\s*)(bullet)(\s+) run globally over the whole string, so \s+ could match the line's own trailing newline — a bare 🌟 line satisfied it. Now the subject is a single line with the newline already stripped by split('\n'), so (\s+) has nothing to match and the line falls through untouched.

Verified, '🌟\n\ntext':

rendered
before <ul>\n<li>🌟</li>\n</ul>\n<p>text</p>
after <p>🌟</p>\n<p>text</p>

The difference is bigger when a bare bullet leads into a real one — 'intro\n\n🌟\n🌟 second' used to collapse both lines into a single <li> via lazy continuation (<li>🌟\n🌟 second</li>), and now produces a paragraph followed by a one-item list.

Verdict: the new behavior is better — a lone emoji with no text after it isn't a bullet, and the lazy-continuation merge was surprising. Flagging it only because it looks incidental rather than intended, and nothing pins it: the next edit to this regex could restore the old behavior without a test noticing. Worth an assertion alongside the two new tests.

Scope: non-blocking; a test, not a code change.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Restored in 8341f99: the bullet pattern now accepts end-of-line after the bullet ((\s+|\r?$)), so a decorative bullet alone on its line becomes a list item again. Test added.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved, and the outcome is better than "restored" — worth recording, because a future reader diffing against main will find three behaviours here, not two.

(\s+|\r?$) brings back the bare-bullet-becomes-a-list-item case, and the new test pins it. What it does not bring back is the lazy-continuation merge, which is the half I'd called surprising. Measured on 'intro\n\n🌟\n🌟 second':

rendered
main <ul><li>🌟\n🌟 second</li></ul> — one item
this branch <ul><li>🌟</li><li>🌟 second</li></ul> — two

main merged them because the old global regex consumed the newline as part of its (\s+), leaving the following line's bullet past lastIndex and unmatched. Line-wise scanning gives each line its own shot at the pattern, so both normalize independently. That is the right answer and it comes for free from the restructuring rather than from the \r?$ alternation — so it isn't pinned by anything. If it's worth keeping, the two-<li> shape is one assertion alongside the bare-bullet test.

Comment on lines +79 to +83
// Prefix decorative bullets with a standard list marker so marked treats them
// as list items — but never inside fenced code blocks. Code-block content must
// survive rendering verbatim: search/replace patches are extracted back out of
// the rendered HTML, and an inserted marker makes the patch text no longer
// match the file it targets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The comment promises a broader invariant than the code delivers. "Code-block content must survive rendering verbatim" reads as covering all code blocks; only fenced ones are protected. A 4-space indented code block is still rewritten.

Verified, 'Example:\n\n 🚧 SITE UNDER CONSTRUCTION 🚧\n\n🌟 tail':

<p>Example:</p>
<pre data-code-language="">* 🚧 SITE UNDER CONSTRUCTION 🚧</pre>

That matters because the extraction side doesn't distinguish the two: parseHtmlContent in packages/host/app/lib/formatted-message/utils.ts walks top-level children and routes every <pre> through extractCodeData, which reads innerHTML straight into parseSearchReplace. An indented code block is a <pre> like any other, so a patch expressed that way corrupts exactly as before.

How much this matters: little, in practice. The source-code-editing skill teaches fenced blocks with a language on every example, and extractCodeData keys the file URL off the first line, so an indented patch is off the taught path. Not asking for it in this PR.

The ask is the wording — so the next reader doesn't build on an invariant that isn't there. Something like:

// ...but never inside fenced code blocks. Fenced content must survive
// rendering verbatim: search/replace patches are extracted back out of the
// rendered HTML, and an inserted marker makes the patch text no longer match
// the file it targets. Indented (4-space) code blocks are not tracked; the
// patch format is always fenced.

Scope: follow-up for the indented-block gap, comment accuracy for this PR. Non-blocking.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 8341f99 by making the code deliver what the comment promised: lines with 4-space (or tab) indentation outside a fence are now left verbatim, since extraction treats an indented code block's

 exactly like a fenced one. The asymmetry you describe (cosmetic when skipped wrongly, destructive when applied wrongly) is now recorded next to the pattern.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The comment now matches the code, and I verified the code does what it says for the case this thread was about: 'Example:\n\n 🚧 SITE UNDER CONSTRUCTION 🚧\n\n🌟 tail' renders <pre><code>🚧 SITE UNDER CONSTRUCTION 🚧</code></pre> where main produced <pre><code>* 🚧 …</code></pre>. The <pre> that parseHtmlContent would have routed through extractCodeData is clean.

But this went past what the thread asked for, and it took something with it. The ask here was the wording — I said explicitly this wasn't being requested for this PR — because closing the gap properly needs block context that a per-line test can't have. INDENTED_CODE_PATTERN tests the raw line, so it cannot tell four spaces of code from four spaces of list content: a decorative bullet nested four spaces or one tab under a real list item now collapses into the parent item's text instead of rendering as a sub-list. Measured table and both ways out are in the new thread on that constant.

The one-line-fix framing still stands, just in the other direction: dropping the pattern and keeping this comment change is a deletion, and it leaves every other fix in 8341f9962b intact.

Comment on lines +361 to +384
test('markdownToHtml leaves fenced code block content verbatim when lines start with decorative bullets', function (assert) {
const markdown = [
'🌟 A real list item',
'```gts',
" <span class='marquee-text'>",
' 🚧 SITE UNDER CONSTRUCTION 🚧',
' </span>',
'```',
].join('\n');
const result = markdownToHtml(markdown, { sanitize: false });

assert.true(
result.includes('<li>🌟 A real list item</li>'),
'bullet normalization still applies outside the fence',
);
assert.true(
result.includes(' 🚧 SITE UNDER CONSTRUCTION 🚧'),
'emoji-led line inside the fence is unchanged',
);
assert.false(
result.includes('* 🚧'),
'no list marker is inserted inside the fence',
);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] This test asserts the shape of the rendered HTML, one step away from the contract that actually broke — and both blocking findings in this review pass it.

The contract is a round trip: a patch the model puts inside a fence has to come back out of extractCodeData byte-identical. Asserting substrings of markdownToHtml's output can hold while the round trip fails, and here it does — neither the CRLF case nor the list-nested-fence case is visible to result.includes(' 🚧 …') / assert.false(result.includes('* 🚧')), because both change the input shape, not the assertions' subject.

The test that would have caught them. Render a real SEARCH/REPLACE block through markdownToHtml, push the result through parseHtmlContent (packages/host/app/lib/formatted-message/utils.ts), and assert codeData.searchReplaceBlock equals the source text — then parameterize it over the shapes: LF and CRLF, fence at top level and nested in a list item, bare ``` and ```gts openers. Use the real markers from @cardstack/runtime-common (SEARCH_MARKER / SEPARATOR_MARKER / REPLACE_MARKER) rather than emoji-only prose, so the test reads as the scenario it defends.

packages/host/tests/integration/components/formatted-aibot-message-test.gts already has the rendering harness for this, and packages/host/tests/unit/code-patching-test.ts covers parseSearchReplace in isolation. Nothing covers the seam between them, which is precisely where this bug lives — a renderer-side rewrite that only manifests as a parser-side mismatch.

On the first new test (prefixes decorative bullets with a list marker): the same behavior is already asserted at the integration level in packages/host/tests/integration/components/ai-assistant-panel/general-test.gts ("it converts decorative star bullets into markdown lists" and the multi-bullet variant). Keeping a unit-level pin is fine and cheaper to run — just noting it isn't new ground, so the round-trip test is where the marginal coverage is.

Scope: test coverage. Non-blocking on its own, but it is what turns the two blocking threads into something a future edit can't silently undo.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Added in 8341f99: a round-trip test now renders a fenced search/replace block through the same pipeline as message bodyHTML and re-extracts it with parseHtmlContent, asserting byte-identity with what was authored — plus a CRLF variant. Both blocking findings fail this test on the previous commit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Resolved, and it is the right test — it asserts the contract rather than the rendered shape, and it drives the real pipeline (escapeHtmlOutsideCodeBlocksmarkdownToHtmlparseHtmlContentcodeData.searchReplaceBlock) instead of a stand-in for it. Using the real SEARCH_MARKER / SEPARATOR_MARKER / REPLACE_MARKER constants means it reads as the scenario it defends, and roundTripSearchReplaceBlock is reusable for the next shape that needs pinning.

I confirmed the claim that it fails on the previous commit: both the CRLF input and the list-nested fence produce a searchReplaceBlock containing * 🚧 there, so strictEqual against the authored block fails on each.

Two shapes from my earlier list are still unparameterized — a fence nested in a list item, and a bare ``` opener — both of which are round-trippable through the same helper and both of which were failing a commit ago. They are covered indirectly by the HTML-shape test, which is weaker for exactly the reason this thread was about. Non-blocking; roundTripSearchReplaceBlock already makes them one case each.

Separately: the nested-decorative-bullet regression described in the new thread is invisible to every test in this file, which is the same gap in a different place — the suite pins what the normalization skips but not what it must still convert.

…code

The fence tracker missed three shapes marked itself treats as code, so the
decorative-bullet rewrite could still corrupt patch content. `.` never
matches a trailing carriage return, so on CRLF input no fence line matched
at all; a fence opened on a list-marker line (`- ```gts`) went unrecognized
and its closer inverted the tracker's state; and 4-space indented code
blocks were rewritten even though extraction treats their <pre> exactly
like a fenced one. Fence lines now tolerate a trailing CR, openers accept a
leading list marker (closers cannot carry one), and indented lines are left
verbatim.

The line-based pattern also dropped the old whole-string behavior of
normalizing a decorative bullet alone on its line — restored by accepting
end-of-line after the bullet.

Tests now exercise the contract that actually broke: a search/replace
block must round-trip byte-identical through render (bodyHTML) and
extraction (parseHtmlContent), including from CRLF input.

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

@richardhjtan richardhjtan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Re-reviewed 8341f9962b as a set of changes rather than as compliance: for each thread, does the new code fix the mechanism that thread named, and does it bring a cost of its own. I re-ran every shape below against marked 12.0.2 with each branch's normalization, comparing rendered HTML on main and here.

Bottom line: both blocking findings are genuinely fixed — I reproduced each failure on the previous commit and confirmed it is gone, including the fence-parity inversion. The round-trip test is the right test and it does fail on the old code. One new regression came in with the indented-code change, which was the one item the thread explicitly said wasn't being asked for: a decorative bullet nested four spaces or a tab under a real list item no longer renders as a sub-list. That is a deletion away from resolved.

Verified fixed

  • List-nested fences. '- ```gts\n 🚧 …\n ```\n\n🌟 after' now renders the fenced line verbatim and still normalizes 🌟 after into a list item — so the opener is seen, the closer pairs with it, and the parity inversion that could switch normalization back on inside a later code block cannot start.
  • CRLF. A patch authored with \r\n round-trips with the emoji-led search line unmutated. (.*)\r?$ is the right shape: the greedy group still stops before the CR because . cannot match it, and the explicit \r? gives $ something to anchor past.
  • Genuine indented code blocks. 'Example:\n\n 🚧 …' renders <pre><code>🚧 …</code></pre> where main produced <pre><code>* 🚧 …</code></pre>.
  • The round-trip test earns its place. Driving escapeHtmlOutsideCodeBlocksmarkdownToHtmlparseHtmlContentcodeData.searchReplaceBlock and asserting byte-identity is the contract that actually broke, and it covers the seam that neither the marked-sync unit tests nor code-patching-test.ts reached before.

One thing that came out better than described

The bare-bullet reply says the behavior was "restored". It is restored and improved, which is worth knowing because a future reader diffing against main will see three outcomes rather than two. On 'intro\n\n🌟\n🌟 second', main renders a single <li>🌟\n🌟 second</li> — the two bullets merged by lazy continuation, because the old global regex consumed the newline and left the second line unmatched. This branch renders two separate <li>s. That is the right answer and it is the one the earlier thread called surprising about main; line-wise scanning fixed it as a side effect.

The remaining item

The indented-code skip closes a real gap and opens a more common one. Detail, the measured table across nesting widths, and two ways out are in the new thread on INDENTED_CODE_PATTERN. My recommendation is to drop that pattern and take the comment-only change the original thread asked for, keeping everything else in this commit — and, either way, to pin the nested-bullet case next to the new indented-block test so the boundary is stated rather than implied.

Still open from before, unchanged

The three independent notions of "where code starts and ends" in this pipeline — escapeHtmlOutsideCodeBlocks's backtick-to-backtick regex, this module's fence tracker, and marked's own lexer — are now four patterns across two files. Letting marked locate the code (lex once, collect code token ranges, rewrite only outside them) would collapse the tracker's three patterns into none and would have prevented both the fence-parity bug and the nested-bullet one. That remains a follow-up, not this PR.


Generated by Claude Code

Comment thread packages/runtime-common/marked-sync.ts Outdated
Comment on lines +91 to +96
// A 4-space (or tab) indented line outside a fence is an indented code block
// to marked, and the extraction side treats its <pre> exactly like a fenced
// one — so rewriting it corrupts code the same way. The rewrite is cosmetic
// when skipped wrongly and destructive when applied wrongly, so indented
// lines are left alone.
const INDENTED_CODE_PATTERN = /^(?: {4}|\t)/;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] New finding — regression introduced by this commit. A decorative bullet nested four spaces (or one tab) under a real list item no longer renders as a sub-list; it collapses into the parent item's text. This is the one change in 8341f9962b that costs more than it buys, and no test in the suite sees it.

The mechanism. In CommonMark, four spaces means "indented code block" only relative to the containing block, and an indented code block cannot interrupt a paragraph. Inside a list item, the content column is already indented, so four spaces there is ordinary list content — a nested bullet, not code. INDENTED_CODE_PATTERN tests the raw line with no notion of either context, so it swallows both cases.

Measured against marked 12.0.2, the same version the workspace catalog resolves, rendering '- item one\n<indent>🌟 nested point\n' through each branch's normalization:

nesting main this branch
2 spaces <li>item one<ul><li>🌟 nested point</li></ul></li> identical
3 spaces <li>item one<ul><li>🌟 nested point</li></ul></li> identical
4 spaces <li>item one<ul><li>🌟 nested point</li></ul></li> <li>item one\n 🌟 nested point</li>
tab <li>item one<ul><li>🌟 nested point</li></ul></li> <li>item one\n 🌟 nested point</li>

So the nested list is gone at exactly the two indentation widths a model is most likely to use for a second level. The whole point of this normalization is that assistants write emoji bullets instead of markdown ones; a nested emoji bullet under a real list item is an ordinary shape for them to produce.

Two other cases I measured, for completeness — both of these are improvements and worth keeping:

  • A genuine indented code block ('Example:\n\n 🚧 …\n\ntail') now renders <pre><code>🚧 …</code></pre> instead of main's <pre><code>* 🚧 …</code></pre>. That is the case this pattern was added for, and it works.
  • A four-space-indented bullet continuing a paragraph ('Intro line:\n 🌟 point') rendered on main as <p>Intro line:\n * 🌟 point</p> — a literal * visible in the prose, because a list can't interrupt a paragraph there. That cosmetic bug is now gone too.

What I'd do. The thread this came from asked for a comment change and said explicitly that the indented-block gap wasn't being asked for in this PR. That framing still holds, and the reason is visible here: distinguishing "four spaces of code" from "four spaces of list content" needs the block context that only the lexer has. Two ways forward:

  1. Drop INDENTED_CODE_PATTERN and narrow the comment as originally suggested — fenced blocks are protected, indented ones are not, and the patch format is always fenced. That restores nested bullets and keeps every other fix in this commit. The cost is the * in an indented code block, which is pre-existing, off the taught path, and not what the reported failure was.
  2. Let marked locate the code, which is the durable version and now has a second reason to exist: lex once over markdown.replace(/\r\n|\r/g, '\n'), collect the source ranges of code tokens, and rewrite only lines outside them. That gets list context, paragraph interruption, CRLF, and fence pairing for free, and would let all three hand-rolled patterns here go away.

I'd take (1) in this PR and file (2), since (1) is a deletion and the rest of this commit is solid.

Whichever way it goes, pin it. '- item one\n 🌟 nested point' asserting a nested <ul> is the assertion that would have caught this, and it belongs next to the new indented code blocks are left verbatim test — the two together state where the boundary actually is.

Scope: regression, introduced here. Blocking in the sense that I would not merge it as a silent rendering change; trivially resolved by (1).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Fixed in 5d743df by taking option (1): INDENTED_CODE_PATTERN is deleted, so a decorative bullet indented four spaces (or a tab) under a real list item nests as a sub-list again. The doc comment now states the boundary explicitly: only fenced blocks are protected, and the reason is recorded — telling "four spaces of code" from "four spaces of list content" needs block context that only the lexer has, and the patch format is always fenced.

The pinning test you asked for is in: - item one\n 🌟 nested point must render a nested <ul>. It replaces the indented code blocks are left verbatim test, which asserted the behavior this deletion removes. I verified against marked 12.0.2 that the nested case renders identically to main at both 4-space and tab indentation, and that the indented-code case returns to main's pre-existing * 🚧 output. All 35 marked-sync unit tests pass.

Inside a list item the content column is already indented, so a
decorative bullet nested four spaces (or a tab) under a real list item
is a nested bullet, not an indented code block. The line-based pattern
had no block context and swallowed both, collapsing nested emoji
bullets into the parent item's text. Fenced blocks remain protected;
the patch format is always fenced, so indented code blocks are left to
the rewrite as before.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jurgenwerk
jurgenwerk merged commit cd3a7f0 into main Aug 20, 2026
31 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants