-
Notifications
You must be signed in to change notification settings - Fork 12
Skip decorative-bullet normalization inside fenced code blocks #5787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+206
β6
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
52318dd
Skip decorative-bullet normalization inside fenced code blocks
jurgenwerk 8341f99
Track fences the way marked parses them: CRLF, list-nested, indented β¦
jurgenwerk 5d743df
Drop indented-code skip: four spaces inside a list is content, not code
jurgenwerk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
extractCodeDatabyte-identical. Asserting substrings ofmarkdownToHtml'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 toresult.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 throughparseHtmlContent(packages/host/app/lib/formatted-message/utils.ts), and assertcodeData.searchReplaceBlockequals the source text β then parameterize it over the shapes: LF and CRLF, fence at top level and nested in a list item, bare```and```gtsopeners. 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.gtsalready has the rendering harness for this, andpackages/host/tests/unit/code-patching-test.tscoversparseSearchReplacein 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 inpackages/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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (
escapeHtmlOutsideCodeBlocksβmarkdownToHtmlβparseHtmlContentβcodeData.searchReplaceBlock) instead of a stand-in for it. Using the realSEARCH_MARKER/SEPARATOR_MARKER/REPLACE_MARKERconstants means it reads as the scenario it defends, androundTripSearchReplaceBlockis 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
searchReplaceBlockcontaining* π§there, sostrictEqualagainst 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;roundTripSearchReplaceBlockalready 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.