From 6bb0fee30fc9acfade9da440adbf7daae74ee1f2 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Wed, 5 Aug 2026 18:18:05 -0400 Subject: [PATCH 1/4] feat(convert): percent-encode image paths into attachment names Attachment names were derived by replacing "/" with "_", which was neither injective nor decodable. "a/b.png" and "a_b.png" both flattened to "a_b.png", and since the encoded name is also the dedupe key, the second file was silently dropped -- so the doc comment's "collision-free" claim was false. Percent-encode instead ("%" -> "%25", then "/" -> "%2F"). Escaping the escape character makes the mapping bijective, which fixes the collision by construction and lets a later read recover an image's original path rather than a flattened one. Confluence stores such names verbatim and matches ri:filename literally, verified against Cloud. A substitution like "/" -> "__" was considered and rejected: it never escapes its own delimiter, so "__a.png" would decode to the absolute path "/a.png". Also record the normalized source path on each collected attachment, and bound publishing by the documentation root: image resolution stays page-relative (as GitHub does), so a shared "../assets" directory works, but an image resolving outside the root markfluence was run from is now IMAGE BROKEN. --- internal/convert/attachname.go | 73 ++++++++++++++++++ internal/convert/attachname_test.go | 114 ++++++++++++++++++++++++++++ internal/convert/convert.go | 9 +++ internal/convert/images.go | 42 +++++++--- internal/convert/page.go | 6 +- internal/convert/renderer.go | 4 + 6 files changed, 238 insertions(+), 10 deletions(-) create mode 100644 internal/convert/attachname.go create mode 100644 internal/convert/attachname_test.go diff --git a/internal/convert/attachname.go b/internal/convert/attachname.go new file mode 100644 index 0000000..ec398ff --- /dev/null +++ b/internal/convert/attachname.go @@ -0,0 +1,73 @@ +package convert + +// attachname.go owns the mapping between a markdown image's source path and the +// Confluence attachment name it is published under. +// +// "/" is not legal in an attachment name, so the path is flattened. The encoding +// escapes its own escape character, which makes it bijective: distinct source +// paths always produce distinct names, and every name markfluence produces +// decodes back to the exact path it came from. That is what lets `read` +// reconstruct an image's original location, and it is why two images can never +// silently collide on one attachment name. +// +// Confluence stores these names verbatim and matches ri:filename literally -- +// verified against Cloud, where "a%2Fb.png" resolves and renders with the name +// re-escaped as "a%252Fb.png" in the image URL. + +import ( + "path" + "strings" +) + +const ( + pctEscape = "%25" // a literal "%" in the source path + pctSlash = "%2F" // a "/" path separator +) + +// attachmentFilename derives the Confluence attachment name for a markdown image +// src. The src is normalized first so a name can never decode to an absolute +// path, then "%" is encoded before "/" -- in that order, so the escapes +// introduced for "/" are not themselves escaped. +func attachmentFilename(src string) string { + rel := normalizeSrc(src) + rel = strings.ReplaceAll(rel, "%", pctEscape) + return strings.ReplaceAll(rel, "/", pctSlash) +} + +// attachmentSource inverts attachmentFilename, recovering the source path an +// attachment was published from. It reports false when the name could not have +// come from markfluence -- currently when it decodes to an empty or absolute +// path, which attachmentFilename never produces -- so callers fall back to +// treating the attachment name as the path. +// +// A name markfluence did not create is decoded on a best-effort basis: there is +// no way to tell a hand-uploaded "a%2Fb.png" from one we published. +func attachmentSource(filename string) (string, bool) { + // Decode "%2F" first and "%25" last. Replacing "%2F" only ever removes text + // and cannot spell a new "%25", while "%25" must be replaced last precisely + // so its output is not rescanned -- that is how a literal "%2F" in the source + // path (encoded "%252F") round-trips instead of collapsing to a separator. + s := strings.ReplaceAll(filename, pctSlash, "/") + s = strings.ReplaceAll(s, pctEscape, "%") + if s == "" || path.IsAbs(s) { + return "", false + } + return s, true +} + +// normalizeSrc reduces a markdown image src to a clean relative path: "./a/x.png" +// and "a/./x.png" both become "a/x.png". A leading "/" is dropped because image +// resolution joins src onto the page's directory anyway, so an absolute-looking +// src was never actually absolute -- and dropping it keeps attachmentFilename +// from ever producing a name that decodes to an absolute path. +// +// A ".." prefix is preserved: an image in a shared directory above the page +// ("../assets/logo.png") is a supported layout, the same as it would be viewing +// the file on GitHub. +func normalizeSrc(src string) string { + s := strings.TrimPrefix(path.Clean(src), "/") + if s == "." { + return "" + } + return s +} diff --git a/internal/convert/attachname_test.go b/internal/convert/attachname_test.go new file mode 100644 index 0000000..219ef7b --- /dev/null +++ b/internal/convert/attachname_test.go @@ -0,0 +1,114 @@ +package convert + +import "testing" + +// TestAttachmentNameRoundTrip is the core guarantee: every source path encodes to +// an attachment name that decodes back to exactly that path. The cases include +// the ones a naive "/" -> "__" substitution gets wrong. +func TestAttachmentNameRoundTrip(t *testing.T) { + cases := []struct { + src string + name string + }{ + {"x.png", "x.png"}, + {"assets/x.png", "assets%2Fx.png"}, + {"a/b/c/deep.png", "a%2Fb%2Fc%2Fdeep.png"}, + + // "_" is ordinary text, so a path and a name that merely looks flattened + // stay distinct -- the collision the old "/" -> "_" encoding produced. + {"a/b.png", "a%2Fb.png"}, + {"a_b.png", "a_b.png"}, + + // A leading "__" must not decode to an absolute path. + {"__a.png", "__a.png"}, + // A component ending in "_" must not shift the separator. + {"a_/b.png", "a_%2Fb.png"}, + // A literal "%2F" in the filename must not decode to a separator. + {"a%2Fb.png", "a%252Fb.png"}, + // The escape character itself. + {"100%.png", "100%25.png"}, + {"%25.png", "%2525.png"}, + + // A shared asset above the page is a supported layout. + {"../assets/logo.png", "..%2Fassets%2Flogo.png"}, + + // Spaces and other characters are left alone -- only "/" is illegal. + {"my docs/a b.png", "my docs%2Fa b.png"}, + } + for _, c := range cases { + if got := attachmentFilename(c.src); got != c.name { + t.Errorf("attachmentFilename(%q) = %q, want %q", c.src, got, c.name) + } + got, ok := attachmentSource(c.name) + if !ok { + t.Errorf("attachmentSource(%q) refused a name we produced", c.name) + continue + } + if got != c.src { + t.Errorf("attachmentSource(%q) = %q, want %q (round trip)", c.name, got, c.src) + } + } +} + +// TestAttachmentFilenameIsInjective is what makes the dedupe in renderImage sound: +// no two distinct sources may share one attachment name. +func TestAttachmentFilenameIsInjective(t *testing.T) { + srcs := []string{ + "a/b.png", "a_b.png", "a__b.png", "a%2Fb.png", "__a.png", "a_/b.png", + "x.png", "assets/x.png", "../x.png", "100%.png", + } + seen := map[string]string{} + for _, src := range srcs { + name := attachmentFilename(src) + if prev, dup := seen[name]; dup { + t.Errorf("%q and %q both encode to %q", prev, src, name) + } + seen[name] = src + } +} + +// TestAttachmentFilenameNormalizes folds equivalent spellings of one path onto a +// single name, so the same file is not uploaded twice under different names. +func TestAttachmentFilenameNormalizes(t *testing.T) { + cases := []struct{ src, want string }{ + {"./x.png", "x.png"}, + {"./assets/x.png", "assets%2Fx.png"}, + {"assets/./x.png", "assets%2Fx.png"}, + {"assets/../assets/x.png", "assets%2Fx.png"}, + // Resolution joins src onto the page directory, so a leading "/" was never + // really absolute; dropping it keeps names from decoding to absolute paths. + {"/assets/x.png", "assets%2Fx.png"}, + } + for _, c := range cases { + if got := attachmentFilename(c.src); got != c.want { + t.Errorf("attachmentFilename(%q) = %q, want %q", c.src, got, c.want) + } + } +} + +// TestAttachmentSourceRefusesAbsolute covers names markfluence never produces: +// a hand-uploaded attachment must not be able to steer a reader at an absolute +// path (which is what #37's export would then write to). +func TestAttachmentSourceRefusesAbsolute(t *testing.T) { + for _, name := range []string{"%2Fetc%2Fpasswd.png", "%2F.png", ""} { + if got, ok := attachmentSource(name); ok { + t.Errorf("attachmentSource(%q) = %q, true; want refusal", name, got) + } + } +} + +// TestAttachmentSourceDecodesForeignNames documents best-effort behavior for +// attachments markfluence did not upload: they decode like any other name, since +// there is no way to tell them apart. +func TestAttachmentSourceDecodesForeignNames(t *testing.T) { + for _, c := range []struct{ name, want string }{ + {"hand-uploaded.png", "hand-uploaded.png"}, + {"screenshot 2026.png", "screenshot 2026.png"}, + {"..%2Fup.png", "../up.png"}, + } { + got, ok := attachmentSource(c.name) + if !ok || got != c.want { + t.Errorf("attachmentSource(%q) = %q, %v; want %q, true", c.name, got, ok, c.want) + } + } +} diff --git a/internal/convert/convert.go b/internal/convert/convert.go index 5faa4c8..c84221f 100644 --- a/internal/convert/convert.go +++ b/internal/convert/convert.go @@ -7,6 +7,7 @@ package convert import ( "bytes" + "os" "path/filepath" "strings" @@ -59,8 +60,16 @@ func MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version str // escaping them; restore them after rendering. shielded, unshield := shieldStorage(md.Body) dir := filepath.Dir(md.Filename) + // The documentation root is the working directory: markfluence is run from + // the root of a documentation tree. An unresolvable cwd disables the check + // rather than failing the conversion. + root, err := os.Getwd() + if err != nil { + root = "" + } r := &storageRenderer{ baseDir: dir, + root: root, currentBasename: filepath.Base(md.Filename), baseURL: baseURL, spaceKey: spaceKey, diff --git a/internal/convert/images.go b/internal/convert/images.go index 707ef57..0f04189 100644 --- a/internal/convert/images.go +++ b/internal/convert/images.go @@ -49,6 +49,11 @@ func (r *storageRenderer) renderImage( r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) + case !r.withinRoot(filepath.Join(r.baseDir, src)): + msg := fmt.Sprintf("IMAGE BROKEN: %s (outside the documentation root)", src) + r.broken = append(r.broken, msg) + _, _ = w.WriteString(html.EscapeString(msg)) + case !isFile(filepath.Join(r.baseDir, src)): msg := fmt.Sprintf("IMAGE BROKEN: %s (not found)", src) r.broken = append(r.broken, msg) @@ -65,7 +70,9 @@ func (r *storageRenderer) renderImage( if err != nil { abs = filepath.Join(r.baseDir, src) } - r.attachments = append(r.attachments, Attachment{Filename: filename, Path: abs}) + r.attachments = append(r.attachments, Attachment{ + Filename: filename, Path: abs, Source: normalizeSrc(src), + }) } _, _ = w.WriteString(acImage(alt, attrs, filename, "")) } @@ -141,14 +148,6 @@ func acImage(alt string, attrs map[string]string, riFilename, riURL string) stri return fmt.Sprintf("%s", leading, resource) } -// attachmentFilename derives a stable, collision-free attachment name from an -// image path: a leading "./" is dropped and "/" becomes "_" so images from -// different directories don't collide. -func attachmentFilename(src string) string { - name := strings.TrimPrefix(src, "./") - return strings.ReplaceAll(name, "/", "_") -} - // nodeText returns the concatenated plain text of a node's descendants. func nodeText(n ast.Node, source []byte) string { var b strings.Builder @@ -178,6 +177,31 @@ func isFile(path string) bool { return err == nil && !info.IsDir() } +// withinRoot reports whether an image path resolves inside the documentation +// root. markfluence is meant to be run from the root of a documentation tree, so +// an image above it -- "../../../secrets/x.png" -- is a mistake rather than a +// shared asset, and is reported broken instead of published. +// +// A path at or below the root is fine, including one reached via ".." from a +// page in a subdirectory: "../assets/logo.png" from docs/guide/foo.md is the +// ordinary shared-assets layout. The check fails open when the root is unknown +// or a path cannot be resolved -- it is an authoring guard, not a security +// boundary. +func (r *storageRenderer) withinRoot(p string) bool { + if r.root == "" { + return true + } + abs, err := filepath.Abs(p) + if err != nil { + return true + } + rel, err := filepath.Rel(r.root, abs) + if err != nil { + return true + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + func isDigits(s string) bool { if s == "" { return false diff --git a/internal/convert/page.go b/internal/convert/page.go index aacc16e..9efc2cc 100644 --- a/internal/convert/page.go +++ b/internal/convert/page.go @@ -15,8 +15,12 @@ type ConfluencePage struct { } // Attachment is a local image the body references, to be uploaded to the page. -// Path is absolute; Filename is the stable, collision-free attachment name. +// Path is absolute. Filename is the attachment name, a bijective encoding of +// Source, so distinct images can never collide on one name. Source is the +// normalized page-relative path the image was written as, recorded on the +// attachment so a later read recovers it exactly rather than inferring it. type Attachment struct { Filename string `json:"filename"` Path string `json:"path"` + Source string `json:"source"` } diff --git a/internal/convert/renderer.go b/internal/convert/renderer.go index 0fa4607..3188376 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -21,6 +21,10 @@ import ( type storageRenderer struct { baseDir string + // root bounds which images may be published: the documentation root, which + // markfluence is expected to be run from. Empty disables the check. + root string + // Link/anchor rewriting context, populated per conversion. currentBasename string baseURL string From ebac47f83d1f588bf12f12d22a2a9bd7912fc3f1 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Wed, 5 Aug 2026 18:18:16 -0400 Subject: [PATCH 2/4] feat(convert): recover an image's original path when reading a page StorageToMarkdown used ri:filename verbatim as the markdown src, so reading a page back produced a flattened path naming a file that does not exist on disk. Decode the attachment name, and prefer the source path markfluence records on the attachment when it is available, since that is exact rather than inferred. An absolute result is refused in both cases and falls back to the raw attachment name: markfluence never produces one, so it means the attachment came from somewhere else -- and export (#37) will write these paths to disk. StorageToMarkdown takes a sources map to carry that data; nil keeps the old name-decoding behavior. read builds it from ListAttachments, but only when the body actually references an attachment, and tolerates a failed lookup rather than failing the read. Threading the map required making the render chain methods on an mdRenderer. Rewriting ri:filename in the parsed tree would have been smaller but would corrupt attachment references inside unknown macros that pass through as raw storage. --- cmd/read/read.go | 28 +++- internal/convert/storage_to_md.go | 132 +++++++++++------- internal/convert/storage_to_md_test.go | 53 ++++++- .../regression/image-properties/test.output | 7 +- .../testdata/regression/images-broken/main.md | 4 + .../regression/images-broken/test.output | 5 +- .../regression/images-local/test.output | 7 +- .../images-shared-parent/assets/logo.png | 1 + .../images-shared-parent/sub/main.md | 7 + .../images-shared-parent/test.input | 4 + .../images-shared-parent/test.output | 12 ++ .../regression/kitchen-sink/test.output | 7 +- .../testdata/storage2md/images/input.storage | 4 +- .../testdata/storage2md/images/output.md | 4 +- 14 files changed, 203 insertions(+), 72 deletions(-) create mode 100644 internal/convert/testdata/regression/images-shared-parent/assets/logo.png create mode 100644 internal/convert/testdata/regression/images-shared-parent/sub/main.md create mode 100644 internal/convert/testdata/regression/images-shared-parent/test.input create mode 100644 internal/convert/testdata/regression/images-shared-parent/test.output diff --git a/cmd/read/read.go b/cmd/read/read.go index b9fade9..73ccc3d 100644 --- a/cmd/read/read.go +++ b/cmd/read/read.go @@ -82,7 +82,7 @@ func run(cmd *cobra.Command, args []string) error { body := page.Body.Storage.Value if formatFlag == formatMarkdown { - body, err = convert.StorageToMarkdown(page.Body.Storage.Value) + body, err = convert.StorageToMarkdown(page.Body.Storage.Value, attachmentSources(c, page)) if err != nil { return operationalFail(pageID, err, jsonout.CodeConvert) } @@ -102,6 +102,32 @@ func run(cmd *cobra.Command, args []string) error { return nil } +// attachmentSources maps each attachment name on the page to the markdown image +// path it was published from, letting the converter restore an image's original +// location exactly rather than inferring it from the attachment name. +// +// It is an optimization, not a requirement: a page with no attachment references +// skips the lookup entirely, and a failed lookup returns nil so the converter +// falls back to decoding names -- a read is worth completing without it, the +// same way a failed page-width read is tolerated below. +func attachmentSources(c *client.ConfluenceClient, page *client.Page) map[string]string { + if !strings.Contains(page.Body.Storage.Value, "markdown walk needs. +// A fresh one is used per conversion, so nothing leaks between documents. +type mdRenderer struct { + // sources maps attachment name -> the image path it was published from. May + // be nil, in which case paths are recovered by decoding attachment names. + sources map[string]string +} + +// sourceFor resolves an attachment name back to the markdown image path to write. +// The path recorded on the attachment wins because it is exact; otherwise the +// name is decoded. An absolute path is never something markfluence published, so +// it is refused in both cases and the raw attachment name is used instead. +func (r *mdRenderer) sourceFor(filename string) string { + if src, ok := r.sources[filename]; ok && src != "" && !path.IsAbs(src) { + return src + } + if src, ok := attachmentSource(filename); ok { + return src + } + return filename +} + // snode is a minimal parsed storage node: an element (name + attrs + children) or // a text node (name == ""). type snode struct { @@ -104,7 +134,7 @@ func qname(n xml.Name) string { // blockStrings renders block-level children to a slice of block strings (joined // by callers with blank lines). listIndent is the continuation indent applied to // nested lists. -func blockStrings(kids []*snode, listIndent string) []string { +func (r *mdRenderer) blockStrings(kids []*snode, listIndent string) []string { var out []string for _, k := range kids { if k.name == "" { @@ -113,7 +143,7 @@ func blockStrings(kids []*snode, listIndent string) []string { } continue } - if s := renderBlock(k, listIndent); s != "" { + if s := r.renderBlock(k, listIndent); s != "" { out = append(out, s) } } @@ -121,44 +151,44 @@ func blockStrings(kids []*snode, listIndent string) []string { } // renderBlock renders a single block-level element. -func renderBlock(n *snode, listIndent string) string { +func (r *mdRenderer) renderBlock(n *snode, listIndent string) string { switch n.name { case "h1", "h2", "h3", "h4", "h5", "h6": level := int(n.name[1] - '0') - return strings.Repeat("#", level) + " " + renderInlineChildren(n) + return strings.Repeat("#", level) + " " + r.renderInlineChildren(n) case "p": - return renderInlineChildren(n) + return r.renderInlineChildren(n) case "ul": - return renderList(n, false, listIndent) + return r.renderList(n, false, listIndent) case "ol": - return renderList(n, true, listIndent) + return r.renderList(n, true, listIndent) case "blockquote": - return prefixLines(strings.Join(blockStrings(n.kids, ""), "\n\n"), "> ") + return prefixLines(strings.Join(r.blockStrings(n.kids, ""), "\n\n"), "> ") case "hr": return "---" case "pre": return "```\n" + textContent(n) + "\n```" case "table": - return renderTable(n) + return r.renderTable(n) case "ac:structured-macro": - return renderMacro(n, true) + return r.renderMacro(n, true) case "ac:image", "a", "strong", "b", "em", "i", "code", "del", "s", "strike", "br": // An inline element sitting at block level (Confluence often emits a bare // not wrapped in

) is rendered as its own paragraph. - return renderInline(n) + return r.renderInline(n) case "ac:layout", "ac:layout-section", "ac:layout-cell": - return renderRawBlock(n) + return r.renderRawBlock(n) case "div": - return strings.Join(blockStrings(n.kids, listIndent), "\n\n") + return strings.Join(r.blockStrings(n.kids, listIndent), "\n\n") default: // Unknown element: render its children as blocks (transparent wrapper). - return strings.Join(blockStrings(n.kids, listIndent), "\n\n") + return strings.Join(r.blockStrings(n.kids, listIndent), "\n\n") } } // renderList renders a ul/ol, incrementing ordered markers and indenting nested // lists to align under their item text. -func renderList(n *snode, ordered bool, indent string) string { +func (r *mdRenderer) renderList(n *snode, ordered bool, indent string) string { var lines []string i := 0 for _, k := range n.kids { @@ -171,31 +201,31 @@ func renderList(n *snode, ordered bool, indent string) string { marker = fmt.Sprintf("%d. ", i) } cont := indent + strings.Repeat(" ", len(marker)) - lines = append(lines, indent+marker+renderListItem(k, cont)) + lines = append(lines, indent+marker+r.renderListItem(k, cont)) } return strings.Join(lines, "\n") } // renderListItem renders an

  • : its inline/paragraph content on the first line, // with any nested lists indented beneath it. -func renderListItem(li *snode, cont string) string { +func (r *mdRenderer) renderListItem(li *snode, cont string) string { var head strings.Builder var tail []string for _, k := range li.kids { switch k.name { case "ul": - tail = append(tail, renderList(k, false, cont)) + tail = append(tail, r.renderList(k, false, cont)) case "ol": - tail = append(tail, renderList(k, true, cont)) + tail = append(tail, r.renderList(k, true, cont)) case "p": - if s := renderInlineChildren(k); s != "" { + if s := r.renderInlineChildren(k); s != "" { if head.Len() > 0 { head.WriteString(" ") } head.WriteString(s) } default: - head.WriteString(renderInline(k)) + head.WriteString(r.renderInline(k)) } } item := strings.TrimSpace(head.String()) @@ -206,7 +236,7 @@ func renderListItem(li *snode, cont string) string { } // renderTable renders a table as a GFM pipe table. Alignment is not preserved. -func renderTable(n *snode) string { +func (r *mdRenderer) renderTable(n *snode) string { var rows []*snode var header *snode var walk func(*snode) @@ -232,7 +262,7 @@ func renderTable(n *snode) string { header, rows = rows[0], rows[1:] } - head := cellTexts(header) + head := r.cellTexts(header) var b strings.Builder b.WriteString("| " + strings.Join(head, " | ") + " |\n") seps := make([]string, len(head)) @@ -240,8 +270,8 @@ func renderTable(n *snode) string { seps[i] = "---" } b.WriteString("| " + strings.Join(seps, " | ") + " |") - for _, r := range rows { - b.WriteString("\n| " + strings.Join(cellTexts(r), " | ") + " |") + for _, row := range rows { + b.WriteString("\n| " + strings.Join(r.cellTexts(row), " | ") + " |") } return b.String() } @@ -257,11 +287,11 @@ func rowHasHeaderCell(tr *snode) bool { } // cellTexts renders a row's cells to inline strings with pipes escaped. -func cellTexts(tr *snode) []string { +func (r *mdRenderer) cellTexts(tr *snode) []string { var cells []string for _, c := range tr.kids { if c.name == "th" || c.name == "td" { - text := strings.ReplaceAll(renderInlineChildren(c), "|", `\|`) + text := strings.ReplaceAll(r.renderInlineChildren(c), "|", `\|`) cells = append(cells, text) } } @@ -273,16 +303,16 @@ func cellTexts(tr *snode) []string { // through as raw storage. A block-context unknown macro uses the round-trip-safe // multi-line form (renderRawBlock, markdown body); an inline one stays raw on a // single line so it does not break out of its paragraph. -func renderMacro(n *snode, block bool) string { +func (r *mdRenderer) renderMacro(n *snode, block bool) string { switch name := n.attrs["ac:name"]; { case name == "code": return renderCodeMacro(n) case name == "toc": return tocToken case calloutMacroInverse[name] != "": - return renderCallout(n, name) + return r.renderCallout(n, name) case block: - return renderRawBlock(n) + return r.renderRawBlock(n) default: return serialize(n) } @@ -303,10 +333,10 @@ func renderCodeMacro(n *snode) string { } // renderCallout renders a callout macro as a GitHub alert blockquote. -func renderCallout(n *snode, macro string) string { +func (r *mdRenderer) renderCallout(n *snode, macro string) string { content := "[!" + calloutMacroInverse[macro] + "]" if body := findChild(n, "ac:rich-text-body"); body != nil { - if inner := strings.Join(blockStrings(body.kids, ""), "\n\n"); inner != "" { + if inner := strings.Join(r.blockStrings(body.kids, ""), "\n\n"); inner != "" { content += "\n" + inner } } @@ -316,47 +346,47 @@ func renderCallout(n *snode, macro string) string { // --- inline rendering -------------------------------------------------------- // renderInlineChildren renders a node's children as a single inline string. -func renderInlineChildren(n *snode) string { +func (r *mdRenderer) renderInlineChildren(n *snode) string { var b strings.Builder for _, k := range n.kids { - b.WriteString(renderInline(k)) + b.WriteString(r.renderInline(k)) } return strings.TrimSpace(b.String()) } // renderInline renders one inline node. -func renderInline(n *snode) string { +func (r *mdRenderer) renderInline(n *snode) string { if n.name == "" { return collapse(n.text) } switch n.name { case "strong", "b": - return "**" + renderInlineChildren(n) + "**" + return "**" + r.renderInlineChildren(n) + "**" case "em", "i": - return "*" + renderInlineChildren(n) + "*" + return "*" + r.renderInlineChildren(n) + "*" case "code": return "`" + textContent(n) + "`" case "del", "s", "strike": - return "~~" + renderInlineChildren(n) + "~~" + return "~~" + r.renderInlineChildren(n) + "~~" case "br": return " \n" case "a": - return renderLink(n) + return r.renderLink(n) case "ac:image": - return renderImage(n) + return r.renderImage(n) case "ac:structured-macro": // An inline macro (e.g. status/emoticon) stays raw on one line so it does // not break out of its paragraph. - return renderMacro(n, false) + return r.renderMacro(n, false) default: - return renderInlineChildren(n) + return r.renderInlineChildren(n) } } // renderLink renders an as a markdown link, falling back to the href as text. -func renderLink(n *snode) string { +func (r *mdRenderer) renderLink(n *snode) string { href := n.attrs["href"] - text := renderInlineChildren(n) + text := r.renderInlineChildren(n) if text == "" { text = href } @@ -368,13 +398,13 @@ func renderLink(n *snode) string { // renderImage renders an as a markdown image, reconstructing the // title/width/height/align attributes into a plain title or a JSON title. -func renderImage(n *snode) string { +func (r *mdRenderer) renderImage(n *snode) string { alt := n.attrs["ac:alt"] src := "" for _, k := range n.kids { switch k.name { case "ri:attachment": - src = k.attrs["ri:filename"] + src = r.sourceFor(k.attrs["ri:filename"]) case "ri:url": src = k.attrs["ri:value"] } @@ -519,13 +549,13 @@ func attrString(attrs map[string]string) string { // raw on a single line. This covers both column layouts and bodied macros // (expand, panel, …), keeping their bodies readable while the structure and // parameters survive verbatim. -func renderRawBlock(n *snode) string { +func (r *mdRenderer) renderRawBlock(n *snode) string { open := "<" + n.name + attrString(n.attrs) + ">" closeTag := "" // Content container: raw tags around a markdown body. if n.name == "ac:rich-text-body" || n.name == "ac:layout-cell" { - if md := strings.Join(blockStrings(n.kids, ""), "\n\n"); md != "" { + if md := strings.Join(r.blockStrings(n.kids, ""), "\n\n"); md != "" { return open + "\n\n" + md + "\n\n" + closeTag } return open + closeTag @@ -542,7 +572,7 @@ func renderRawBlock(n *snode) string { continue // drop inter-tag whitespace } if k.name == "ac:rich-text-body" || k.name == "ac:layout-cell" || hasElementChild(k) { - parts = append(parts, renderRawBlock(k)) + parts = append(parts, r.renderRawBlock(k)) } else { parts = append(parts, serialize(k)) } diff --git a/internal/convert/storage_to_md_test.go b/internal/convert/storage_to_md_test.go index b67d697..76d9706 100644 --- a/internal/convert/storage_to_md_test.go +++ b/internal/convert/storage_to_md_test.go @@ -32,7 +32,7 @@ func TestStorageToMarkdown(t *testing.T) { if err != nil { t.Fatalf("reading input: %v", err) } - md, err := convert.StorageToMarkdown(string(in)) + md, err := convert.StorageToMarkdown(string(in), nil) if err != nil { t.Fatalf("StorageToMarkdown: %v", err) } @@ -78,7 +78,7 @@ func TestStorageToMarkdownAcceptsForwardCorpus(t *testing.T) { if err := json.Unmarshal(data, &page); err != nil { t.Fatalf("parsing golden: %v", err) } - if _, err := convert.StorageToMarkdown(page.HTML); err != nil { + if _, err := convert.StorageToMarkdown(page.HTML, nil); err != nil { t.Errorf("StorageToMarkdown(%q storage): %v", name, err) } }) @@ -109,7 +109,7 @@ func TestRoundTripStableCallouts(t *testing.T) { if err != nil { t.Fatalf("MdToConfluence: %v", err) } - got, err := convert.StorageToMarkdown(page.HTML) + got, err := convert.StorageToMarkdown(page.HTML, nil) if err != nil { t.Fatalf("StorageToMarkdown: %v", err) } @@ -123,7 +123,7 @@ func TestRoundTripStableCallouts(t *testing.T) { func TestStorageToMarkdownStripsGeneratedIDs(t *testing.T) { in := `` + `DONE` - got, err := convert.StorageToMarkdown(in) + got, err := convert.StorageToMarkdown(in, nil) if err != nil { t.Fatal(err) } @@ -151,7 +151,7 @@ func TestRoundTripPassthrough(t *testing.T) { if err != nil { t.Fatalf("MdToConfluence: %v", err) } - back, err := convert.StorageToMarkdown(page.HTML) + back, err := convert.StorageToMarkdown(page.HTML, nil) if err != nil { t.Fatalf("StorageToMarkdown: %v", err) } @@ -161,3 +161,46 @@ func TestRoundTripPassthrough(t *testing.T) { }) } } + +// TestStorageToMarkdownPrefersRecordedSource checks the two ways an image path is +// recovered. The path recorded on the attachment wins because it is exact; with +// no record, the attachment name is decoded, which is equally exact for a name +// markfluence produced. +func TestStorageToMarkdownPrefersRecordedSource(t *testing.T) { + const in = `

    ` + + got, err := convert.StorageToMarkdown(in, nil) + if err != nil { + t.Fatal(err) + } + if want := "![d](assets/x.png)\n"; got != want { + t.Errorf("decoded from name: got %q, want %q", got, want) + } + + // A recorded source overrides the name -- this is what makes an attachment + // whose name cannot be decoded faithfully (one a human uploaded, or one from + // an older markfluence) still resolve to the right path. + sources := map[string]string{"assets%2Fx.png": "images/original name.png"} + got, err = convert.StorageToMarkdown(in, sources) + if err != nil { + t.Fatal(err) + } + if want := "![d](images/original name.png)\n"; got != want { + t.Errorf("from recorded source: got %q, want %q", got, want) + } +} + +// TestStorageToMarkdownRefusesAbsoluteSource covers a tampered or foreign +// attachment: neither a decoded name nor a recorded path may point a reader -- +// or a later export writing files -- at an absolute location. +func TestStorageToMarkdownRefusesAbsoluteSource(t *testing.T) { + const in = `

    ` + got, err := convert.StorageToMarkdown(in, map[string]string{"%2Fetc%2Fpasswd.png": "/etc/passwd.png"}) + if err != nil { + t.Fatal(err) + } + // Falls back to the raw attachment name rather than an absolute path. + if want := "![d](%2Fetc%2Fpasswd.png)\n"; got != want { + t.Errorf("got %q, want %q", got, want) + } +} diff --git a/internal/convert/testdata/regression/image-properties/test.output b/internal/convert/testdata/regression/image-properties/test.output index 2bbcc4d..af041dc 100644 --- a/internal/convert/testdata/regression/image-properties/test.output +++ b/internal/convert/testdata/regression/image-properties/test.output @@ -1,12 +1,13 @@ { "attachments": [ { - "filename": "assets_shot.png", - "path": "/assets/shot.png" + "filename": "assets%2Fshot.png", + "path": "/assets/shot.png", + "source": "assets/shot.png" } ], "broken": [], - "html": "

    Image Properties

    \n

    A JSON title sets title/width/height/align attributes:

    \n

    \n

    A plain-string title becomes a tooltip (ac:title):

    \n

    \n

    Invalid width/align values are dropped with warnings:

    \n

    \n", + "html": "

    Image Properties

    \n

    A JSON title sets title/width/height/align attributes:

    \n

    \n

    A plain-string title becomes a tooltip (ac:title):

    \n

    \n

    Invalid width/align values are dropped with warnings:

    \n

    \n", "warnings": [ "assets/shot.png: ignoring width='wide' (must be a number)", "assets/shot.png: ignoring align='middle' (must be left, center, or right)" diff --git a/internal/convert/testdata/regression/images-broken/main.md b/internal/convert/testdata/regression/images-broken/main.md index 1c6f85c..4877b14 100644 --- a/internal/convert/testdata/regression/images-broken/main.md +++ b/internal/convert/testdata/regression/images-broken/main.md @@ -7,3 +7,7 @@ A reference to a file that isn't there: A reference to a file with an unsupported extension: ![a pdf](notes.pdf) + +A reference to a file above the documentation root: + +![escaped](../../../../../../../../etc/passwd.png) diff --git a/internal/convert/testdata/regression/images-broken/test.output b/internal/convert/testdata/regression/images-broken/test.output index fb6dd3f..b012a92 100644 --- a/internal/convert/testdata/regression/images-broken/test.output +++ b/internal/convert/testdata/regression/images-broken/test.output @@ -2,8 +2,9 @@ "attachments": [], "broken": [ "IMAGE BROKEN: assets/missing.png (not found)", - "IMAGE BROKEN: notes.pdf (unsupported type)" + "IMAGE BROKEN: notes.pdf (unsupported type)", + "IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)" ], - "html": "

    Broken Images

    \n

    A reference to a file that isn't there:

    \n

    IMAGE BROKEN: assets/missing.png (not found)

    \n

    A reference to a file with an unsupported extension:

    \n

    IMAGE BROKEN: notes.pdf (unsupported type)

    \n", + "html": "

    Broken Images

    \n

    A reference to a file that isn't there:

    \n

    IMAGE BROKEN: assets/missing.png (not found)

    \n

    A reference to a file with an unsupported extension:

    \n

    IMAGE BROKEN: notes.pdf (unsupported type)

    \n

    A reference to a file above the documentation root:

    \n

    IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)

    \n", "warnings": [] } diff --git a/internal/convert/testdata/regression/images-local/test.output b/internal/convert/testdata/regression/images-local/test.output index eb8238f..cc5d07a 100644 --- a/internal/convert/testdata/regression/images-local/test.output +++ b/internal/convert/testdata/regression/images-local/test.output @@ -1,11 +1,12 @@ { "attachments": [ { - "filename": "assets_diagram.png", - "path": "/assets/diagram.png" + "filename": "assets%2Fdiagram.png", + "path": "/assets/diagram.png", + "source": "assets/diagram.png" } ], "broken": [], - "html": "

    Local Image

    \n

    A local image becomes an ri:attachment and is collected for upload:

    \n

    \n", + "html": "

    Local Image

    \n

    A local image becomes an ri:attachment and is collected for upload:

    \n

    \n", "warnings": [] } diff --git a/internal/convert/testdata/regression/images-shared-parent/assets/logo.png b/internal/convert/testdata/regression/images-shared-parent/assets/logo.png new file mode 100644 index 0000000..9b9a92c --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/assets/logo.png @@ -0,0 +1 @@ +stub image content (never read by md_to_confluence) diff --git a/internal/convert/testdata/regression/images-shared-parent/sub/main.md b/internal/convert/testdata/regression/images-shared-parent/sub/main.md new file mode 100644 index 0000000..f8fdcc2 --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/sub/main.md @@ -0,0 +1,7 @@ +# Shared Assets + +A page in a subdirectory referencing an asset directory above it -- the layout +GitHub renders too -- is published, with the path preserved in the attachment +name: + +![the logo](../assets/logo.png) diff --git a/internal/convert/testdata/regression/images-shared-parent/test.input b/internal/convert/testdata/regression/images-shared-parent/test.input new file mode 100644 index 0000000..e72fb1d --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/test.input @@ -0,0 +1,4 @@ +{ + "filename": "sub/main.md", + "files": ["sub/main.md", "assets/logo.png"] +} diff --git a/internal/convert/testdata/regression/images-shared-parent/test.output b/internal/convert/testdata/regression/images-shared-parent/test.output new file mode 100644 index 0000000..11c8486 --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/test.output @@ -0,0 +1,12 @@ +{ + "attachments": [ + { + "filename": "..%2Fassets%2Flogo.png", + "path": "/assets/logo.png", + "source": "../assets/logo.png" + } + ], + "broken": [], + "html": "

    Shared Assets

    \n

    A page in a subdirectory referencing an asset directory above it -- the layout GitHub renders too -- is published, with the path preserved in the attachment name:

    \n

    \n", + "warnings": [] +} diff --git a/internal/convert/testdata/regression/kitchen-sink/test.output b/internal/convert/testdata/regression/kitchen-sink/test.output index a289b1f..69cfa6c 100644 --- a/internal/convert/testdata/regression/kitchen-sink/test.output +++ b/internal/convert/testdata/regression/kitchen-sink/test.output @@ -1,11 +1,12 @@ { "attachments": [ { - "filename": "assets_logo.png", - "path": "/assets/logo.png" + "filename": "assets%2Flogo.png", + "path": "/assets/logo.png", + "source": "assets/logo.png" } ], "broken": [], - "html": "

    Release Notes

    \n\n

    See the upgrade guide before starting.

    \n

    Back up your data before upgrading.

    \n

    What's New

    \n

    A soft-wrapped paragraph describing the release across multiple source lines that collapse into one.

    \n

    \n

    Configuration

    \n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
    SettingDefault
    retries3
    timeout30s
    \npython

    A pasted status macro: Stable

    \n", + "html": "

    Release Notes

    \n\n

    See the upgrade guide before starting.

    \n

    Back up your data before upgrading.

    \n

    What's New

    \n

    A soft-wrapped paragraph describing the release across multiple source lines that collapse into one.

    \n

    \n

    Configuration

    \n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
    SettingDefault
    retries3
    timeout30s
    \npython

    A pasted status macro: Stable

    \n", "warnings": [] } diff --git a/internal/convert/testdata/storage2md/images/input.storage b/internal/convert/testdata/storage2md/images/input.storage index 1139c4f..f3f5e7b 100644 --- a/internal/convert/testdata/storage2md/images/input.storage +++ b/internal/convert/testdata/storage2md/images/input.storage @@ -1,5 +1,5 @@ -

    +

    -

    +

    diff --git a/internal/convert/testdata/storage2md/images/output.md b/internal/convert/testdata/storage2md/images/output.md index 0579601..c3006f9 100644 --- a/internal/convert/testdata/storage2md/images/output.md +++ b/internal/convert/testdata/storage2md/images/output.md @@ -1,4 +1,4 @@ -![an architecture diagram](assets_diagram.png) +![an architecture diagram](assets/diagram.png) ![a bare block-level image](bare.png) @@ -6,4 +6,4 @@ ![tooltipped](shot.png "A Tooltip") -![sized shot](assets_shot.png '{"title":"A Tooltip","width":300,"height":150,"align":"center"}') +![sized shot](assets/shot.png '{"title":"A Tooltip","width":300,"height":150,"align":"center"}') From 931bf11957d2f3720ecd7b4b5ca1a182c4fd0d3c Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Wed, 5 Aug 2026 18:18:25 -0400 Subject: [PATCH 3/4] feat(client): record an attachment's source path in its comment Decoding an attachment name is inference; the comment is truth. Stamp uploads with "markfluence: sha256= path=" so reading a page back recovers an image's original location exactly, and so an attachment can be identified as markfluence-managed. The old "mzcld:checksum: " form is still parsed, and skip/update now compares the *parsed* checksum rather than the whole comment string. Without that, changing the format would re-upload every attachment whose name did not change; with it, an unchanged file keeps its old comment and is still correctly skipped. The path is written last and unquoted so it may contain spaces. --- cmd/create/create.go | 2 +- cmd/update/update.go | 2 +- internal/client/client.go | 73 +++++++++++++++++++++-- internal/client/client_test.go | 103 +++++++++++++++++++++++++++++++-- 4 files changed, 167 insertions(+), 13 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 696033f..fde0f3a 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -499,7 +499,7 @@ func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) strin func toLocalAttachments(atts []convert.Attachment) []client.LocalAttachment { out := make([]client.LocalAttachment, len(atts)) for i, a := range atts { - out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename} + out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename, Source: a.Source} } return out } diff --git a/cmd/update/update.go b/cmd/update/update.go index 9e68b0f..306a7ef 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -298,7 +298,7 @@ func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) strin func toLocalAttachments(atts []convert.Attachment) []client.LocalAttachment { out := make([]client.LocalAttachment, len(atts)) for i, a := range atts { - out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename} + out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename, Source: a.Source} } return out } diff --git a/internal/client/client.go b/internal/client/client.go index eb26973..326d15c 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -37,9 +37,19 @@ import ( "time" ) -// attachmentChecksumPrefix is stored in an attachment's comment so a later run -// can tell whether the local file changed. -const attachmentChecksumPrefix = "mzcld:checksum: " +// An uploaded attachment carries markfluence bookkeeping in its comment: the +// checksum a later run compares to tell whether the local file changed, and the +// markdown image path it was published from, so reading the page back recovers +// the image's original location exactly instead of inferring it from the +// attachment name. +const ( + // attachmentCommentPrefix marks an attachment as markfluence-managed. + attachmentCommentPrefix = "markfluence: " + // legacyChecksumPrefix is the older checksum-only comment form. It is still + // parsed -- comparing the parsed checksum rather than the whole comment is + // what lets the format change without re-uploading every attachment. + legacyChecksumPrefix = "mzcld:checksum: " +) const ( timeoutRead = 30 * time.Second @@ -187,10 +197,58 @@ type Property struct { Version Version `json:"version"` } -// LocalAttachment is a local image to sync to a page. +// LocalAttachment is a local image to sync to a page. Source is the markdown +// image path it was written as, recorded in the attachment's comment; it may be +// empty, in which case only a checksum is recorded. type LocalAttachment struct { Path string Filename string + Source string +} + +// AttachmentMeta is the markfluence bookkeeping parsed out of an attachment's +// comment. A hand-uploaded attachment has none, leaving Managed false. +type AttachmentMeta struct { + SHA256 string + Source string + Managed bool +} + +// Meta parses this attachment's comment into markfluence's bookkeeping. +func (a Attachment) Meta() AttachmentMeta { return parseAttachmentComment(a.Metadata.Comment) } + +// attachmentComment builds the comment stored on an uploaded attachment. source +// is written last and unquoted so it may contain spaces. +func attachmentComment(sum, source string) string { + c := attachmentCommentPrefix + "sha256=" + sum + if source != "" { + c += " path=" + source + } + return c +} + +// parseAttachmentComment reads both the current form ("markfluence: sha256= +// path=") and the legacy checksum-only form, so an attachment written by +// an older markfluence is still recognized as unchanged. +func parseAttachmentComment(comment string) AttachmentMeta { + if sum, ok := strings.CutPrefix(comment, legacyChecksumPrefix); ok { + return AttachmentMeta{SHA256: strings.TrimSpace(sum), Managed: true} + } + rest, ok := strings.CutPrefix(comment, attachmentCommentPrefix) + if !ok { + return AttachmentMeta{} + } + m := AttachmentMeta{Managed: true} + if sum, after, found := strings.Cut(strings.TrimPrefix(rest, "sha256="), " "); found { + m.SHA256, rest = sum, after + } else { + m.SHA256, rest = sum, "" + } + // path= is last, so its value is the remainder verbatim. + if src, ok := strings.CutPrefix(rest, "path="); ok { + m.Source = src + } + return m } // SyncAction reports what sync_attachments did for one file. @@ -539,7 +597,7 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt if err != nil { return nil, err } - comment := attachmentChecksumPrefix + sum + comment := attachmentComment(sum, att.Source) contentType := mime.TypeByExtension(filepath.Ext(att.Filename)) if contentType == "" { contentType = "application/octet-stream" @@ -549,7 +607,10 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt switch { case !ok: p.action = "created" - case cur.Metadata.Comment == comment: + case cur.Meta().SHA256 == sum: + // Compare the checksum, not the whole comment: an attachment stamped + // by an older markfluence is unchanged and must not be re-uploaded + // merely because the comment format has moved on. p.action = "skipped" default: p.action = "updated" diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 7435afd..39d8f76 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -1,10 +1,12 @@ package client import ( + "io" "net/http" "net/http/httptest" "os" "path/filepath" + "strings" "sync/atomic" "testing" "time" @@ -20,9 +22,19 @@ func TestMain(m *testing.M) { type scripted struct { responses []resp calls []string + bodies []string idx int } +// lastBody returns the body of the most recent request, for assertions about +// what was actually sent. +func (s *scripted) lastBody() string { + if len(s.bodies) == 0 { + return "" + } + return s.bodies[len(s.bodies)-1] +} + type resp struct { status int body string @@ -33,6 +45,8 @@ func newServer(t *testing.T, responses ...resp) (*ConfluenceClient, *scripted) { s := &scripted{responses: responses} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { s.calls = append(s.calls, r.Method) + body, _ := io.ReadAll(r.Body) + s.bodies = append(s.bodies, string(body)) if s.idx >= len(s.responses) { t.Errorf("unexpected extra request: %s %s", r.Method, r.URL.Path) w.WriteHeader(500) @@ -345,10 +359,12 @@ func TestSyncAttachmentsCreatesWhenAbsent(t *testing.T) { } } -func TestSyncAttachmentsSkipsWhenChecksumMatches(t *testing.T) { +// A legacy comment still identifies an unchanged file, so a format change does +// not force a re-upload of every attachment. +func TestSyncAttachmentsSkipsWhenLegacyChecksumMatches(t *testing.T) { path, sum := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - attachmentChecksumPrefix + sum + `"}}]}` + legacyChecksumPrefix + sum + `"}}]}` c, s := newServer(t, resp{200, list}) actions, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}) if err != nil { @@ -365,7 +381,7 @@ func TestSyncAttachmentsSkipsWhenChecksumMatches(t *testing.T) { func TestSyncAttachmentsUpdatesWhenChecksumDiffers(t *testing.T) { path, _ := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - attachmentChecksumPrefix + `stale"}}]}` + legacyChecksumPrefix + `stale"}}]}` c, s := newServer(t, resp{200, list}, resp{200, `{}`}) actions, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}) if err != nil { @@ -383,8 +399,8 @@ func TestPlanAttachmentsClassifiesWithoutUploading(t *testing.T) { path, sum := writeTempImage(t) // same.png matches (skip), stale.png differs (update), new.png is absent (create). list := `{"results":[` + - `{"id":"a1","title":"same.png","metadata":{"comment":"` + attachmentChecksumPrefix + sum + `"}},` + - `{"id":"a2","title":"stale.png","metadata":{"comment":"` + attachmentChecksumPrefix + `stale"}}` + + `{"id":"a1","title":"same.png","metadata":{"comment":"` + legacyChecksumPrefix + sum + `"}},` + + `{"id":"a2","title":"stale.png","metadata":{"comment":"` + legacyChecksumPrefix + `stale"}}` + `]}` c, s := newServer(t, resp{200, list}) actions, err := c.PlanAttachments("1", []LocalAttachment{ @@ -573,3 +589,80 @@ func TestResolveNext(t *testing.T) { }) } } + +// TestAttachmentCommentRecordsSource checks the comment an upload stamps: the +// checksum used to skip unchanged files, plus the path the image was published +// from so a later read recovers it exactly. +func TestAttachmentCommentRecordsSource(t *testing.T) { + got := attachmentComment("abc123", "../assets/logo.png") + if want := "markfluence: sha256=abc123 path=../assets/logo.png"; got != want { + t.Errorf("attachmentComment = %q, want %q", got, want) + } + if got := attachmentComment("abc123", ""); got != "markfluence: sha256=abc123" { + t.Errorf("attachmentComment without a source = %q", got) + } +} + +func TestParseAttachmentComment(t *testing.T) { + cases := []struct { + name string + comment string + want AttachmentMeta + }{ + {"current form", "markfluence: sha256=abc123 path=assets/x.png", + AttachmentMeta{SHA256: "abc123", Source: "assets/x.png", Managed: true}}, + {"no source recorded", "markfluence: sha256=abc123", + AttachmentMeta{SHA256: "abc123", Managed: true}}, + // The path is written last and unquoted, so it may contain spaces. + {"source with spaces", "markfluence: sha256=abc123 path=my docs/a b.png", + AttachmentMeta{SHA256: "abc123", Source: "my docs/a b.png", Managed: true}}, + {"legacy form", legacyChecksumPrefix + "abc123", + AttachmentMeta{SHA256: "abc123", Managed: true}}, + {"hand-uploaded", "a note from a human", AttachmentMeta{}}, + {"empty", "", AttachmentMeta{}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := parseAttachmentComment(c.comment); got != c.want { + t.Errorf("parseAttachmentComment(%q) = %+v, want %+v", c.comment, got, c.want) + } + }) + } +} + +// TestSyncAttachmentsStampsSource checks that an upload records the source path, +// so reading the page back can recover the image's original location. +func TestSyncAttachmentsStampsSource(t *testing.T) { + path, _ := writeTempImage(t) + c, s := newServer(t, resp{200, `{"results":[]}`}, resp{200, `{}`}) + _, err := c.SyncAttachments("1", []LocalAttachment{ + {Path: path, Filename: "assets%2Fx.png", Source: "assets/x.png"}, + }) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(s.lastBody(), "path=assets/x.png") { + t.Errorf("upload body did not record the source path:\n%s", s.lastBody()) + } +} + +// TestSyncAttachmentsSkipsWhenCurrentChecksumMatches is the new-format twin of the +// legacy skip test. +func TestSyncAttachmentsSkipsWhenCurrentChecksumMatches(t *testing.T) { + path, sum := writeTempImage(t) + list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + + attachmentComment(sum, "x.png") + `"}}]}` + c, s := newServer(t, resp{200, list}) + actions, err := c.SyncAttachments("1", []LocalAttachment{ + {Path: path, Filename: "x.png", Source: "x.png"}, + }) + if err != nil { + t.Fatal(err) + } + if len(actions) != 1 || actions[0].Action != "skipped" { + t.Fatalf("actions = %v, want [skipped]", actions) + } + if !eqStrings(s.calls, []string{"GET"}) { + t.Errorf("calls = %v, want [GET] (no upload)", s.calls) + } +} From 41c42d24c55c7444926b847699f91914150985de Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Wed, 5 Aug 2026 18:18:32 -0400 Subject: [PATCH 4/4] docs: document attachment name encoding and the documentation root Cover the percent-encoding, that image paths resolve relative to the markdown file the way GitHub renders them, that markfluence should be run from the root of the documentation tree, and that republishing a page leaves its old underscore-named attachment behind unreferenced -- markfluence never deletes. --- CLAUDE.md | 4 +- README.md | 28 +++ _plans/018_attachment-name-encoding.md | 275 +++++++++++++++++++++++++ 3 files changed, 305 insertions(+), 2 deletions(-) create mode 100644 _plans/018_attachment-name-encoding.md diff --git a/CLAUDE.md b/CLAUDE.md index fd0572f..c4936bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,8 +48,8 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `cmd/root.go` — the cobra root: `--url`/`--username`/`--debug`/`--no-color` persistent flags, version from `internal/buildinfo`, and registration of the four subcommands. `Execute()` prints cobra-generated errors (bad args/flags) but not `ui.ErrSilent`, which marks a failure a command already reported. - `cmd/{update,create,fix,info}/` — one package per command (each exports `Cmd`), orchestrating the `internal` packages and `internal/ui` output. `create` is two-phase and transactional (validate all, then create parents-first in topological order); `fix` is read-only on the server. -- `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send` (429 for any method honoring `Retry-After`, plus 502/503/504 and network errors for idempotent methods only; exponential backoff capped), `SetContentProperty` retry-once on top (recovers a lost create-POST response), `SyncAttachments` (SHA-256-in-comment skip/update), `_links.next` pagination. `config.go` holds `Resolve` and the `.env` reader. -- `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version string) (*ConfluencePage, error)`. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `images.go`, `links.go` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting), `tables.go` (the `` tag, stamped with Confluence's `data-layout="align-start"` so tables auto-size to their content and left-align, plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour` — rows still fall through to the GFM renderer), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. +- `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send` (429 for any method honoring `Retry-After`, plus 502/503/504 and network errors for idempotent methods only; exponential backoff capped), `SetContentProperty` retry-once on top (recovers a lost create-POST response), `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; the comment is `markfluence: sha256=… path=…`, and the legacy `mzcld:checksum: …` form is still parsed — the skip test compares the *parsed* checksum, so the format change doesn't force a re-upload), `_links.next` pagination. `config.go` holds `Resolve` and the `.env` reader. +- `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version string) (*ConfluencePage, error)`. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `attachname.go` owns the source-path↔attachment-name mapping (percent-encoding `%`→`%25` then `/`→`%2F`, which is **bijective** — that is what makes the dedupe collision-free and lets `read` recover an image's original path; decode refuses an absolute result, which markfluence never produces); `images.go` (resolution stays page-relative like GitHub; the documentation root — cwd — bounds what may be published, and an image above it is `IMAGE BROKEN`), `links.go` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting), `tables.go` (the `
    ` tag, stamped with Confluence's `data-layout="align-start"` so tables auto-size to their content and left-align, plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour` — rows still fall through to the GFM renderer), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. - `internal/frontmatter` — flat YAML frontmatter parse/quote/`UpdateField`, and the `MarkdownFile` type (`Parse`/`ParseFile`, exported `Filename`/`Content`/`Frontmatter`/`Body`, and `Title`/`PageID`/`Space`/`Parent` accessors that normalize missing/blank/`"null"`). - `internal/pagewidth` — the `page_width` `Width` enum (`narrow`/`wide`/`max`, default `max`), `Declared`, the vocab↔content-property maps, `WidthFromProperties`, and `Apply`/`Read` against the client. - `internal/buildinfo` — `Version` (set via ldflags), `CommitDate` (from the `vcs.time` build setting), and `Stamp`. diff --git a/README.md b/README.md index a2be604..30cdf89 100644 --- a/README.md +++ b/README.md @@ -502,6 +502,34 @@ Example: references a remote URL); a missing/unsupported image becomes `IMAGE BROKEN: …` text. +Image paths resolve relative to the Markdown file, the same way they do when you +view the file on GitHub, so a page in a subdirectory can share an asset +directory above it: + +``` +docs/ ← run markfluence from here + assets/logo.png + guide/page.md → ![logo](../assets/logo.png) +``` + +**Run markfluence from the root of your documentation tree.** That root bounds +which images may be published: an image resolving outside it (`../../secrets/x.png`) +is reported as `IMAGE BROKEN: … (outside the documentation root)` rather than +uploaded. + +Confluence attachment names cannot contain `/`, so the path is percent-encoded +into the attachment name — `assets/logo.png` is attached as `assets%2Flogo.png`, +and `../assets/logo.png` as `..%2Fassets%2Flogo.png`. The encoding is reversible, +so `markfluence read` restores an image's original path instead of a flattened +one. markfluence also records the source path in the attachment's comment, which +it prefers over decoding the name. + +> [!NOTE] +> Pages published before this encoding existed used `/` → `_`. Republishing such +> a page uploads the image under its new name and updates the page to match, but +> the old attachment stays behind, unreferenced — markfluence never deletes. +> Remove those manually if the clutter bothers you. + Extra properties ride in the title as JSON: ```markdown diff --git a/_plans/018_attachment-name-encoding.md b/_plans/018_attachment-name-encoding.md new file mode 100644 index 0000000..a55e4d9 --- /dev/null +++ b/_plans/018_attachment-name-encoding.md @@ -0,0 +1,275 @@ +# Plan: attachment names that round-trip + +Make the mapping between a markdown image's source path and its Confluence +attachment name **bijective**, and record the source path on the attachment, so +that publishing a page and reading it back recovers the image's original +location instead of a flattened approximation. + +Confluence attachment names cannot contain `/`, so a path has to be flattened +into a name. Today that is one line (`internal/convert/images.go:147`): + +```go +// attachmentFilename derives a stable, collision-free attachment name from an +// image path: a leading "./" is dropped and "/" becomes "_" so images from +// different directories don't collide. +func attachmentFilename(src string) string { + name := strings.TrimPrefix(src, "./") + return strings.ReplaceAll(name, "/", "_") +} +``` + +There is no inverse: `renderImage` (`internal/convert/storage_to_md.go:377`) +uses `ri:filename` verbatim as the markdown `src`. That produces two defects. + +**The "collision-free" claim is false.** `a/b.png` and `a_b.png` both flatten to +`a_b.png`. Because the encoded name is also the dedupe key +(`internal/convert/images.go:59`), the *second* file is silently skipped: one set +of bytes is uploaded and both images on the page resolve to it. `_` is a bad +separator precisely because `_` is common in filenames. + +**Directory structure is unrecoverable.** The publish → `read` → publish cycle is +a stable fixed point, but a lossy one: + +``` +docs/img.png → publish → docs_img.png → read → ![](docs_img.png) → publish → docs_img.png +``` + +The directory is gone and the read-back `src` names a file that does not exist on +disk. This is what makes #37 (`export`) incoherent — export cannot lay +attachments out so the markdown's image links resolve. #37 calls this +reconciliation its crux. + +## Out of scope (deliberately) + +- **Clamping `..` when writing files.** A decoded `../assets/logo.png` is + legitimate (see the base-directory decision below), and `read` only prints + text, so an accurate path is the right output. Refusing to write outside a + destination directory belongs to #37, at the point export actually creates + files — which it must do for hand-uploaded attachments regardless. +- **Warning about orphaned attachments.** Detecting one means plumbing a legacy + name through `convert.Attachment` → `client.LocalAttachment` → + `planAttachments` for a one-time cosmetic concern. #9's `attachment-list` will + let users see every attachment on a page and remove orphans directly. +- **#9 (attachment subcommands).** Paused for this: a standalone + `attachment-upload` needs settled guidance on what remote name to use. + +## Decisions locked + +### Percent-encode, because the encoding must escape its own escape character + +`%` → `%25`, then `/` → `%2F`. Decode reverses it: `%2F` → `/` first, `%25` → `%` +last, so the `%25` output is not rescanned — which is exactly how a literal +`%2F` in a source path (encoded `%252F`) round-trips instead of collapsing into a +separator. + +| source path | `_` (today) | `%2F` | +|---|---|---| +| `assets/x.png` | `assets_x.png` | `assets%2Fx.png` | +| `a/b.png` | `a_b.png` ⚠️ | `a%2Fb.png` | +| `a_b.png` | `a_b.png` ⚠️ collides with the above | `a_b.png` | +| `__a.png` | `__a.png` | `__a.png` | +| `a_/b.png` | `a__b.png` | `a_%2Fb.png` | +| literal `a%2Fb.png` | `a%2Fb.png` | `a%252Fb.png` | + +Because the encoding is injective, **the collision defect is fixed by +construction**: two distinct sources can no longer produce one name, the dedupe +becomes sound, and the doc comment's claim becomes true. + +**Rejected: `/` → `__`.** The obvious fix, and wrong. It is a substitution, not +an escaping scheme — it never escapes its own delimiter, so it cannot be +bijective: + +| source | `__` encoding | decodes to | +|---|---|---| +| `__a.png` | `__a.png` | `/a.png` — **absolute path at filesystem root** | +| `a_/b.png` | `a___b.png` | `a/_b.png` — wrong | +| literal `a__b.png` | `a__b.png` | `a/b.png` — wrong | + +The first is disqualifying: #37's export would write to `/a.png`. + +**Rejected: escaped underscore** (`_` → `__`, then `/` → `_s`). Equally bijective +and certain to be accepted by any API, but produces names like +`docs_sguide_simg.png`. Kept as the fallback if percent-encoding had failed the +live probe. + +**Rejected: fullwidth solidus `／`** (U+FF0F). Renders almost identically to a +slash in Confluence's UI, which is genuinely attractive, but it is a +confusable-character hazard — users copy the name and get a character that is not +a slash — plus non-ASCII filename risk on upload and on export to disk. + +### Names are page-anchored; the documentation root bounds what may be published + +Image *resolution* stays page-relative — `filepath.Join(baseDir, src)` where +`baseDir` is the markdown file's directory — unchanged, and the same thing GitHub +does rendering a repo's markdown. + +Two separable questions follow: what bounds the "too far out" check, and what the +name is anchored to. With cwd `docs/` and page `docs/guide/foo.md`: + +| src | resolves to | A: root-anchored | **B: page-anchored, root-bounded** | C: page-anchored, page-bounded | +|---|---|---|---|---| +| `image1.png` | `docs/guide/image1.png` | `guide%2Fimage1.png` | `image1.png` | `image1.png` | +| `sub/deep.png` | `docs/guide/sub/deep.png` | `guide%2Fsub%2Fdeep.png` | `sub%2Fdeep.png` | `sub%2Fdeep.png` | +| `../assets/logo.png` | `docs/assets/logo.png` | `assets%2Flogo.png` | `..%2Fassets%2Flogo.png` | ❌ BROKEN | +| `../../outside.png` | `outside.png` | ❌ BROKEN | ❌ BROKEN | ❌ BROKEN | + +**B is chosen**, because it is how these files behave viewed on GitHub: the path +the author wrote is the truth, and a shared asset directory above the page works. + +- The name derives from the `src`, so it **never depends on which directory + markfluence was invoked from** — no `--root` flag is needed, and cwd only + affects the boundary check, not naming. +- A is the only model where `..` can never appear in a name, which would let + decode refuse both absolute and escaping paths. Its price is that the name + depends on the invocation directory: run from `docs/guide/` instead of `docs/` + and the same image gets a different name, silently orphaning the old + attachment — the exact migration pain this work exists to stop repeating. +- C keeps stable names *and* the clean invariant, but bans shared asset + directories outright, forcing assets to be duplicated under each page's + directory. + +markfluence is expected to be run from the root of the documentation tree. An +image resolving outside it is `IMAGE BROKEN: … (outside the documentation root)`, +consistent with the existing broken-image handling. The check fails open when cwd +is unknown: it is an authoring guard, not a security boundary. + +### Decode refuses absolute paths only + +The encode side normalizes (`path.Clean`, strip a leading `/`) so an absolute path +is never produced. On decode, an absolute result therefore proves the attachment +did not come from markfluence — refuse it and fall back to the raw attachment +name. + +`..` is deliberately **not** refused: under model B we produce it legitimately. + +No encoding can fix the underlying ambiguity for foreign attachments — a +hand-uploaded `%2Fetc%2Fpasswd.png` decodes to `/etc/passwd.png` under *any* +scheme, because there is no way to distinguish a name we wrote from one a human +typed. That is why the guard lives on the decode side rather than in the +encoding. + +### The comment carries the source path + +Decoding a name is inference; the comment is truth, and markfluence already +writes one on every attachment it uploads. New format, retiring the dead `mzcld` +name (markfluence has not been that tool for a long time, and #9's +`attachment-list` would surface the string to users): + +``` +legacy: mzcld:checksum: ab12cd… +new: markfluence: sha256=ab12cd… path=assets/x.png +``` + +`path=` is written last and unquoted so a source path may contain spaces. + +**Both forms are parsed, and skip/update compares the parsed `sha256` rather than +the raw comment string.** Without that, changing the format would force a +needless re-upload of every attachment whose name did not change; with it, an +unchanged root-level image keeps its legacy comment and is still correctly +skipped. The path is stamped the next time the file actually changes. + +This also gives #9's `attachment-list` a real signal for whether an attachment is +markfluence-managed. + +### `StorageToMarkdown` takes a sources map + +It is a pure function over a storage string — no client, no page id — so it +cannot look up comments itself: + +```go +func StorageToMarkdown(storage string, sources map[string]string) (string, error) +// sources maps attachment name → source path; nil falls back to decoding names. +``` + +`read` builds the map from `ListAttachments`, but **only when the body actually +contains `ri:attachment`**, and a failed listing passes `nil` rather than failing +the read — the same tolerance `read` already applies to page width +(`cmd/read/read.go:142`). There is exactly one production caller +(`cmd/read/read.go:85`), so the signature change is cheap. + +Threading the map requires converting the render chain (`blockStrings`, +`renderBlock`, `renderList`, `renderListItem`, `renderTable`, `cellTexts`, +`renderMacro`, `renderCallout`, `renderInlineChildren`, `renderInline`, +`renderLink`, `renderImage`, `renderRawBlock`) into methods on a small +`mdRenderer`, mirroring the forward direction's `storageRenderer`. + +**Rejected: rewriting `ri:filename` in the parsed tree** after `parseStorage`, +which would need no signature changes at all. It would corrupt attachment +references inside unknown macros, which pass through as raw storage and are +re-serialized verbatim — writing a decoded path back into published content. + +## Migration: orphaned attachments + +markfluence never deletes. For every already-published page with a subdirectory +image, the next `update` uploads under the new name, rewrites the body to +reference it, and leaves the old `assets_x.png` attached but unreferenced. +Nothing breaks and no data is lost, but the cruft is permanent and accumulates +with every page published under the old scheme before this lands — which is an +argument for doing it sooner rather than later. + +## Verified against the live API + +Atlassian documents none of the attachment-name rules, so percent-encoding was +**probed before any code was written**; the fallback was escaped-underscore. + +- **`%` is stored verbatim.** Uploaded as `probe%2Fsub%2Fx.png`; both the create + response and a later `GET .../child/attachment` return the title unchanged — + not rejected, not stripped, not normalized back to a slash. +- **`ri:filename` matching is literal.** Rendered non-destructively via + `POST /wiki/rest/api/contentbody/convert/view?contentIdContext=`: the + percent-encoded name resolves to a real `confluence-embedded-image`, + structurally identical to an underscore-named control, while a nonexistent + attachment renders the `unknown-attachment` placeholder — so the test + discriminates. The rendered `src` is **double-encoded** (`%252F`), i.e. + Confluence treats `%2F` as literal filename characters and escapes them + correctly. +- **The comment format survives** verbatim under `metadata.comment`. +- **Download works**: `{base}/wiki` + `_links.download` → 200, bytes identical. + +End-to-end after implementation, against a real page: `assets/markfluence-test.png` +published as `assets%2Fmarkfluence-test.png`, the page renders 3 embedded images +with 0 placeholders, `read` returned `![…](assets/markfluence-test.png)` with the +directory recovered, and a re-run skips. + +Incidental findings, recorded for #9: `expand=extensions` yields `fileSize` and +`mediaType`; `version.number` is available; the collection carries +`size`/`limit`/`start` and **omits `_links.next` when results fit one page**, so +`start`/`limit` offset pagination is the reliable approach; `_links.download` has +the form `/rest/api/content/{pageId}/child/attachment/{attId}/download`. + +## Testing + +- `internal/convert/attachname_test.go`: round-trip over every edge case above — + the three `__` failures, the escape character itself, spaces, `../` — plus an + injectivity test (the property the dedupe depends on), normalization of + equivalent spellings, and refusal of names decoding to absolute paths. +- Regression cases: `images-shared-parent` (a page in a subdirectory using + `../assets`, proving the supported layout publishes as `..%2F…`) and a third + entry in `images-broken` for an image above the root. +- `internal/convert/storage_to_md_test.go`: a recorded source overrides a decoded + name; an absolute recorded source is refused. +- `internal/client/client_test.go`: comment construction and parsing of both + forms, that an upload stamps `path=`, and skip-on-match for the new format. The + **existing** skip/update tests are repointed at `legacyChecksumPrefix`, so they + become the legacy-tolerance coverage. +- Goldens: `make regen-regressions`, plus the `storage2md/images` fixtures, whose + `input.storage` moves to the new scheme so `output.md` demonstrates the decode. + +## Docs + +- `README.md`: images section — resolution is page-relative like GitHub, run from + the documentation root, the encoding itself with worked examples, and a note + that republishing leaves the old underscore-named attachment behind. +- `CLAUDE.md`: `attachname.go` in the converter breakdown, the root-bounding rule + in `images.go`, and the comment format plus tolerant-parsing rationale on + `SyncAttachments`. + +## Commits + +1. `feat(convert): percent-encode image paths into attachment names` — encoding, + normalization, root bounding, `Attachment.Source`, tests. +2. `feat(convert): recover an image's original path when reading a page` — decode, + `sources` param, `mdRenderer`, `read` plumbing, goldens. +3. `feat(client): record an attachment's source path in its comment` — comment + format, tolerant parsing, compare on parsed sha. +4. `docs: document attachment name encoding and the documentation root`.