Make attachment filenames round-trip: percent-encode paths, record the source path in the comment

[willkg]

Attachment filenames are currently derived by replacing / with _, which is neither injective nor decodable. Replace it with a bijective percent-encoding, record the original source path in the attachment's comment, and teach the storage→markdown converter to recover the path — so an image's directory structure survives a publish/read round trip.

Current behavior

Encoding happens in exactly one place, internal/convert/images.go:147:

// 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 decode. renderImage (internal/convert/storage_to_md.go:377) uses ri:filename verbatim as the markdown src.

Problem 1: 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 (images.go:59), the second file is silently skipped: one set of bytes is uploaded and both images resolve to it. _ is a poor separator precisely because _ is so common in filenames.

Problem 2: directory structure is unrecoverable

The publish → read → publish cycle is a stable fixed point today, 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 doesn't exist on disk. This is what makes #37 (export) incoherent — export can't lay attachments out so the markdown's image links resolve.

Why not just use __

The obvious fix — / → __ — was considered and rejected. It is a substitution, not an escaping scheme: it never escapes its own delimiter, so it cannot be bijective. Three concrete failures:

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 the worst: #37's export would write to /a.png. Any correct scheme must escape its own escape character.

Decision: percent-encode

Encode % → %25, then / → %2F. Bijective, standard, decodable with the standard library, and legible to anyone who has seen a URL.

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, Problem 1 is fixed by construction: two distinct sources can no longer produce one name, the images.go:59 dedupe becomes sound, and the doc comment's collision-free claim becomes true.

Alternatives rejected: escaped-underscore (_→__, /→_s) is equally bijective but produces unreadable names like docs_sguide_simg.png; fullwidth solidus ／ renders beautifully but is a confusable-character hazard.

This assumes Confluence stores % in an attachment name verbatim — that it doesn't reject it, strip it, or normalize %2F back to a slash. Atlassian documents none of this, so it must be verified empirically against a live instance before the rest of the work proceeds. If the assumption fails, fall back to escaped-underscore.

Decision: page-anchored names, root-bounded check

