From 3c0272ac2232ee3a0edd0c259b71e9fd3384f211 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Tue, 2 Jun 2026 01:10:56 +0300 Subject: [PATCH] fix: round-trip fidelity for heading comments, literal tags, and emphasis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaced by `verify` on two real documents that aren't in the justfile set. - Heading-only HTML comment (a TOC macro marker, `

`) was dropped by xml2md — a CommentNode is neither text nor element, so it fell through walkChildren and vanished, collapsing `# ` to `#`. - Literal angle-bracket text stored as `<table>` (e.g. `box.backup.start()`) round-tripped into a phantom `
` element that swallowed the rest of the document: xml2md now backslash-escapes a tag-like `<` in both the TextNode and walkChildrenInline paths, and renderText unescapes only `\<` (never a blanket UnescapePunctuations, which would eat the deliberate `\|` table-pipe escaping). - ` x ` (whitespace inside the markers) rendered as literal `** x **`; writeEmphasis now moves the whitespace outside the markers. - normalizeForVerify canonicalizes values Markdown can't carry — task ids,
)`) decodes to `
` in the text node; emitted raw into Markdown, goldmark parses it as an HTML table and the phantom element swallows the rest of the document. `xml2md.escapeMarkdownAngle` backslash-escapes a *tag-like* `<` (followed by a letter, `/`, `!`, `?`) in **both** text paths — the `walk` TextNode branch *and* `walkChildrenInline` (used for `
  • ` bodies, a separate branch that's easy to miss). The matching unescape is in `confluence.Renderer.renderText`: it strips only `\<` (goldmark keeps a backslash-escaped run as one Text node with the backslash intact, and this renderer emits the raw segment) — **never** a blanket `util.UnescapePunctuations`, which would also eat the deliberate `\|` table-pipe escaping and re-break `base.xml`. A non-tag `<` (`a < b`, `<=`) is left alone. *Test:* `TestRoundTrip_LiteralAngleBracketText`. +- **Emphasis can't be flanked by whitespace** — ` x ` (space just inside the tag) renders as literal `** x **` because CommonMark requires the run to be flanked by non-whitespace. `xml2md.writeEmphasis` (used for strong/em/del) captures the children into a scratch buffer and moves leading/trailing whitespace *outside* the markers (` **x** `). The stored and round-tripped forms then differ only in space placement, which `normalizeForVerify` reconciles (`reEmphLeadWS`/`reEmphTrailWS`). *Test:* `TestRoundTrip_EmphasisInteriorSpaces`. -These two live in the formatter / verify layer rather than the converter, but bite the same way: +These live in the formatter / verify layer rather than the converter, but bite the same way: + +- **Some XML carries values Markdown has no slot for** — `` (Confluence renumbers them every save), `
      ` bullet styling, and a stray trailing nbsp before `` cannot survive a Markdown round-trip and aren't meaningful content. `normalizeForVerify` canonicalizes them on **both** sides (`reTaskID` zeroes the id, `reListStyleAttr` drops `
        /
          ` attributes, `reTrailWSTaskBody` strips the trailing space — note the whitespace class must include `\x{00a0}`, the actual byte stored). *Tests:* `TestNormalizeForVerify_TaskID`, `TestNormalizeForVerify_ListStyleAttr`, `TestNormalizeForVerify_TaskBodyTrailingWS`, `TestNormalizeForVerify_EmphasisWhitespace`. - **A "visible char" class in regexes must exclude `<`, not just whitespace** — `verify.normalizeForVerify`'s `reBareTextAfterHeading` wraps bare text between a heading and a following block in `

          …

          `. Its "≥1 visible char" token must be `[^\s<]`, never `\S`: `\S` matches `<`, so the capture swallows the next tag's opening bracket (e.g. ``) and the inserted `

          ` lands between a code macro's open tag and its body — destroying round-trip for a no-language fenced code block after a heading. *Tests:* `TestNormalizeForVerify_BareTextAfterHeading`, `TestVerifyRoundTrip_CodeBlockAfterHeading`. - **`fmt` line-wrapping must keep `…` whole** — breaking a line inside an inline code span injects the continuation indent into the span's text, which `xml2md` collapses into a spurious space *inside* the span (`` ` app.manifest.lock` ``). `format.splitAtoms` emits an entire `…` as one unbreakable atom. Significant whitespace between adjacent inline elements (` `) is carried on each atom's `spaceBefore` flag — never via `strings.Fields`, which drops trailing space. *Tests:* `TestPrettyXML_WrapKeepsCodeSpanWhole`, `TestPrettyXML_WrapPreservesSpaceBeforeCodeAfterWord`, `TestPrettyXML_SpaceBetweenInlineElements`. diff --git a/cmd/mdcx/verify.go b/cmd/mdcx/verify.go index 5965513e5d172a1faa2b7eab2d31de8b1af01401..8c8f025f13084acf52341854ed643350fc85d08e 100644 --- a/cmd/mdcx/verify.go +++ b/cmd/mdcx/verify.go @@ -130,6 +130,25 @@ var ( // (e.g. ) and wrap a structural element in

          ...

          — // which splits a code macro open from its body and destroys round-trip. reBareTextAfterHeading = regexp.MustCompile(`(?s)()([^<]*?[^\s<][^<]*?)(\s*<(?:table|ul|ol|h[1-6]|ac:|p|hr))`) + // reTaskID canonicalizes the numeric task id. Confluence assigns these ids + // itself and renumbers them on every save; Markdown has no slot to carry the + // original value, so the round-trip re-numbers from 1. The exact number is + // not meaningful content — zero it on both sides so verify ignores it. + reTaskID = regexp.MustCompile(`\d+`) + // reListStyleAttr strips presentational attributes (e.g. + // `style="list-style-type: square;"`) from
            /
              openers. Markdown has no + // way to carry list bullet styling, so the round-trip emits a bare
                /
                  . + reListStyleAttr = regexp.MustCompile(`<(ul|ol)\s+[^>]*>`) + // reTrailWSTaskBody strips trailing horizontal whitespace before a task-body + // close — the original often ends a task body with a stray space after the + // user link that no Markdown construct reproduces. + reTrailWSTaskBody = regexp.MustCompile(`[ \t\x{00a0}]+()`) + // reEmphLeadWS / reEmphTrailWS move whitespace that sits just inside an + // emphasis tag to just outside it, canonicalizing ` x ` + // (the stored form) and ` x ` (the round-trip form, since + // Markdown emphasis cannot be flanked by whitespace) to one shape. + reEmphLeadWS = regexp.MustCompile(`<(strong|em|del)>([ \t]+)`) + reEmphTrailWS = regexp.MustCompile(`([ \t]+)`) ) // normalizeForVerify strips XML patterns that cannot survive a round-trip @@ -165,6 +184,15 @@ func normalizeForVerify(xml string) string { xml = rePImageOnly.ReplaceAllString(xml, "$1") // Wrap bare text directly after headings in

                  ...

                  . xml = reBareTextAfterHeading.ReplaceAllString(xml, "$1

                  $2

                  $3") + // Canonicalize Confluence-assigned task ids — Markdown can't carry them. + xml = reTaskID.ReplaceAllString(xml, "0") + // Drop presentational attributes from
                    /
                      openers. + xml = reListStyleAttr.ReplaceAllString(xml, "<$1>") + // Strip trailing whitespace before a task-body close. + xml = reTrailWSTaskBody.ReplaceAllString(xml, "$1") + // Move whitespace from just inside an emphasis tag to just outside it. + xml = reEmphLeadWS.ReplaceAllString(xml, "$2<$1>") + xml = reEmphTrailWS.ReplaceAllString(xml, "$1") return xml } diff --git a/cmd/mdcx/verify_test.go b/cmd/mdcx/verify_test.go index 0aa9ecc18368af621af082ffdb46bab8c9aac0c4..35fd5d983ae0ace268cd3d37f407f6b72fbe6c45 100644 --- a/cmd/mdcx/verify_test.go +++ b/cmd/mdcx/verify_test.go @@ -171,6 +171,43 @@ func TestVerifyRoundTrip_CodeBlockAfterHeading(t *testing.T) { "code-block body must survive the round-trip") } +func TestNormalizeForVerify_TaskID(t *testing.T) { + // Confluence-assigned task ids are renumbered by the round-trip; verify must + // ignore the value by zeroing it on both sides. + in := `9complete` + want := `0complete` + assert.Equal(t, want, normalizeForVerify(in)) +} + +func TestNormalizeForVerify_ListStyleAttr(t *testing.T) { + // Markdown can't carry list bullet styling, so presentational attributes on + //
                        /
                          are stripped on both sides. + assert.Equal(t, `
                          • x
                          `, + normalizeForVerify(`
                          • x
                          `)) + assert.Equal(t, `
                          1. x
                          `, + normalizeForVerify(`
                          1. x
                          `)) + // A bare
                            is untouched. + assert.Equal(t, `
                            • x
                            `, normalizeForVerify(`
                            • x
                            `)) +} + +func TestNormalizeForVerify_TaskBodyTrailingWS(t *testing.T) { + // The stored form ends a task body with a stray nbsp after the user link that + // no Markdown construct reproduces; strip it (regular space and nbsp). + nbsp := " " + in := `` + nbsp + `` + want := `` + assert.Equal(t, want, normalizeForVerify(in)) +} + +func TestNormalizeForVerify_EmphasisWhitespace(t *testing.T) { + // The stored ` x ` and the round-trip ` x ` + // must canonicalize to the same shape (whitespace outside the tags). + stored := `a x b` + roundtrip := `a x b` + assert.Equal(t, normalizeForVerify(roundtrip), normalizeForVerify(stored)) + assert.Equal(t, `a x b`, normalizeForVerify(stored)) +} + // --- computeDiffOps --- func TestComputeDiffOps_IdenticalInputs(t *testing.T) { diff --git a/confluence/renderer.go b/confluence/renderer.go index 623aa9cb5eaf637ea511dffe5d74a495f1f54875..8d76646c87960b9b8b05f088f30c5fd36a7d9718 100644 --- a/confluence/renderer.go +++ b/confluence/renderer.go @@ -443,7 +443,15 @@ func (r *Renderer) renderText(w util.BufWriter, source []byte, node ast.Node, en return ast.WalkContinue, nil } n := node.(*ast.Text) - w.WriteString(html.EscapeString(string(n.Segment.Value(source)))) + // goldmark keeps a backslash-escaped run (e.g. `\
    • `) as a single Text + // node with the backslash still in the segment — its default writer strips + // escapes at render time, but this renderer emits the raw segment. We unescape + // only `\<`: that is the one escape xml2md adds (to stop literal angle-bracket + // text like `box.backup.start(
    )` being re-parsed as an HTML tag), and + // limiting it to `<` leaves the deliberate `\|` table-pipe escaping — which + // must survive verbatim — untouched. + value := strings.ReplaceAll(string(n.Segment.Value(source)), `\<`, `<`) + w.WriteString(html.EscapeString(value)) if n.HardLineBreak() { w.WriteString("
    ") } else if n.SoftLineBreak() { diff --git a/converter/md2xml_test.go b/converter/md2xml_test.go index 2648bc41b2d27fc65747c874aa19c2ac54dec59a..19bcd73e604edb5b84dcbf1daca02734ffcfa00f 100644 --- a/converter/md2xml_test.go +++ b/converter/md2xml_test.go @@ -622,6 +622,74 @@ func TestRoundTrip_PageLink(t *testing.T) { assert.Contains(t, xmlOutput, "Основной документ:") } +// TestConfluenceToMarkdown_CommentInHeading guards against the regression +// where an HTML comment that is the sole content of a heading — e.g. a TOC +// macro marker md2xml emits as `

    ` — +// was silently dropped by xml2md. A CommentNode is neither a TextNode nor an +// ElementNode, so it used to fall through to walkChildren (a comment has no +// children) and vanish, collapsing `# ` to a bare `#`. +func TestConfluenceToMarkdown_CommentInHeading(t *testing.T) { + xmlInput := `

    ` + + md, err := ConfluenceToMarkdown(xmlInput) + require.NoError(t, err) + assert.Contains(t, md, `# `) + + // And the comment survives a full markdown round-trip. + xmlOutput, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + mdBack, err := ConfluenceToMarkdown(xmlOutput) + require.NoError(t, err) + assert.Equal(t, strings.TrimSpace(md), strings.TrimSpace(mdBack)) +} + +// TestRoundTrip_LiteralAngleBracketText guards against the catastrophic +// regression where literal angle-bracket text stored as `<table>` (e.g. +// the function signature `box.backup.start(
    )`) round-tripped into a real +// `
    ` element: xml2md emitted a bare `
    `, goldmark parsed it as raw +// HTML, and the phantom table swallowed the rest of the document. xml2md now +// backslash-escapes a tag-like `<`, and renderText unescapes it back to `<`. +func TestRoundTrip_LiteralAngleBracketText(t *testing.T) { + for _, where := range []string{ + `

    call box.backup.start(<table>) now

    `, + ``, + } { + md, err := ConfluenceToMarkdown(where) + require.NoError(t, err) + // The '<' must be escaped so goldmark won't parse it as a table tag. + assert.Contains(t, md, `\
    `, "input %q", where) + + xmlOut, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + assert.Contains(t, xmlOut, "box.backup.start(<table>)", "input %q", where) + assert.NotContains(t, xmlOut, "
    ", "literal text must not become a table element") + } + + // A non-tag '<' (comparison operator) is left unescaped and still survives. + md, err := ConfluenceToMarkdown(`

    t <= time_end

    `) + require.NoError(t, err) + assert.NotContains(t, md, `\<`, "a non-tag '<' should not be escaped") + xmlOut, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + assert.Contains(t, xmlOut, "t <= time_end") +} + +// TestRoundTrip_EmphasisInteriorSpaces guards against ` x ` +// (whitespace just inside the tag) round-tripping to literal `** x **` asterisks +// — CommonMark does not treat a whitespace-flanked run as emphasis. xml2md now +// moves the spaces outside the markers (` **x** `). +func TestRoundTrip_EmphasisInteriorSpaces(t *testing.T) { + md, err := ConfluenceToMarkdown(`

    Вызов box.backup.start() будет

    `) + require.NoError(t, err) + assert.Contains(t, md, "**box.backup.start()**", "spaces must move outside the markers") + assert.NotContains(t, md, "** box.backup.start", "no whitespace just inside the markers") + + xmlOut, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + assert.Contains(t, xmlOut, "box.backup.start()") + assert.NotContains(t, xmlOut, "**", "asterisks must not leak as literal text") +} + // TestRoundTrip_NestedOrderedList guards against the regression where a // nested
      inside an outer
      1. was flattened into a single flat list // because the markdown was emitted with a 2-space indent (insufficient for diff --git a/converter/xml2md.go b/converter/xml2md.go index e20e41c10248557fb9ca8138a584b4b4da16b5ca..6b04522f46159d625bf1c4cd2942655f55dd0c16 100644 --- a/converter/xml2md.go +++ b/converter/xml2md.go @@ -115,6 +115,35 @@ func (c *xmlConverter) ensureSingleNewline() { } } +// writeEmphasis renders an inline emphasis element (strong/em/del) wrapping its +// children in `marker`, with any leading/trailing whitespace moved *outside* the +// markers. CommonMark does not recognize `** x **` as emphasis (the run must be +// flanked by non-whitespace), so ` x ` has to become ` **x** ` +// — otherwise the asterisks round-trip as literal text. An all-whitespace body +// is emitted verbatim with no markers. +func (c *xmlConverter) writeEmphasis(marker string, n *html.Node, depth int) { + saved := c.buf + c.buf = &bytes.Buffer{} + c.walkChildren(n, depth) + inner := c.buf.String() + c.buf = saved + + body := strings.TrimLeft(inner, " \t\r\n") + lead := inner[:len(inner)-len(body)] + core := strings.TrimRight(body, " \t\r\n") + trail := body[len(core):] + + if core == "" { + c.buf.WriteString(inner) + return + } + c.buf.WriteString(lead) + c.buf.WriteString(marker) + c.buf.WriteString(core) + c.buf.WriteString(marker) + c.buf.WriteString(trail) +} + func (c *xmlConverter) walkChildren(n *html.Node, depth int) { for child := n.FirstChild; child != nil; child = child.NextSibling { c.walk(child, depth) @@ -134,10 +163,27 @@ func (c *xmlConverter) walk(n *html.Node, depth int) { // but preserve the trimmed content text = collapseWhitespace(text) } + // Backslash-escape '<' so literal angle-bracket text (e.g. a function + // signature like `box.backup.start(
    )`, stored as `<table>`) + // is not re-parsed as an HTML tag by goldmark on the way back to XML — + // an unescaped `
    ` swallows the rest of the document into a phantom + // table. md2xml's escaper turns the resulting `<` text back into `<`, + // closing the round-trip. + text = escapeMarkdownAngle(text) c.buf.WriteString(text) return } + // Preserve literal HTML comments (e.g. a TOC macro marker that md2xml + // emitted inside a heading: `

    `). + // Without this, the comment node — being neither text nor element — falls + // through to walkChildren below and is silently dropped, collapsing + // `# ` to a bare `#` on round-trip. + if n.Type == html.CommentNode { + c.buf.WriteString("") + return + } + if n.Type != html.ElementNode { c.walkChildren(n, depth) return @@ -181,17 +227,11 @@ func (c *xmlConverter) walk(n *html.Node, depth int) { // Inline formatting case tag == "strong", tag == "b": - c.buf.WriteString("**") - c.walkChildren(n, depth) - c.buf.WriteString("**") + c.writeEmphasis("**", n, depth) case tag == "em", tag == "i": - c.buf.WriteString("*") - c.walkChildren(n, depth) - c.buf.WriteString("*") + c.writeEmphasis("*", n, depth) case tag == "del", tag == "s": - c.buf.WriteString("~~") - c.walkChildren(n, depth) - c.buf.WriteString("~~") + c.writeEmphasis("~~", n, depth) case tag == "code": if !isPrevSiblingCode(n) { c.buf.WriteString("`") @@ -781,7 +821,9 @@ func (c *xmlConverter) walkChildrenInline(n *html.Node, depth int) { text = strings.TrimRight(text, " ") } if text != "" { - c.buf.WriteString(text) + // Escape tag-like '<' so literal angle-bracket text in list items + // (e.g. `box.backup.start(
    )`) is not re-parsed as raw HTML. + c.buf.WriteString(escapeMarkdownAngle(text)) } continue } @@ -1207,6 +1249,34 @@ func collapseWhitespace(s string) string { return buf.String() } +// escapeMarkdownAngle backslash-escapes a '<' that would start an HTML tag +// (i.e. is followed by a letter, '/', '!' or '?') so goldmark keeps it as +// literal text instead of raw HTML — otherwise a `
    ` in body text +// swallows the rest of the document into a phantom table. The matching +// renderText unescape turns `\<` back into `<`, closing the round-trip. +// A lone '<' (as in `a < b` or `<=`) is left alone: goldmark already treats +// it as literal text, so escaping it would only add noise. +func escapeMarkdownAngle(s string) string { + if !strings.Contains(s, "<") { + return s + } + var buf strings.Builder + for i := 0; i < len(s); i++ { + if s[i] == '<' && (i == 0 || s[i-1] != '\\') && i+1 < len(s) && isTagNameStart(s[i+1]) { + buf.WriteString(`\<`) + continue + } + buf.WriteByte(s[i]) + } + return buf.String() +} + +// isTagNameStart reports whether b can follow '<' to begin an HTML tag, +// comment, or processing instruction that goldmark would parse as raw HTML. +func isTagNameStart(b byte) bool { + return (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') || b == '/' || b == '!' || b == '?' +} + // hasTaskStatus checks if a node contains a task-status element. func hasTaskStatus(n *html.Node) bool { for child := n.FirstChild; child != nil; child = child.NextSibling {