From 442bf5982d1e46a2737804b2f30a9b4895e9ac02 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Fri, 8 May 2026 04:12:09 +0300 Subject: [PATCH] fix: round-trip fidelity for tables, page links, and nested lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four real-document failures surfaced via `mdcx verify`: - Pipes inside `` in GFM table cells were treated as column separators, splitting cells. `writeTableRow` now escapes `|` as `\|` per the GFM spec; the parser strips the backslash before inline parsing so the pipe survives inside the resulting code span. - Two `` elements separated by whitespace text were being merged because `isPrev/NextSiblingCode` skipped whitespace nodes. Adjacency is now strict. - `` was dropped because goldmark cannot parse `` as raw inline HTML (the colon disqualifies it). Preserved via a `` placeholder; the inverse is driven by a new `pageLinkDepth` counter in the renderer. - Nested `
    ` collapsed to a flat list and went loose because the fixed `2*(depth-1)` indent didn't satisfy CommonMark's column rule for ordered markers and a stray `\n` after each `
  1. ` made the list loose. Indent is now a per-level stack (3 for ol, 2 for ul/task) and list-item terminators use `ensureSingleNewline`. Regression tests in converter/md2xml_test.go pin each fix; CLAUDE.md documents the gotchas. --- CLAUDE.md | 16 ++- confluence/renderer.go | 53 +++++++++- converter/md2xml_test.go | 159 ++++++++++++++++++++++++++++++ converter/xml2md.go | 207 ++++++++++++++++++++++++++++++++------- 4 files changed, 396 insertions(+), 39 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9f37227b5a2ef192887b8c79b97cff8a3a7ebc0e..d42ea2f268df4d90c968f3debb0dce1707e386b5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,7 +40,7 @@ confluence/ **CDATA preprocessing** — `xml2md.go` replaces `` with `` fake elements before parsing, because `golang.org/x/net/html` doesn't handle CDATA. The reverse direction uses `confluence.escapeCDATA()` to split `]]>` sequences. -**Renderer state** — `confluence.Renderer` tracks `taskIDCounter`, `inTaskBody`, `inlineCommentDepth`, and pending attributes from HTML comments. This state is per-conversion and must remain consistent across the AST walk. +**Renderer state** — `confluence.Renderer` tracks `taskIDCounter`, `inTaskBody`, `inlineCommentDepth`, `pageLinkDepth`, and pending attributes from HTML comments. This state is per-conversion and must remain consistent across the AST walk. **Template markers** — `` / `` delimit the editable region in Confluence pages. The `push --template` flow: fetch page → replace between markers → update with version increment. @@ -52,3 +52,17 @@ confluence/ - Task lists detected by checkbox presence, rendered as `` - Code blocks → `` with CDATA body - XML namespaces (`ac:`, `ri:`) are string-constructed, not from an XML library + +### Round-trip gotchas (with regression tests) + +These are subtle traps the round-trip has been bitten by; touching the related code without updating the tests in `converter/md2xml_test.go` will likely re-break a real document. Each item names the test that pins it. + +- **GFM tables and `|` inside ``** — a literal `|` in a cell is a column separator, even inside backticks. `xml2md.writeTableRow` runs every cell through `escapeTablePipes` which writes `\|`; the GFM parser strips the backslash at table-parse time *before* inline parsing, so the pipe survives inside the resulting code span. *Tests:* `TestRoundTrip_TablePipeInCode`, `TestEscapeTablePipes_NoDoubleEscape`. +- **Adjacent `` merge must not span whitespace** — `ab` (no gap) merges into one code span on the way to MD; `a b` (whitespace text node between) must stay as two. `isPrevSiblingCode` / `isNextSiblingCode` therefore look at the *immediate* neighbour only and refuse to skip whitespace text nodes. *Test:* `TestConfluenceToMarkdown_AdjacentCodeMerge` (case "whitespace-separated …"). +- **Page links have no markdown form** — `` is preserved as a `` placeholder. Goldmark cannot parse `` as raw inline HTML because of the `:` in the tag name, so a span wrapper is required. The reverse path lives in `confluence.Renderer.convertRawSpan`; closing `` consults `pageLinkDepth` to emit `` at the right point. *Test:* `TestRoundTrip_PageLink`. +- **Nested-list indent depends on the *parent* list type** — CommonMark places content of `1. ` at column 4 and `- ` at column 3, so a nested item must be indented by ≥ the parent marker's width. `xmlConverter.listIndents` is a stack of per-level widths (3 for `
      `, 2 for `
        ` / task-list); `itemIndent()` sums all *ancestor* contributions. Using a fixed `2*(depth-1)` collapses nested ordered lists into siblings. *Tests:* `TestRoundTrip_NestedOrderedList`, `TestRoundTrip_NestedMixedList`, `TestRoundTrip_DoubleNestedUnorderedList`. +- **Tight lists are one stray `\n` away from loose** — a blank line anywhere in a list turns it loose, which makes goldmark wrap every `
      • ` in `

        `. `ensureSingleNewline()` writes `\n` only when the buffer doesn't already end in one, preventing the double newline that appeared when a `

      • `'s body ended with a nested list (which itself terminated in `\n`). Always use it instead of `c.buf.WriteString("\n")` after walking list-item content. + +### Verifying real documents + +`mdcx verify ` formats the input, round-trips it through `xml2md → md2xml`, formats the output, and diffs them. The justfile in `~/data/1` runs this against five real Confluence pages — exercise it after any change to converter or renderer code, since the unit tests can miss multi-feature interactions (e.g. a task list inside a `
        ` inside a table cell). diff --git a/confluence/renderer.go b/confluence/renderer.go index 46be42a849e33cecf2b4842a552cd0e6423da1ba..623aa9cb5eaf637ea511dffe5d74a495f1f54875 100644 --- a/confluence/renderer.go +++ b/confluence/renderer.go @@ -17,6 +17,7 @@ type Renderer struct { taskIDCounter int inTaskBody bool inlineCommentDepth int + pageLinkDepth int pendingTableAttrs string // stored from comment pendingCodeMacroID string // stored from comment pendingCodeAttrOrder string // stored from comment @@ -535,9 +536,17 @@ func (r *Renderer) renderRawHTML(w util.BufWriter, source []byte, node ast.Node, func (r *Renderer) convertRawSpan(raw string) string { if !strings.HasPrefix(raw, "" && r.inlineCommentDepth > 0 { - r.inlineCommentDepth-- - return "" + // or page link. Page links nest inside inline comments rarely, so the + // page-link counter takes precedence when both could apply. + if raw == "" { + if r.pageLinkDepth > 0 { + r.pageLinkDepth-- + return "" + } + if r.inlineCommentDepth > 0 { + r.inlineCommentDepth-- + return "" + } } return raw } @@ -571,6 +580,44 @@ func (r *Renderer) convertRawSpan(raw string) string { } } + // body + // or self-closing form when the original ac:link had no anchor body. + if strings.Contains(raw, "data-page-link=") { + space := extractAttrValue(raw, "data-space-key") + title := extractAttrValue(raw, "data-content-title") + anchor := extractAttrValue(raw, "data-anchor") + var b strings.Builder + b.WriteString(``) + // Self-closing span emits both opener and closer in convertRawSpan; for + // `` the renderer never sees a separate closing tag, so wrap + // up the ac:link here and signal the inline-span tracker not to expect one. + if strings.HasSuffix(raw, "/>") { + b.WriteString(``) + return b.String() + } + // For `body`, defer the emission to the + // matching . + r.pageLinkDepth++ + return b.String() + } + return raw } diff --git a/converter/md2xml_test.go b/converter/md2xml_test.go index 17254cb180e1344d8d8d908d45c45619f549f67c..2648bc41b2d27fc65747c874aa19c2ac54dec59a 100644 --- a/converter/md2xml_test.go +++ b/converter/md2xml_test.go @@ -515,6 +515,14 @@ func TestConfluenceToMarkdown_AdjacentCodeMerge(t *testing.T) { input: `

        a text b

        `, expected: "`a` text `b`", }, + { + // Regression: whitespace-only text between two elements + // previously caused the merge logic to swallow the gap and + // concatenate the spans. + name: "whitespace-separated code elements stay separate", + input: `

        Благодаря previous_backup_id tt restore plan видит разрыв.

        `, + expected: "`previous_backup_id` `tt restore plan`", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -552,3 +560,154 @@ func TestRoundTrip_InlineCommentFromRealXML(t *testing.T) { assert.Contains(t, xmlOutput, `ac:ref="b2f6ce98-4dc9-45e0-a9b6-b4a5109657ca"`) assert.Contains(t, xmlOutput, "Не майся дурью") } + +// TestRoundTrip_TablePipeInCode locks in the fix for table cells whose +// contents include a literal `|`. Without escaping, the pipe was treated as a +// column separator at re-parse time, splitting a single cell across columns +// and dropping content. Cells must escape `|` as `\|` per the GFM spec. +func TestRoundTrip_TablePipeInCode(t *testing.T) { + xmlInput := `` + + `` + + `` + + `` + + `` + + `

        Команда

        Где

        Действие

        mode --target=<incremental|full> --storage=<config>

        менеджер

        Возвращает mode + from_vclock.

        ` + + md, err := ConfluenceToMarkdown(xmlInput) + require.NoError(t, err) + // The pipe inside the code span must be escaped in the markdown so the + // table parser does not treat it as a column boundary. + assert.Contains(t, md, `\|`) + + xmlOutput, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + // All three columns must survive the round-trip. + assert.Contains(t, xmlOutput, `mode --target=<incremental|full> --storage=<config>`) + assert.Contains(t, xmlOutput, "менеджер") + assert.Contains(t, xmlOutput, "from_vclock") +} + +// TestEscapeTablePipes_NoDoubleEscape guards the helper against doubling up +// already-escaped pipes (e.g. when a cell's rendered markdown was produced by +// an upstream that already escaped some pipes itself). +func TestEscapeTablePipes_NoDoubleEscape(t *testing.T) { + cases := map[string]string{ + "": "", + "plain": "plain", + "a|b": `a\|b`, + `already\|done`: `already\|done`, + "`a|b`": "`a\\|b`", + } + for in, want := range cases { + got := escapeTablePipes(in) + assert.Equal(t, want, got, "input=%q", in) + } +} + +// TestRoundTrip_PageLink locks in the fix for . +// These page references have no plain markdown equivalent and previously got +// dropped on the way to MD, leaving an empty paragraph after a round-trip. +func TestRoundTrip_PageLink(t *testing.T) { + xmlInput := `

        Основной документ:

        ` + + md, err := ConfluenceToMarkdown(xmlInput) + require.NoError(t, err) + require.Contains(t, md, `data-page-link`) + require.Contains(t, md, `data-space-key="TSC"`) + require.Contains(t, md, `data-content-title="Бекапы и Point-in-time recovery в кластере Tarantool"`) + + xmlOutput, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + assert.Contains(t, xmlOutput, ``) + assert.Contains(t, xmlOutput, "Основной документ:") +} + +// 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 +// ordered lists, which need ≥3) and a blank line that turned the list loose. +func TestRoundTrip_NestedOrderedList(t *testing.T) { + xmlInput := `
              ` + + `
            1. Outer one.
                ` + + `
              1. Inner one.
                2. ` + + `
              2. Inner two.
                3. ` + + `
              2. ` + + `
            2. Outer two.
              3. ` + + `
            3. Outer three.
              4. ` + + `
            ` + + md, err := ConfluenceToMarkdown(xmlInput) + require.NoError(t, err) + + xmlOutput, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + + // Nested
              must survive — a flattened version would have 5 sibling
            1. s. + // Whitespace between closing tags is normalized away in this assertion. + noWS := strings.Join(strings.Fields(xmlOutput), "") + assert.Contains(t, noWS, "
            2. Outerone.
                ") + assert.Contains(t, xmlOutput, "
              1. Inner one.
                2. ") + assert.Contains(t, xmlOutput, "
              2. Inner two.
                3. ") + assert.Contains(t, noWS, "
              3. ") + assert.Contains(t, xmlOutput, "
            3. Outer two.
              4. ") + + // List must remain tight — no

              wrappers around items. + assert.NotContains(t, xmlOutput, "

            4. Outer one.") + assert.NotContains(t, xmlOutput, "

            5. Inner one.") +} + +// TestRoundTrip_NestedMixedList guards the same fix for an OL containing a +// nested UL, which is what the tt.xml mismatch exercised. +func TestRoundTrip_NestedMixedList(t *testing.T) { + xmlInput := `

                ` + + `
              1. Outer one.
                  ` + + `
                • Bullet one.
                  • ` + + `
                • Bullet two.
                  • ` + + `
                2. ` + + `
              2. Outer two.
                3. ` + + `
              ` + + md, err := ConfluenceToMarkdown(xmlInput) + require.NoError(t, err) + + xmlOutput, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + + noWS := strings.Join(strings.Fields(xmlOutput), "") + assert.Contains(t, noWS, "
            6. Outerone.
                ") + assert.Contains(t, xmlOutput, "
              • Bullet one.
                • ") + assert.Contains(t, xmlOutput, "
              • Bullet two.
                • ") + assert.Contains(t, noWS, "
              7. ") + assert.Contains(t, xmlOutput, "
            7. Outer two.
              8. ") + assert.NotContains(t, xmlOutput, "

            8. Outer one.") +} + +// TestRoundTrip_DoubleNestedUnorderedList covers a UL nested inside another +// UL — pre-fix the cumulative indent computation didn't account for the +// parent list, so deeply nested items collapsed. +func TestRoundTrip_DoubleNestedUnorderedList(t *testing.T) { + xmlInput := `

                ` + + `
              • Top one.
                  ` + + `
                • Mid one.
                    ` + + `
                  • Deep one.
                    • ` + + `
                  • Deep two.
                    • ` + + `
                  • ` + + `
                • Mid two.
                  • ` + + `
                • ` + + `
              ` + + md, err := ConfluenceToMarkdown(xmlInput) + require.NoError(t, err) + + xmlOutput, err := MarkdownToConfluence([]byte(md)) + require.NoError(t, err) + + noWS := strings.Join(strings.Fields(xmlOutput), "") + assert.Contains(t, noWS, "
            9. Topone.
                ") + assert.Contains(t, noWS, "
              • Midone.
                  ") + assert.Contains(t, xmlOutput, "
                • Deep one.
                  • ") + assert.Contains(t, xmlOutput, "
                • Deep two.
                  • ") + assert.Contains(t, xmlOutput, "
                • Mid two.
                  • ") + // Three nesting levels collapse to three closing
                s. + assert.Equal(t, 3, strings.Count(xmlOutput, "
              ")) +} diff --git a/converter/xml2md.go b/converter/xml2md.go index 27a89576e023a0d6c4a77e12ba898bfdb94a9bd0..e20e41c10248557fb9ca8138a584b4b4da16b5ca 100644 --- a/converter/xml2md.go +++ b/converter/xml2md.go @@ -71,9 +71,48 @@ func preprocessCDATA(s string) string { } type xmlConverter struct { - buf *bytes.Buffer - listDepth int - inListItem bool + buf *bytes.Buffer + listDepth int + inListItem bool + listIndents []int // per-level indent width contributed by each ancestor list +} + +// pushList increments list depth and records the indent contribution for the +// new level (3 spaces for ordered, 2 for unordered/task). Items at deeper +// levels indent by the cumulative width of all ancestors. +func (c *xmlConverter) pushList(width int) { + c.listDepth++ + c.listIndents = append(c.listIndents, width) +} + +// popList undoes pushList. +func (c *xmlConverter) popList() { + c.listDepth-- + if len(c.listIndents) > 0 { + c.listIndents = c.listIndents[:len(c.listIndents)-1] + } +} + +// itemIndent returns the indent string for a list item at the current depth. +// It sums the contributions of all ancestor lists (everything except the +// innermost level, since that's the level whose marker we're about to emit). +func (c *xmlConverter) itemIndent() string { + total := 0 + if len(c.listIndents) > 1 { + for i := 0; i < len(c.listIndents)-1; i++ { + total += c.listIndents[i] + } + } + return strings.Repeat(" ", total) +} + +// ensureSingleNewline writes a newline only if the buffer does not already end +// with one, preventing accidental blank lines that turn tight lists loose. +func (c *xmlConverter) ensureSingleNewline() { + bs := c.buf.Bytes() + if len(bs) == 0 || bs[len(bs)-1] != '\n' { + c.buf.WriteByte('\n') + } } func (c *xmlConverter) walkChildren(n *html.Node, depth int) { @@ -183,22 +222,22 @@ func (c *xmlConverter) walk(n *html.Node, depth int) { // Lists case tag == "ul": - c.listDepth++ + c.pushList(2) // "- " marker → children indent 2 spaces if c.listDepth == 1 { c.buf.WriteString("\n") } c.walkChildren(n, depth) - c.listDepth-- + c.popList() if c.listDepth == 0 { c.buf.WriteString("\n") } case tag == "ol": - c.listDepth++ + c.pushList(3) // "1. " marker → children indent 3 spaces if c.listDepth == 1 { c.buf.WriteString("\n") } c.walkOL(n, depth) - c.listDepth-- + c.popList() if c.listDepth == 0 { c.buf.WriteString("\n") } @@ -209,13 +248,12 @@ func (c *xmlConverter) walk(n *html.Node, depth int) { if hasTaskStatus(n) { // Task status handler will write the prefix, walkChildrenInline for text c.walkChildrenInline(n, depth) - c.buf.WriteString("\n") + c.ensureSingleNewline() } else { - indent := strings.Repeat(" ", max(0, c.listDepth-1)) - c.buf.WriteString(indent) + c.buf.WriteString(c.itemIndent()) c.buf.WriteString("- ") c.walkChildrenInline(n, depth) - c.buf.WriteString("\n") + c.ensureSingleNewline() } c.inListItem = prev @@ -314,8 +352,8 @@ func (c *xmlConverter) handleConfluenceElement(n *html.Node, tag string, depth i // Confluence links (user mentions, page links) case strings.Contains(tag, "ac:link"): - if c.hasUserChild(n) { - c.walkChildren(n, depth) + if pageNode := findPageChild(n); pageNode != nil { + c.writePageLinkSpan(pageNode, n) } else { c.walkChildren(n, depth) } @@ -341,15 +379,15 @@ func (c *xmlConverter) handleConfluenceElement(n *html.Node, tag string, depth i // Confluence task lists case strings.Contains(tag, "task-list"): - c.listDepth++ + c.pushList(2) c.walkChildren(n, depth) - c.listDepth-- + c.popList() case strings.Contains(tag, "task-body"): c.walkChildren(n, depth) - c.buf.WriteString("\n") + c.ensureSingleNewline() case strings.Contains(tag, "task-status"): status := strings.TrimSpace(getTextContent(n)) - indent := strings.Repeat(" ", max(0, c.listDepth-1)) + indent := c.itemIndent() if status == "complete" { c.buf.WriteString(indent + "- [x] ") } else { @@ -672,12 +710,38 @@ func (c *xmlConverter) writeTableRow(cells []string, cols int) { cell = cells[i] } c.buf.WriteString(" ") - c.buf.WriteString(cell) + c.buf.WriteString(escapeTablePipes(cell)) c.buf.WriteString(" |") } c.buf.WriteString("\n") } +// escapeTablePipes escapes literal `|` characters in GFM table cell content. +// Per the GFM spec, `\|` is recognized as an escaped pipe at table-parse time +// and reduced to `|` before inline parsing — so escaped pipes survive even +// inside `code spans`. Pre-existing `\|` sequences are left alone to avoid +// double-escaping. +func escapeTablePipes(s string) string { + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); i++ { + ch := s[i] + if ch == '\\' && i+1 < len(s) && s[i+1] == '|' { + b.WriteByte('\\') + b.WriteByte('|') + i++ + continue + } + if ch == '|' { + b.WriteByte('\\') + b.WriteByte('|') + continue + } + b.WriteByte(ch) + } + return b.String() +} + func (c *xmlConverter) writeTableSep(cols int) { c.buf.WriteString("|") for range cols { @@ -694,11 +758,10 @@ func (c *xmlConverter) walkOL(n *html.Node, depth int) { } tag := strings.ToLower(child.Data) if tag == "li" { - indent := strings.Repeat(" ", max(0, c.listDepth-1)) - c.buf.WriteString(indent) + c.buf.WriteString(c.itemIndent()) fmt.Fprintf(c.buf, "%d. ", idx) c.walkChildrenInline(child, depth) - c.buf.WriteString("\n") + c.ensureSingleNewline() idx++ } } @@ -1024,6 +1087,80 @@ func (c *xmlConverter) hasUserChild(n *html.Node) bool { return false } +// findPageChild returns the descendant of an , if any. +// ac:link wraps either an , , or other ri:* reference; +// page links need a special preservation path because they have no plain +// markdown equivalent. +func findPageChild(n *html.Node) *html.Node { + for child := n.FirstChild; child != nil; child = child.NextSibling { + if child.Type == html.ElementNode { + tag := strings.ToLower(child.Data) + if strings.Contains(tag, "ri:page") { + return child + } + } + } + return nil +} + +// writePageLinkSpan emits a placeholder that survives +// markdown round-trip. md2xml's convertRawSpan reverses this back into a full +// . +func (c *xmlConverter) writePageLinkSpan(page, link *html.Node) { + space := getAttr(page, "ri:space-key") + if space == "" { + space = getAttr(page, "space-key") + } + title := getAttr(page, "ri:content-title") + if title == "" { + title = getAttr(page, "content-title") + } + anchor := getAttr(link, "ac:anchor") + if anchor == "" { + anchor = getAttr(link, "anchor") + } + + c.buf.WriteString(`") + c.buf.WriteString(body) + c.buf.WriteString("") + } else { + c.buf.WriteString("/>") + } +} + +// pageLinkBody returns the plain-text body of an , dropping the +// reference itself. Confluence allows or +// raw text as the visible label of a page link. +func pageLinkBody(link *html.Node) string { + var b strings.Builder + for child := link.FirstChild; child != nil; child = child.NextSibling { + switch child.Type { + case html.TextNode: + b.WriteString(child.Data) + case html.ElementNode: + tag := strings.ToLower(child.Data) + if strings.Contains(tag, "ri:page") || strings.Contains(tag, "ri:user") { + continue + } + b.WriteString(getTextContent(child)) + } + } + return strings.TrimSpace(b.String()) +} + // Helper functions func findNode(n *html.Node, tag string) *html.Node { @@ -1121,26 +1258,26 @@ func extractAttrOrder(n *html.Node) string { return strings.Join(names, ",") } -// isNextSiblingCode checks if the next non-whitespace sibling is a element. +// isNextSiblingCode checks if the next sibling is a element directly +// adjacent in the source. Any intervening text node (even whitespace) breaks +// adjacency, so `a b` does not merge. func isNextSiblingCode(n *html.Node) bool { - for s := n.NextSibling; s != nil; s = s.NextSibling { - if s.Type == html.TextNode && strings.TrimSpace(s.Data) == "" { - continue - } - return s.Type == html.ElementNode && strings.ToLower(s.Data) == "code" + s := n.NextSibling + if s == nil { + return false } - return false + return s.Type == html.ElementNode && strings.ToLower(s.Data) == "code" } -// isPrevSiblingCode checks if the previous non-whitespace sibling is a element. +// isPrevSiblingCode checks if the previous sibling is a element directly +// adjacent in the source. Any intervening text node (even whitespace) breaks +// adjacency. func isPrevSiblingCode(n *html.Node) bool { - for s := n.PrevSibling; s != nil; s = s.PrevSibling { - if s.Type == html.TextNode && strings.TrimSpace(s.Data) == "" { - continue - } - return s.Type == html.ElementNode && strings.ToLower(s.Data) == "code" + s := n.PrevSibling + if s == nil { + return false } - return false + return s.Type == html.ElementNode && strings.ToLower(s.Data) == "code" } // isParentTag reports whether n's parent is an element with the given tag name.