Image resolution stays page-relative (filepath.Join(baseDir, src) where baseDir is the markdown file's directory) — unchanged, and the same thing GitHub does when rendering a repo's markdown.

• The name is anchored to the page, derived from the src the author wrote. So it never depends on which directory markfluence was invoked from, and no --root flag is needed.
• The documentation root (the working directory) bounds what's allowed. An image resolving outside it becomes an IMAGE BROKEN warning, consistent with the existing broken-image handling. markfluence is expected to run from the root of the documentation tree.

With cwd docs/ and page docs/guide/foo.md:

src	resolves to	attachment name
image1.png	docs/guide/image1.png	image1.png
sub/deep.png	docs/guide/sub/deep.png	sub%2Fdeep.png
../assets/logo.png	docs/assets/logo.png	..%2Fassets%2Flogo.png
../../outside.png	outside.png	❌ IMAGE BROKEN (outside the root)

Because ../assets/logo.png is a legitimate shared-assets layout that we deliberately support, .. can legitimately appear in a name.

Decision: decode rejects 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 was not created by markfluence — reject it and fall back to the raw attachment name.

.. is not rejected: we produce it legitimately, and read only prints text, so an accurate ../assets/logo.png is the right output. Clamping .. belongs to #37, at the point where export actually writes files into a destination directory — which it must do for hand-uploaded attachments regardless.

Decision: the comment carries the source path

Decoding a filename is inference. The comment is truth, and markfluence already writes one on every attachment it uploads.

New format, retiring the dead mzcld name:

legacy:  mzcld:checksum: ab12cd…
new:     markfluence: sha256=ab12cd… path=assets/x.png

Both forms are parsed, and skip/update compares the parsed sha256, not the raw comment string. Without that, changing the format would force a needless re-upload of every attachment whose name didn't change; with it, an unchanged root-level image keeps its legacy comment and is still correctly skipped. The path gets 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.

Decision: how read uses the comment

StorageToMarkdown is a pure function over a storage string — no client, no page id — so it cannot look up comments itself. Add a lookup parameter:

func StorageToMarkdown(storage string, sources map[string]string) (string, error)
// sources maps attachment filename → original source path; nil falls back to
// decoding the filename.

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 (read.go:142). There is exactly one production caller (read.go:85), so the signature change is cheap.

Migration: orphaned attachments

markfluence never deletes. For every already-published page with a subdirectory image, the next update will upload under the new name, rewrite the body to reference it, and leave 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.

No warning is planned: detecting it means plumbing a legacy name through convert.Attachment → client.LocalAttachment → planAttachments for a one-time cosmetic concern. #9's attachment-list will give users a direct way to see every attachment on a page and remove orphans themselves.

Work items

• Probe first: confirm a live Confluence stores assets%2Fx.png verbatim and its _links.download resolves. Everything below depends on it.
• attachmentFilename: normalize (path.Clean, strip leading /), then percent-encode; correct the doc comment.
• Reject images resolving outside the documentation root as IMAGE BROKEN.
• Add the source path to convert.Attachment and client.LocalAttachment; thread it through toLocalAttachments (duplicated in update.go:298 and create.go:499).
• New comment format with tolerant parsing of both forms; compare on parsed sha.
• StorageToMarkdown(storage, sources); decode %2F, prefer the comment, reject absolute results.
• read: conditional ListAttachments, graceful degradation.
• Tests both directions: round-trip fixed point, every edge case in the tables above, absolute-path rejection, outside-root rejection, legacy-comment skip.
• Regenerate goldens (make regen-regressions) — affects images-local, image-properties, kitchen-sink, and adds a field to every golden with attachments.
• Update testdata/storage2md/images/ fixtures, which assert assets_diagram.png / assets_shot.png.
• Document the encoding and the docs-root convention in README (currently undocumented, which is part of why it's surprising), plus the orphaned-attachment consequence.

Related

• Implement attachment subcommands (list, upload, download) #9 (attachment subcommands) — paused pending this; a standalone attachment-upload needs settled guidance on what remote name to use.
• Implement an export subcommand (page + attachments to the filesystem) #37 (export) — the consumer that makes the decode worth having, and the owner of .. clamping at write time.

[willkg]

Probe results: percent-encoding is confirmed viable

Verified against a live Cloud instance (via the api.atlassian.com gateway, scoped token). The probe attachment was uploaded to a scratch page and deleted afterward; the page is back to its prior state.

1. Confluence stores % in an attachment name verbatim. Uploaded as probe%2Fsub%2Fx.png; both the create response and a subsequent GET .../child/attachment return title: 'probe%2Fsub%2Fx.png' — not rejected, not stripped, not normalized back to a slash.

2. ri:filename matching is literal — no URL-decoding. Rendered non-destructively via POST /wiki/rest/api/contentbody/convert/view?contentIdContext=<page>:

ri:filename	result
probe%2Fsub%2Fx.png	✅ resolves — <img class="confluence-embedded-image" src="…/probe%252Fsub%252Fx.png?…">
assets_markfluence-test.png (control, existing)	✅ resolves — structurally identical
definitely-not-here.png (negative control)	❌ unknown-attachment placeholder

The negative control confirms the test discriminates. Note the rendered src is double-encoded (%252F = %25 + 2F): Confluence treats %2F as literal filename characters and escapes them correctly for the URL. This is exactly the desired behavior.

3. The comment format survives. markfluence: sha256=deadbeef path=probe/sub/x.png stored and returned verbatim under metadata.comment.

4. Download works. GET {base}/wiki + _links.download → HTTP 200, image/png, bytes byte-identical to the upload.

The load-bearing assumption holds; no fallback to escaped-underscore is needed.

Incidental findings useful for #9

• expand=extensions yields fileSize and mediaType; version.number is available — everything attachment-list needs.
• The collection carries size/limit/start, and omits _links.next entirely when results fit in one page — so start/limit offset pagination is the reliable approach.
• _links.download has the form /rest/api/content/{pageId}/child/attachment/{attId}/download and resolves against {base}/wiki.
• A real attachment on the scratch page still carries a legacy mzcld:checksum: … comment, confirming the tolerant-parsing requirement is not hypothetical.