From 6073cbc707b423554ec009e2bc73a0439e3d9dab Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 07:42:17 -0400 Subject: [PATCH 01/34] docs(plans): plan recursive export and the attachment name it depends on Exports a page's subtree, or a whole space, as a directory tree mirroring the Confluence hierarchy. The plan's first half is not about export at all: 025 asserted that nothing depends on single-page export's flat-in-the-root attachment placement, and the attachment name does. Page-scoping an attachment changes the path its markdown points at, which changes the percent-encoded name, which makes republishing create a second attachment and orphan the first. So an attachment is named by its basename instead, relying on the path already recorded in its comment since 026 commit 7. That makes page-scoped placement free, stops an asset that moves from orphaning itself, and retires the disagreement where attachfile.Resolve reads a stored name verbatim while sourceFor decodes it. It costs a publish-time refusal when two assets in one file share a basename, and one round of re-uploads for names already published with a directory component. The same rule applies to read and attachment-download, not to export alone, so the three agree on where an attachment lives and byte-identity survives for a top-level page. Slug collisions among siblings are disambiguated with a - suffix rather than refused, which also makes every page directory unique and so makes page-scoped placement collision-free by construction. L5 and L6 flip to Holds, gated on the property test they have never had. Refs #59. --- _plans/029_recursive-export.md | 650 +++++++++++++++++++++++++++++++++ 1 file changed, 650 insertions(+) create mode 100644 _plans/029_recursive-export.md diff --git a/_plans/029_recursive-export.md b/_plans/029_recursive-export.md new file mode 100644 index 0000000..7e00fec --- /dev/null +++ b/_plans/029_recursive-export.md @@ -0,0 +1,650 @@ +# Plan: recursive export, and the attachment name it depends on (#59) + +Exports a page's subtree, or a whole space, as a directory tree that mirrors the +Confluence hierarchy and publishes back unedited. Closes #59, and flips **L5** +(`roundtrip-from-confluence`) and **L6** (`roundtrip-from-disk`) from Partial to +Holds -- the two guarantees `docs/guarantees.md` already defers to this issue. + +The directory layout and the two attachment placement rules were settled in +#59's design comment and in `_plans/025` §"Multi-page export layout". This plan +does not re-derive them. What it adds is the sequence, and the two decisions 025 +got wrong -- the attachment name, and what to do about a slug collision. + +## The thing 025 got wrong + +025 says of page-scoped attachment placement: "Single-page export adopts the same +rule ... Nothing depends on today's flat-in-the-root behaviour." + +Something does: the attachment **name**. + +`images.go:104` names an attachment `AttachmentFilename(rootRel)` -- the +root-relative source path, percent-encoded -- and `client.go:1028` keys existing +attachments by name to decide create-vs-update. So the name is the identity, and +it is a function of the path. Page-scoping changes the path a native attachment's +markdown points at (`diagram.png` becomes `home/diagram.png`), which changes the +name (`home%2Fdiagram.png`), which makes republishing **create a second +attachment and orphan the first** -- with a changed `ri:filename` in the body, so +the page changes too. That breaks the attachment-name round-trip, which is the +half of L5 that does hold today -- not L5 as a whole, which #125 showed failing +single-page for an unrelated reason (`260585a`). + +Putting the page file inside its own directory does not dodge it: `Source` is +root-relative since `_plans/026` commit 4, so the name follows the asset's +position in the tree and not its position relative to the page. **Flat at the +dest root is the only placement that preserves an encoded name.** + +### So the name changes instead + +An attachment is named by its **basename**. `attachname.go`'s own doc comment +says the encoding buys exactly two things, and both are payable another way: + +| the encoding bought | replaced by | +|---|---| +| no two assets can collide on one name | an explicit refusal at publish time, naming both paths | +| the path is recoverable from the name alone | the `path=` already recorded in the comment (026 commit 7) | + +What that buys back, beyond L5: + +- **Page-scoped placement becomes free.** The name stops moving when the file + moves, so `dest//` costs nothing and orphans nothing. +- **Moving an asset stops orphaning it.** `assets/flow.png` to `img/flow.png` + keeps the name, and the recorded-path disagreement already makes that an + update that repairs the path (`client.go`, the path-disagreement rule). +- **`attachfile.Resolve` and `convert.sourceFor` stop disagreeing.** They + disagree today: `Resolve` uses `a.Title` verbatim (`attachfile.go:99`, + deliberately -- a hand-uploaded `a%2Fb.png` is indistinguishable from a + published one) while `sourceFor` decodes it (`storage_to_md.go:117`). A page + whose attachment is encoded but whose comment is missing therefore exports to + `dest/assets%2Fbrand.png` with markdown saying `assets/brand.png`: a broken + image, today, in single-page export. Deleting the decode retires the class. +- **Names become readable in Confluence's own UI**, which is where everyone + other than markfluence sees them. + +What it costs, stated so nobody discovers it later: + +1. **A new publish-time failure.** One file referencing `arch/diagram.png` and + `deploy/diagram.png` has no valid naming. Refused, not warned: publishing + would render one image in both places and record only one path. +2. **Path recovery rests entirely on the comment.** A markfluence-published + attachment whose comment is gone (an older format, a hand edit) is + indistinguishable from a Confluence-native one, so it takes the unsourced + rule -- `dest//` -- rather than returning to the directory it + was published from. +3. **Switching schemes orphans once.** Every already-published attachment whose + path has a directory component re-uploads under its basename, leaving the + encoded original behind. One time, on markfluence's own pages. Orphan cleanup + is a follow-up (bound by **S4**-**S6**, all three currently Vacuous). +4. **An attachment's version history can mix two assets.** A page that used to + reference `assets/flow.png` and now references `img/flow.png` updates the one + `flow.png` attachment in place. The rendering is right and the recorded path + is right; only the history is mixed. +5. **A hand-uploaded attachment is clobbered on a far wider surface.** An + unmanaged attachment has `SHA256 == ""`, which never equals a real sum, so it + is silently overwritten as an update (`client.go:1052`). That is today's + behaviour, but today it takes a hand-upload named like an encoded path; + under basenames, any hand-uploaded `logo.png` on a page is taken over the + moment that page references any `**/logo.png`. Versioned and recoverable, and + adjacent in spirit to **S5** without breaching it -- markfluence is writing a + new version of something it did not create. +6. **Two same-basename assets can still collide through paths the converter does + not see**: a batch of `attachment-upload` FILEs, and an `ri:filename` inside + raw storage the shield passes through. Both are closed in commits 2 and 4 + rather than accepted; they are listed here because neither is covered by the + one refusal in `images.go` that the argument above rests on. + +## Decisions + +### Surface + +**`export --depth`**, a string vocabulary: a non-negative number or `all`, +default `0`. Mirrors `children --depth`'s precedent, with one deliberate +divergence to state in the code: `children` refuses `0` because there it is a +request for nothing, while here it is the named page alone -- real, useful, and +the current default. Depth counts levels *below* the target, so `--depth 1` is +the page plus its direct children, and a folder counts as a level exactly as it +does in `children`. + +**`--space KEY`** lists a whole space instead of a page, via +`pagetree.WalkSpace`, resolved through `ResolveSpaceID` first so an unknown key +fails as a typo (exit 2) rather than as a 404 indistinguishable from a rejected +credential. Exactly one of `PAGE` and `--space`, checked before credentials. + +**`--space` requires an explicit `--depth`** (`Flags().Changed("depth")`, the +mechanism `children`'s hint and `search --cql` already use). A whole-space export +is thousands of requests and a large tree; it should be asked for, not be what a +bare typo produces. The error names `--depth all`. `--depth 0` with `--space` +is explicit but still a request for nothing, and is refused with the same +message -- the target of a space walk is its pages, and the space itself is not +a thing that can be written. + +**`--file` is refused when multi-page** -- it names one file, while the page's +directory name comes from the slug regardless, so allowing it would let the two +disagree. `--all-attachments` and `--skip-attachments` apply per page, unchanged. + +### Layout + +A page is `.md`, gaining a `/` beside it when it has children or +attachments. `slugify` (`cmd/export/export.go`) is reused unchanged, for folder +directories as well as page files: lowercase and trim, drop everything outside +`[\p{L}\p{N}_\s-]`, collapse each whitespace run to one `-`, trim the ends, cap +at 80 runes, and fall back to the id when nothing survives. Two properties the +pre-flight leans on: it lowercases, so `Deploy` and `deploy` already collide and +get refused; and it drops `/`, so no title can inject a path separator. + +Punctuation is deleted rather than separated, so equivalence runs through +whitespace alone -- `Title:1` yields `title1` while `Title 1` and `Title: 1` both +yield `title-1`. **Known limit:** the slug keeps non-ASCII deliberately +(`über-café`), and NFD and NFC spellings of one title are different Go strings, +so the pre-flight groups them separately where a normalizing filesystem (APFS) +sees one filename. That pair gets no suffix and the second write falls through to +the **S3** exists-skip. Normalizing before comparison would fix it and needs +`golang.org/x/text`, a new direct dependency for a case nobody has hit -- so it +is named here rather than solved. A **folder** shapes the path and nothing else: no result row, no +directory unless something lands inside (an empty folder is unrepresentable on +disk anyway -- `create` cannot make one), and as the *named root* the folder is +`dest` itself, its children landing at the top level exactly as a space's roots +do, since there is no `dest/.md` to hang a directory off. A folder title +slugging to nothing falls back to its id, as `pageFilename` already does. + +Attachments follow provenance, per 025: a recorded `path=` goes to +`dest/`, and one with none goes to `dest//`. + +**The markdown moves with the placement, and for *both* provenances.** This is +the second coincidence 025's worked example rests on, and it is not the fallback +root the `markfluence.yaml` section below deals with. `sourceFor` +(`storage_to_md.go:113`) writes a recorded path **verbatim**, and a recorded path +is root-relative (026 commit 4) -- but publish-time image resolution is +**page-relative**: `images.go:65` calls `rootRelative(r.root.Dir, r.baseDir, +fsPath)`, joining the src onto the *referencing file's own directory*. The two +agree only while the `.md` sits at the dest root, which is precisely what +mirroring ends. Left alone, `dest/home/child.md` would carry +`![](assets/brand.png)`, resolve it to `dest/home/assets/brand.png`, find +nothing, and republish as `IMAGE BROKEN` -- for every sourced asset on every +page below the root, which is most of a real tree and includes this plan's own +live fixture. + +So `StorageOptions` gains the page's **dest-relative directory**, not an +unsourced-only prefix, and `sourceFor` positions both cases against it: + +| the attachment | placed at | markdown written | +|---|---|---| +| recorded `path=assets/brand.png`, page at `dest/home/child.md` | `dest/assets/brand.png` | `../assets/brand.png` | +| recorded `path=assets/brand.png`, page at `dest/home.md` | `dest/assets/brand.png` | `assets/brand.png` | +| no recorded path, page at `dest/home/child.md` | `dest/home/child/diagram.png` | `child/diagram.png` | +| no recorded path, page at `dest/home.md` | `dest/home/diagram.png` | `home/diagram.png` | + +The fourth row is the one an earlier draft left undefined, and defining it is +what settles the rest. **Page-scoping applies to every page, the named export +target included**: `export PAGE` with no `--depth` places an unsourced +attachment at `dest//`, not at `dest/`. 025's reason holds -- +without it the same native page exports as different markdown depending on how +many pages were asked for, the invocation-dependence **L2** forbids for names +and that nothing should reintroduce for placement. + +So single-page export is **not** byte-identical to today for a page with a +comment-less attachment: it moves from `dest/diagram.png` to +`dest//diagram.png`, deliberately. A **sourced** attachment does get the +identity transform on a root page (`rel(".", recorded)` = `recorded`), so that +half is unchanged. The next section is what keeps `read` and +`attachment-download` in step with this rather than stranding them on the old +one. + +Implementation note: Go's `path` package has no `Rel`, so this is +`filepath.Rel` bracketed by `FromSlash`/`ToSlash` -- the precedent is +`rootRelative` itself (`images.go:223`), which already does the same +conversion. And the relative form is what **C1** wants anyway: it is what +resolves in a GitHub preview, and what the author originally wrote. + +### One placement rule, three commands + +An attachment with no recorded path is page-scoped by **`read`, +`attachment-download` and `export` alike** -- not by `export` only. `read` +derives the position from the page's own title, `attachment-download` writes +`dest//`, and `--flat` (which download already has) is the +documented opt-out meaning "bare name, straight in `--dest`". + +An earlier draft scoped `export` alone and left the other two flat, on the +reasoning that `read` prints to stdout and has no directory to position against. +That produced two conventions and cost `pagedoc.go:1-8`'s stated property, that +`read` and `export` emit byte-identical markdown. One rule keeps it, and is +easier to explain than what we have today: an attachment already lands at its +recorded path, directories and all, so page-scoping makes the *unsourced* case +behave the same way instead of differently. + +It also keeps the pairing that made the flat option attractive. `read`'s output +resolves against what `attachment-download` writes, because both moved, rather +than because both stayed. + +**Byte-identity survives for a page at the dest top level whose slug is unique +among its siblings** -- a single-page export, a space's root pages, the named +export target. That is the comparison anyone actually runs, and it is the one +the two-convention draft broke. + +It does *not* survive deeper in the tree, and the reason is structural rather +than fixable: three things `read` cannot know. + +- **A sourced destination is depth-dependent.** `dest/home/child.md` carries + `../assets/brand.png`, computed as `rel("home", recorded)`. `read` has no tree + and no dest, so its position is the top level and it emits the recorded path + verbatim. The unsourced case escapes this only by cancellation -- both sides + prefix the same page directory -- which is why the difference is easy to miss. +- **A `-` suffix comes from a sibling.** A page in a collision group exports + as `deploy-prod-123456/diagram.png` where `read` says `deploy-prod/`. +- **`parent:`** is a path in an exported tree and an id everywhere else. Note + this difference does not exist *today* -- `export` gets its frontmatter from + the same `pagedoc.Frontmatter` `read` uses (`export.go:153` → + `pagedoc.Render`) -- so it is introduced by commit 16, not inherited. + +So the property `pagedoc`'s package comment should state, and what commit 23 +writes there, is: **one conversion, parameterized by position and parent form, +identical output whenever those parameters are** -- with top-level placement +called out as the case where they always are. + +`pagedoc.Options` carries the position and the parent form, and all three +commands go through it, so they cannot drift by accident -- only by argument. + +### `parent:` + +An in-set parent becomes a relative `.md` path (`parent: ../home.md`), which +`create` already resolves relative to the referring file (`create.go:630`). Three +exceptions, each forced: + +- the **export root** keeps its live parent id -- truthful, and unchanged from + today's single-page export; +- a page whose parent is an exported **folder** keeps the folder id, since a + folder has no `.md` to point at and `create` accepts a folder id (#68); +- under `--space`, a root page gets `parent: null`, which is what it is. + +Two notes on what reads these. `update` never looks at `parent` at all, so the +`markfluence.yaml` marker is load-bearing for images and nothing else. And `fix` +reconciles frontmatter from the live page, so running it on an exported tree +rewrites `parent: ../home.md` back to a numeric id -- consistent with what `fix` +is for, and surprising enough to warrant a sentence in its help. + +`page_id` is kept in every file, so the tree republishes to the pages it came +from. Retargeting a tree at fresh pages would mean stripping ids: out of scope. + +### `markfluence.yaml` at `dest` + +Written for a multi-page export when `dest` has none, reported on its own line, +honoured by `--dry-run`, never overwritten (**S3**). The envelope's `roots` +becomes `[dest]`. + +**Written before the first page**, not after the last. A run that dies partway +is the case the retry story is built around, and a partial tree with no marker +is a tree whose every shared asset republishes as `IMAGE BROKEN` -- so writing +it last would hand the user exactly the broken artefact the rest of this section +exists to prevent. + +This is load-bearing, not tidiness. 025's worked example for single-page export +notes the republish works because the file lands at `out/onboarding.md`, so the +no-config fallback root -- the file's own directory -- happens to *be* `out/`. In +a mirrored tree that coincidence dies: `out/home/onboarding.md`'s fallback root is +`out/home/`, so a shared asset reconstructed at `out/assets/brand.png` sits above +its root and republishes as `IMAGE BROKEN`. Without the marker, a recursive +export is not republishable at all. + +### Collisions, refusals and failures + +**Slug collisions are disambiguated, not refused.** A pre-flight over the whole +walked set groups nodes by slug per directory -- the namespace covers page files, +page directories and folder directories together, so a page "Team" beside a +folder "Team" is one group -- and every member of a group larger than one takes a +`-` suffix on both its file and its directory. Reported as a warning naming +each page, exit 0, and the check needs only the walk's titles, so it costs no +bodies. + +It buys two things, and the second is easy to miss: sibling filenames stop +colliding, *and* every page directory in the tree becomes unique -- which is +what makes page-scoped attachment placement collision-free by construction, and +lets the conflict rule below stay as narrow as it is. + +025 and #59's comment both say refuse, and the round of grilling that produced +this plan agreed. That was wrong, on three counts: + +- **The recourse does not exist.** 025's remedy is retitling in Confluence or + exporting subtrees separately. `--space` exists to export spaces the caller + does not own, where retitling is not available -- so refusal makes such a + space permanently unexportable over a punctuation variant. +- **L2 does not cover an exported filename.** It constrains reference + resolution and attachment naming, both of which are identity. 025 invoked + "L2 in the export direction" by analogy. Per **L8** identity and hierarchy + are never inferred from disk layout -- `page_id` in the frontmatter carries + both -- so an exported filename is ergonomic, and set-dependence in an + ergonomic name costs nothing an attachment name's would. +- **"Depends on walk order" is not true.** The walk is position-ordered and + deterministic, and suffixing *every* member of the group rather than the + second one encountered removes ordering from the question entirely: no member + holds a privileged unsuffixed name. + +Suffixing also retires the argument for making this atomic in the first place. A +refusal had to abort the whole export because a refused page leaves its +children's `parent:` paths dangling; nothing is refused now. And 025's objection +to "skipping with a warning" -- that it is a partial export exiting 0 -- does not +reach this, because a suffixed export is complete. The one cost is that a later +subtree export containing only one of the pair writes the unsuffixed name. + +The `-` form is also already the established fallback: a title that slugs to +nothing becomes `.md` today (`pageFilename`). The ordering this implies is worth spelling out, because naming one consumer +invites an implementation that special-cases one consumer. The suffix pass runs +over the walk's titles and ids before any per-page work, and **everything +downstream reads its output**: the `parent:` path a child writes +(`../deploy-prod-123456.md`, not the name the page would otherwise have had), +the position handed to the renderer, the `dest//` an unsourced +attachment is placed in, the pre-fetch exists-stat the retry story depends on, +and the `--dry-run` preview. `pagetree.Walk`/`WalkSpace` return the complete +slice before any page is fetched (`pagetree.go:42`), so this is a free in-memory +pass and one processing pass after it -- not two passes over the network. + +**A walk that fails is not a page that fails.** `pagetree.Walk`/`WalkSpace` +abort on the first child-listing error (`pagetree.go:42`), before the per-page +phase exists, so the rule below does not reach it. For a `PAGE` target that is +`operationalFail` against the named id; for `--space` there is no page id to +name, so it takes the stderr `errorObject` shape `children --space` established +for exactly this -- which `find` and `search` share. + +**A page failing mid-run skips its subtree**, one failed result each, wording +from `create`'s existing precedent for the same shape (`create.go:374`, "parent +page was not created; skipping"). Same invariant: no emitted file points at a +missing parent. They count as failed, so the summary and exit status are honest. + +**Two pages writing one destination with different content fails the second**: +the attachment's status is `failed` (page-level status keeps its existing +`wrote`/`skipped`/`""` vocabulary, so the schema's enum is untouched), code +`VALIDATION`, error naming the other page. Not overridable by `--force`, which +is about local files rather than about picking a winner between two pages. +Identical bytes skip, which is 025's success case reached by **S3**. + +**The rule is deliberately narrow: it compares two recorded paths, and nothing +else.** Comparing bytes without downloading them means comparing the checksums +in the attachment comments (`parseAttachmentComment`, `client.go:394`) -- the +listing carries no server-side digest, only `Extensions.FileSize`, which can +prove difference and never identity. Two recorded paths are decidable, because a +recorded path means a managed attachment and both sides truncate their sum +identically (`client.go:1037`). Every other way two writes can meet takes the +**S3** exists-skip instead, and that is a narrowing rather than a hole: + +- **Two unsourced attachments cannot collide**, with one named exception. Each + is page-scoped into `dest//`, and the slug suffix pass makes page + directories unique by construction -- the second thing that pass buys. The + exception is the normalization limit in §Layout: an NFD/NFC title pair gets no + suffix, so on APFS the two share one directory and their same-named + attachments meet there. That pair takes the **S3** skip like everything else + here, which is the same answer, reached without the guarantee. +- **Unsourced meeting a recorded path** needs a recorded `path=` equal to another + page's slug directory -- `home/diagram.png` in a tree that also has a page + titled "Home". It is the same shape as an attachment colliding with a page + file, and gets the same answer. + +The alternative was to report a conflict whenever a checksum is missing on +either side. Rejected: it fires almost only on the case above, and it makes the +rule read differently depending on provenance for no gain in what the caller can +actually do about it. + +**An attachment colliding with a page file** gets no special handling: attachment +names are unknown until each page is fetched, so pre-flighting them would double +the walk's cost for a case measured in zero occurrences. **S3** skips it. + +### Retry cost + +The walk supplies the title, so a page's destination is known before any per-page +request. When the file exists and `--force` is absent, the page reports `skipped` +and its render is skipped -- saving the `page_width` read and every `` +title lookup -- while its attachments are still listed and the missing ones still +downloaded, which is what makes a retry resume a run that died partway through +attachments rather than pages. With `--skip-attachments` a complete tree costs +only the walk. `--force` redoes everything, and is also how a tree whose pages +changed upstream is refreshed: export never overwrites on its own. + +Serial, no concurrency: parallel fetches against a shared instance is how a rate +limit gets provoked. Cost is documented in the help the way `children`'s is. + +### Output + +Human output keeps today's `wrote`/`skipped`/`failed` line format verbatim -- +the paths and names inside those lines do change, per §Layout and the naming +switch -- and adds a trailing count for a multi-page run. + +`--json` needs no envelope change: the `export` branch is already +`oneOf[exportResult, singleOpFailure]`, `roots` is already required, and a +mid-run page failure fits `singleOpFailure`, which carries a `page_id`. Three +things inside the branch do change, and "one added field" was too optimistic: + +- **`exportResult` gains `parent_file`** (required, nullable): the relative path + written into the frontmatter, null when the parent stayed an id. `create` + already uses that name for the concept. +- **`basicSummary` is replaced by an `exportSummary` carrying `skipped`.** + `basicSummary` is `total`/`succeeded`/`failed` only, and skip-and-resume is + this feature's whole retry story -- a summary that cannot say "40 skipped" + contradicts the section above it. A separate def rather than a new key on + `basicSummary`, which `children`/`find`/`search` share. +- **The `markfluence.yaml` write gets a `project_file` field on that summary**, + reversing the earlier decision that `roots` was signal enough. `roots` cannot + distinguish a marker that was found from one that was created, and a file + written by the command that appears nowhere in `--json` -- including in its + `--dry-run` preview -- is precisely the invisible write **S3**-adjacent + reporting exists to prevent. + +## Guarantee changes + +| id | change | +|---|---| +| **L3** | Note rewritten. Both clauses become false: an asset that moves keeping its basename no longer changes identity, and tree reconstruction now rests on the comment rather than on the name. | +| **L5**, **L6** | Partial to **Holds**, but only on the back of a property test (commit 24). With basename naming a native page republishes to the same attachment, so the round-trip is a fixed point in both directions -- and that is the *argument*, not the evidence. `260585a` corrected this file to say L5 has no property test and that its status should be read "as an assertion about known constructs rather than a property", after #125 turned out to be a measured single-page counterexample to a claim made here. Flipping the status on another unverified argument would repeat exactly that. | +| **R2** | Note widened to cover naming as well as placement -- a basename collision is the same obligation one step earlier in the pipeline. **The label stays `report-unplaceable-attachments`**: `docs/guarantees.md:27` makes labels as permanent as ids, since a renamed label makes an old citation silently wrong. An earlier draft of this plan proposed renaming it, which that rule forbids. | +| **S4**-**S6** | Unchanged, still Vacuous. Orphan cleanup is a follow-up issue. | + +## Commit sequence + +The naming change comes first and stays separable in history, even though it +ships in one PR with the tree feature. + +**Naming** + +1. `feat(convert)`: name an attachment by its basename. `attachname.go` keeps the + file and its rationale, rewritten -- it still owns the mapping, now a + projection rather than a bijection. +2. `feat(convert)`: refuse two same-basename assets in one file, reported through + `Broken` with `nodeLine`'s `"line %d: "` prefix, naming both paths. + + Two constraints on the implementation. The rekeyed `r.seen` compares + `rootRel`, not the raw src, or `./a/x.png` and `a/x.png` read as a collision + with themselves -- `rootRelative` (`images.go:223`) is what cleans them into + one path. And the check covers `ri:filename` inside the raw storage the + shield passes through (`convert.go:69`), which never enters `r.seen` at all: + `export`'s own `referencedNames` (`cmd/export/export.go:224`) scans for + exactly that construct, which is the proof it occurs. Missing it would let a + converted image silently rebind a pasted reference. +3. `feat(check)`: report a basename collision offline -- no network needed to see + it, which is exactly what `check` is for. +4. `refactor(attachmentupload)`: `--name` records the path, names by basename, + and the same refusal applies across a batch of FILEs. `localAttachments` + (`attachmentupload.go:152`) has no in-batch duplicate check, and + `planAttachments` builds its `remote` map once and never updates it inside + the loop (`client.go:1026`), so `a/x.png b/x.png` would plan two `created` + calls for one title, or two `updated` calls against one `existingID` -- + last-write-wins, both reported as success. +5. `refactor(convert)`: stop interpreting a stored name. `sourceFor` becomes + recorded-path-else-verbatim and `AttachmentSource` is deleted, which is what + makes `Resolve` and `sourceFor` agree by construction. `client.go:1062`'s + comment ("The name is the encoding of the path, so the two move together") + becomes false here and is rewritten in this commit, not left for the docs one. + + `TestRoundTripEncodedImageSources` goes with it, in this commit rather than + the next: it asserts the property being removed -- "every destination + `read`/`export` writes must decode back to exactly the path `update` + published from" -- for exactly the reason this plan cites, that otherwise + "re-publishing the export would upload them again under new attachment + names". It is **rewritten, not regenerated**, into the property that replaces + it: a destination round-trips through the recorded `path=`, and a stored name + is never decoded. +6. `test`: the golden churn -- eight files carry `%2F`, and they split two ways. + Five `regression/*/test.output` goldens regenerate (`make + regen-regressions`); `regression/images-encoded-src/main.md` and the two + hand-authored `storage2md/{images,images-encoded-src}/input.storage` inputs do + not, and need editing by hand. Both `images-encoded-src` cases want renaming + as well, since the encoding stops being the thing they exercise. Beyond + `internal/convert`, `%2F` fixtures also live in `cmd/{export,attachmentupload, + attachmentlist,attachmentdownload}` and `internal/{attachfile,client,pagedoc}` + tests -- all of them fail loudly rather than silently, so this is unbudgeted + work rather than a risk. +7. `docs(confluence)`: `attachments.md`'s framing. Its opening states that + markfluence percent-encodes `%`→`%25` then `/`→`%2F` and calls it bijective; + `:58`'s name-length math is expressed in slashes-per-255-characters; `:123` + says "the name is the encoding of the path, so the two move together". All + three go. **The Verified probes stay** -- that a `%2F` name resolves and + re-escapes to `%252F` in the image URL, and that form fields decode as + Latin-1, are facts about Confluence and remain true whether or not + markfluence produces such a name. Separating the two is the whole job in this + file; do not delete a measurement because we stopped relying on it. +8. `docs`: the guarantees. L3's note (both clauses false), R2's *note* widened + to cover naming as well as placement with its label left alone + (`report-unplaceable-attachments` -- see §Guarantee changes for why a rename + is not available), and `root-model.md:114`'s "what its Confluence attachment + name encodes". +9. `docs`: README and CLAUDE.md for the naming change. README's four regions -- + the attachment-naming paragraph at `:1189`, `--name` "which markfluence + encodes for you" at `:749`, the `attachment-list` SOURCE example at `:718`, + and §`export`'s no-`--attachments-dir` rationale. CLAUDE.md's `internal/ + convert` (`attachname.go`'s clause), `attachment{list,upload,download}` + ("never a decode of the stored name"), and `internal/attachfile` paragraphs. + + Two things to get right here. **The `--attachments-dir` rationale is now + void**: it rejects the flag because rewriting an image's `src` would change + its attachment name, and under basenames moving `assets/x.png` to + `attachments/x.png` keeps the name `x.png`. Rewrite the reasoning; the flag + stays unimplemented, but it is no longer impossible and the paragraph must + not claim otherwise. And **README `:730` already documents this orphan + class** -- "attachments left behind by the encoding change" found via + `attachment-list` -- so cost 3 has precedent to point at rather than an + argument to make. +10. `docs(plans)`: a correction note on `_plans/025`, which is where "nothing + depends on today's flat-in-the-root behaviour" and the collision refusal are + asserted. Amending a landed plan is within convention here (026 has six + commits, 021-023 two each), and leaving the document that states the wrong + thing unmarked is how the next reader re-derives it. + +**Tree export** + +11. `refactor`: `slugify` moves out of `cmd/export` into a package all three + commands can reach. It stops being export's private helper the moment + `read` and `attachment-download` position attachments by the same rule. +12. `feat(pagedoc)`: the page's position through `Options`/`StorageOptions`, + applied by `sourceFor` to sourced and unsourced attachments alike. This is + the commit the L5 flip depends on; building anything downstream on an + unsourced-only prefix bakes in `IMAGE BROKEN` for every shared asset below + the root. +13. `feat(attachfile)`: page-scope an attachment with no recorded path. `--flat` + is unchanged and becomes the documented opt-out. +14. `feat(read)`: page-scoped positions, from the page's own slug. +15. `feat(attachment-download)`: page-scoped by default. The behaviour change to + a shipped command gets its own commit rather than riding inside an export + one, and it is not free: the command fetches only `ListAttachments` today + (`attachmentdownload.go:76`) and a slug needs the page's title, so this adds + a `GetPageOrNil`. If that fetch fails the run fails before anything is + written (`operationalFail`) -- half the attachments scoped and half not is + worse than none. A folder id, which `pageref.Resolve` accepts, has a title + too and scopes the same way. +16. `feat(export)`: `--depth`, the walk, the mirrored layout, `parent:` paths, + the `--file` refusal when multi-page, `Args` relaxed from `ExactArgs(1)` to + `MaximumNArgs(1)` for `--space`, and completion for both new flags + (`completion.Values` for `--depth`, `cobra.NoFileCompletions` for `--space`, + exactly as `cmd/children/children.go:57` does) -- without which + `TestSubcommandsCompleteArgs` in `cmd` fails -- + including the sentence in `fix`'s help that an exported `parent:` path is + reconciled back to a numeric id, which belongs with the commit that starts + emitting such paths rather than with a docs commit. +17. `feat(export)`: `--space`, requiring an explicit `--depth`. +18. `feat(export)`: `markfluence.yaml` at `dest`, and `roots`. +19. `feat(export)`: destination-conflict detection across pages. +20. `feat(export)`: the slug-collision pre-flight and its `-` suffixing. +21. `feat(export)`: skip a rendered page whose file already exists. +22. `feat(export)`: the multi-page summary, `parent_file`, and the schema. The + summary stops being the `map[string]int` at `export.go:311` and becomes a + typed struct, since `project_file` is not an int -- `Envelope.Summary` is + `any` (`jsonout.go:53`), so this costs nothing but must be said, and + `schematest`'s no-`omitempty`, every-field-typed rules apply to it. +23. `docs`: the tree feature. L5/L6 to Holds (gated on commit 24), README's + §`export` for `--depth`/`--space`/the layout/the suffix rule, its §`read` + and §`attachment-download` for the shared placement rule and `--flat`, + CLAUDE.md's `cmd/export`, `internal/pagetree` and + `attachment{list,upload,download}` paragraphs, and `pagedoc`'s package + comment -- where byte-identity is *narrowed* to a unique-slug page, not + retired. + + Code comments travel with their commits rather than landing here: + `attachname.go`'s file rationale in commit 1, `client.go:1062` in commit 5, + `attachfile.go`'s package comment and `Resolve` in commit 13, + `storage_to_md.go`'s `sourceFor` in commit 12. A doc commit that sweeps up + comments for code changed eight commits earlier is a doc commit nobody can + review. +24. `test(convert)`: the L5 property test the status flip rests on. Last, so it + is written against the finished behaviour, but it gates commit 23 rather + than decorating it -- if it cannot be made to pass, L5/L6 stay Partial and + commit 23 says so instead. + +## Verification + +Commits 14 and 15 change two shipped commands, so they get their own coverage +rather than riding on export's: a `read` output assertion showing an unsourced +attachment's destination is now page-scoped, and an `attachment-download` +path-set case covering both the scoped default and `--flat`. The live pass runs +all three commands against the fixture, since the point of the one-rule change +is that their outputs agree. + +A **path-set test** per fixture tree in `cmd/export` against a fake server: +assert the complete set of written paths, so a placement regression reads as a +diff of the tree rather than as one changed string. Plus the usual unit coverage +for the depth vocabulary, the pre-flight, the conflict rule, and `parent:` +emission. + +Then **live**, against the personal space (76646426): a standing three-level +fixture -- a root page, a child page under a folder, a grandchild; one native +attachment with no recorded path; one shared asset referenced from two pages at +the same recorded path; a deliberate near-collision pair of titles ("Deploy: +Prod" and "Deploy Prod", which must come out as two suffixed files rather than +one file or an error). Kept standing +so later work re-verifies against it. Folder children and attachment comment +shapes are exactly where fake servers have been wrong before. + +The end-to-end check is L5 itself: export the fixture subtree, then `update` it +back unedited and confirm no change -- which also proves the marker file is doing +its job, since without it every shared asset would come back `IMAGE BROKEN`. + +Expect one exception on the first cycle, or the verification reads as a failure: +an unsourced native attachment is unmanaged, so `meta.SHA256 ""` never equals a +real sum (`client.go:1052`) and the first `update` restamps it once with a +comment. Cycle two is the fixed point. "No change" is the claim about cycle two +onward, and the run that proves L5 is the second one. + +### The L5 property test (commit 24) + +A live pass proves the fixture round-trips; it does not prove the Law, and +`docs/guarantees.md` now says so explicitly. So the status flip needs a test +that generates rather than enumerates: storage in, `StorageToMarkdown`, then +`MdToConfluence`, asserting the storage that comes back is semantically what +went in. + +`internal/convert/storage_to_md_test.go` already has four narrower versions of +this shape to build on -- `TestRoundTripStableCallouts`, `TableAlignment`, +`TableCellBG`, and `TestRoundTripPassthrough`. The last one is the closest and +also the warning: `d86bec4` had to add a case to its hardcoded list because a +construct that existed was simply absent from it, which is the enumeration +failure #125 came through. The new test takes its corpus from the regression +suite's own cases rather than a hand-kept list, so a case added anywhere is +covered here by construction. + +Known exclusions belong in the test as named skips with reasons, not as silent +gaps: a table cell background outside the twenty-one swatches, and a column +alignment that is per-paragraph in storage but per-column in GFM. + +## Out of scope + +- **`--clean`**, for a tree whose pages were deleted upstream. Removal is bound by + **S4**-**S6**; making them non-vacuous is its own work. +- **Orphan cleanup** after the naming switch. Same reason, and it wants the same + spec. +- **A warning when a pre-existing local file differs from the recorded checksum.** + The local file is the user's working copy; export is not a sync tool. +- **Stripping `page_id`** to republish a tree elsewhere. +- **Concurrency.** From 825517b814780035ecfa0c1b2d8b1b0ca0a36e73 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 07:45:38 -0400 Subject: [PATCH 02/34] feat(convert): name an attachment by its base name The name is the attachment's identity -- Confluence matches ri:filename literally, and planAttachments decides create-vs-update by name -- so an encoded name moves whenever the path moves, and a moved name is a new attachment with the old one orphaned. Exporting a page positions its images, which moves paths; so does reorganising a repository. The path is not lost, because it is recorded in the attachment's comment since 026 commit 7, which is authoritative where a decoded name was only ever a guess about a name markfluence might not have written. What the encoding did buy is collision-freedom, and that becomes an explicit refusal in the next commit. localAttachments stops round-tripping the source through the name for the same reason: decoding a base name back into a source would record "x.png" for an asset at "docs/assets/x.png". The invariant inverts rather than disappearing -- the stored name is the base name of the recorded source -- and its test says so. decodeName went with it, having no callers left. The five affected regression goldens are regenerated here rather than in a later sweep: a commit that leaves the suite red is a commit nobody can bisect through. Refs #59. --- cmd/attachmentupload/attachmentupload.go | 12 +-- cmd/attachmentupload/attachmentupload_test.go | 67 +++++++------- internal/convert/attachname.go | 58 ++++++++---- internal/convert/attachname_test.go | 88 ++++++++----------- .../regression/image-properties/test.output | 4 +- .../regression/images-encoded-src/test.output | 10 +-- .../regression/images-local/test.output | 4 +- .../images-shared-parent/test.output | 4 +- .../regression/kitchen-sink/test.output | 4 +- 9 files changed, 130 insertions(+), 121 deletions(-) diff --git a/cmd/attachmentupload/attachmentupload.go b/cmd/attachmentupload/attachmentupload.go index 1e51450..11de1fd 100644 --- a/cmd/attachmentupload/attachmentupload.go +++ b/cmd/attachmentupload/attachmentupload.go @@ -170,10 +170,10 @@ func localAttachments(files []string, name string, roots *project.Cache) ([]clie if filename == "" { return nil, fmt.Errorf("%q is not a usable attachment name", source) } - // Round-trip the name so source is exactly what a decode yields. - if decoded, ok := convert.AttachmentSource(filename); ok { - source = decoded - } + // source is recorded as given, not as a decode of the name. The name is + // now the base name, so decoding it back would throw the path away and + // record "x.png" for an asset at "docs/assets/x.png" -- which is the one + // copy of the path there is. out = append(out, client.LocalAttachment{Path: f, Filename: filename, Source: source}) } return out, nil @@ -268,7 +268,3 @@ func failEnvelope(pageID string, err error, code jsonout.Code, roots *project.Ca env.Roots = roots.Roots() return env } - -// decodeName is convert.AttachmentSource, wrapped so tests can assert the -// lockstep invariant without importing the converter. -func decodeName(filename string) (string, bool) { return convert.AttachmentSource(filename) } diff --git a/cmd/attachmentupload/attachmentupload_test.go b/cmd/attachmentupload/attachmentupload_test.go index f6509c0..646e191 100644 --- a/cmd/attachmentupload/attachmentupload_test.go +++ b/cmd/attachmentupload/attachmentupload_test.go @@ -2,6 +2,7 @@ package attachmentupload import ( "os" + "path" "path/filepath" "testing" @@ -23,9 +24,9 @@ func writeFile(t *testing.T, dir, name string) string { func TestLocalAttachmentsUsesBaseName(t *testing.T) { dir := t.TempDir() - path := writeFile(t, dir, "docs/assets/x.png") + file := writeFile(t, dir, "docs/assets/x.png") - got, err := localAttachments([]string{path}, "", project.NewCache("")) + got, err := localAttachments([]string{file}, "", project.NewCache("")) if err != nil { t.Fatal(err) } @@ -38,8 +39,8 @@ func TestLocalAttachmentsUsesBaseName(t *testing.T) { if got[0].Source != "x.png" { t.Errorf("source = %q, want x.png", got[0].Source) } - if got[0].Path != path { - t.Errorf("path = %q, want %q", got[0].Path, path) + if got[0].Path != file { + t.Errorf("path = %q, want %q", got[0].Path, file) } } @@ -50,60 +51,62 @@ func TestLocalAttachmentsUsesBaseName(t *testing.T) { // would record (internal/convert/images.go). func TestLocalAttachmentsSourceIsRootRelative(t *testing.T) { root := t.TempDir() - path := writeFile(t, root, "docs/assets/x.png") + file := writeFile(t, root, "docs/assets/x.png") - got, err := localAttachments([]string{path}, "", project.NewCache(root)) + got, err := localAttachments([]string{file}, "", project.NewCache(root)) if err != nil { t.Fatal(err) } if want := "docs/assets/x.png"; got[0].Source != want { t.Errorf("source = %q, want %q", got[0].Source, want) } - if want := "docs%2Fassets%2Fx.png"; got[0].Filename != want { + if want := "x.png"; got[0].Filename != want { t.Errorf("filename = %q, want %q", got[0].Filename, want) } } -// TestLocalAttachmentsNameEncodesPath is the point of --name taking a path: the -// user writes a path and markfluence produces the attachment a publish of -// ![](assets/x.png) would resolve to, without them typing an escape. -func TestLocalAttachmentsNameEncodesPath(t *testing.T) { +// TestLocalAttachmentsNameTakesAPath is the point of --name taking a path: the +// user writes the path the markdown uses and markfluence produces the +// attachment a publish of ![](assets/x.png) would resolve to -- the base name, +// with the path itself kept as the recorded source. +func TestLocalAttachmentsNameTakesAPath(t *testing.T) { dir := t.TempDir() - path := writeFile(t, dir, "somewhere/else.png") + file := writeFile(t, dir, "somewhere/else.png") - got, err := localAttachments([]string{path}, "assets/x.png", project.NewCache("")) + got, err := localAttachments([]string{file}, "assets/x.png", project.NewCache("")) if err != nil { t.Fatal(err) } - if got[0].Filename != "assets%2Fx.png" { - t.Errorf("filename = %q, want assets%%2Fx.png", got[0].Filename) + if got[0].Filename != "x.png" { + t.Errorf("filename = %q, want x.png", got[0].Filename) } if got[0].Source != "assets/x.png" { t.Errorf("source = %q, want assets/x.png", got[0].Source) } } -// TestLocalAttachmentsSourceIsAlwaysTheDecodedName is the lockstep invariant: -// if the recorded path and the stored name could disagree, a later publish -// would upload a second attachment while a restoring download put this one -// where the markdown never references it. -func TestLocalAttachmentsSourceIsAlwaysTheDecodedName(t *testing.T) { +// TestLocalAttachmentsNameIsTheSourcesBaseName is the lockstep invariant, in +// the form the basename scheme gives it: the stored name is exactly the base +// name of the recorded source. If the two could disagree, a later publish would +// upload a second attachment while a restoring download put this one where the +// markdown never references it. +// +// It replaces an invariant stated the other way round -- that the source is +// always a decode of the name -- which held only while the name carried the +// whole path. Decoding a base name back into a source would discard the path, +// and the comment is now the only place it is written down. +func TestLocalAttachmentsNameIsTheSourcesBaseName(t *testing.T) { dir := t.TempDir() - path := writeFile(t, dir, "f.png") + file := writeFile(t, dir, "f.png") for _, name := range []string{"", "assets/x.png", "./a/./b.png", "../shared/logo.png", "plain.png"} { - got, err := localAttachments([]string{path}, name, project.NewCache("")) + got, err := localAttachments([]string{file}, name, project.NewCache("")) if err != nil { t.Fatalf("--name %q: %v", name, err) } - decoded, ok := decodeName(got[0].Filename) - if !ok { - t.Errorf("--name %q: stored name %q does not decode", name, got[0].Filename) - continue - } - if decoded != got[0].Source { - t.Errorf("--name %q: source %q != decode of %q (%q)", - name, got[0].Source, got[0].Filename, decoded) + if want := path.Base(got[0].Source); got[0].Filename != want { + t.Errorf("--name %q: stored name %q is not the base name of source %q (%q)", + name, got[0].Filename, got[0].Source, want) } } } @@ -117,9 +120,9 @@ func TestLocalAttachmentsSourceIsAlwaysTheDecodedName(t *testing.T) { func TestLocalAttachmentsRejectsRootEscape(t *testing.T) { unrelatedRoot := t.TempDir() fileDir := t.TempDir() - path := writeFile(t, fileDir, "x.png") + file := writeFile(t, fileDir, "x.png") - if _, err := localAttachments([]string{path}, "", project.NewCache(unrelatedRoot)); err == nil { + if _, err := localAttachments([]string{file}, "", project.NewCache(unrelatedRoot)); err == nil { t.Error("want an error when --root does not contain the file") } } diff --git a/internal/convert/attachname.go b/internal/convert/attachname.go index e190a87..c2a3a27 100644 --- a/internal/convert/attachname.go +++ b/internal/convert/attachname.go @@ -1,23 +1,35 @@ package convert // attachname.go owns the mapping between a markdown image's source path and the -// Confluence attachment name it is published under. +// Confluence attachment name it is published under: the name is the path's base +// name. // -// "/" 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. +// "/" is not legal in an attachment name, so a path cannot be a name and +// something has to give. This used to flatten the whole path by percent-encoding +// it, which was bijective -- distinct paths always produced distinct names, and +// a name decoded back to the path it came from. Two things paid for that, and +// both were worth more than it was. // -// 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. +// The name is the attachment's identity: Confluence matches ri:filename +// literally, and planAttachments decides create-vs-update by name. So an +// encoded name moves whenever the path moves, and a moved name is a new +// attachment with the old one orphaned. Exporting a page positions its images, +// which moves paths; so does reorganising a repository. Neither should churn +// what is stored on the page. // -// The codec is exported because the attachment subcommands share it: upload -// encodes a --name path, and download decodes a stored name. It lives here -// rather than in its own package because the converter's image handling is what -// defines the mapping. +// The path is not lost by dropping the encoding, because it is recorded in the +// attachment's comment (client.attachmentComment, since 026 commit 7). That is +// where `read` and `export` recover an image's original location from, and it +// is authoritative where a decoded name was only ever a guess about a name +// markfluence might not have written. +// +// What the encoding did buy is collision-freedom, and that is now an explicit +// refusal: two assets under one page whose base names agree are reported by +// images.go rather than silently given one name. See _plans/029. +// +// Exported because the attachment subcommands share it -- upload derives a name +// from a --name path. It lives here rather than in its own package because the +// converter's image handling is what defines the mapping. import ( "path" @@ -30,13 +42,21 @@ const ( ) // 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. +// src: the base name of the file, and nothing else. The src is normalized first +// so that "a/./x.png" and "a/x.png" agree, and so a name is never derived from a +// path that was absolute. +// +// The mapping is deliberately lossy, and the loss is what makes it usable. Two +// assets under one page can want the same name -- "arch/diagram.png" and +// "deploy/diagram.png" -- and that is refused where it is detected, in +// images.go, rather than encoded around here. func AttachmentFilename(src string) string { rel := normalizeSrc(src) - rel = strings.ReplaceAll(rel, "%", pctEscape) - return strings.ReplaceAll(rel, "/", pctSlash) + if rel == "" { + // path.Base would answer "." for this; an empty src has no name. + return "" + } + return path.Base(rel) } // AttachmentSource inverts AttachmentFilename, recovering the source path an diff --git a/internal/convert/attachname_test.go b/internal/convert/attachname_test.go index 3f90110..9183ac0 100644 --- a/internal/convert/attachname_test.go +++ b/internal/convert/attachname_test.go @@ -2,68 +2,58 @@ 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) { +// TestAttachmentFilename pins the mapping: the name is the path's base name. +func TestAttachmentFilename(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"}, + {"assets/x.png", "x.png"}, + {"a/b/c/deep.png", "deep.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"}, + // Nothing in the name is escaped any more, so characters that used to be + // the encoding's business are ordinary text. + {"a%2Fb.png", "a%2Fb.png"}, + {"100%.png", "100%.png"}, + {"%25.png", "%25.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, and its name is + // the file's, not the route taken to it. + {"../assets/logo.png", "logo.png"}, - // A shared asset above the page is a supported layout. - {"../assets/logo.png", "..%2Fassets%2Flogo.png"}, + {"my docs/a b.png", "a b.png"}, - // Spaces and other characters are left alone -- only "/" is illegal. - {"my docs/a b.png", "my docs%2Fa b.png"}, + // No path, no name. + {"", ""}, + {".", ""}, } 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) +// TestAttachmentFilenameCollidesOnBaseName documents the mapping's deliberate +// loss, so that a future reader finds it stated rather than discovers it. +// +// The old percent-encoding was injective and this is not: two assets in +// different directories share a name. That is refused where it can be seen with +// both paths in hand -- renderImage, for one page -- rather than designed around +// here, because the name is the attachment's identity and an encoded name churns +// every time a path moves. See _plans/029. +func TestAttachmentFilenameCollidesOnBaseName(t *testing.T) { + for _, pair := range [][2]string{ + {"arch/diagram.png", "deploy/diagram.png"}, + {"x.png", "assets/x.png"}, + {"assets/x.png", "../x.png"}, + } { + a, b := AttachmentFilename(pair[0]), AttachmentFilename(pair[1]) + if a != b { + t.Errorf("AttachmentFilename(%q) = %q and (%q) = %q; expected them to collide", + pair[0], a, pair[1], b) } - seen[name] = src } } @@ -72,12 +62,12 @@ func TestAttachmentFilenameIsInjective(t *testing.T) { 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"}, + {"./assets/x.png", "x.png"}, + {"assets/./x.png", "x.png"}, + {"assets/../assets/x.png", "x.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"}, + // really absolute. + {"/assets/x.png", "x.png"}, } for _, c := range cases { if got := AttachmentFilename(c.src); got != c.want { diff --git a/internal/convert/testdata/regression/image-properties/test.output b/internal/convert/testdata/regression/image-properties/test.output index 8a4b542..9b12529 100644 --- a/internal/convert/testdata/regression/image-properties/test.output +++ b/internal/convert/testdata/regression/image-properties/test.output @@ -1,13 +1,13 @@ { "attachments": [ { - "filename": "assets%2Fshot.png", + "filename": "shot.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": [ "line 13: assets/shot.png: ignoring width='wide' (must be a number)", "line 13: assets/shot.png: ignoring align='middle' (must be left, center, or right)" diff --git a/internal/convert/testdata/regression/images-encoded-src/test.output b/internal/convert/testdata/regression/images-encoded-src/test.output index 079a198..7e82bd9 100644 --- a/internal/convert/testdata/regression/images-encoded-src/test.output +++ b/internal/convert/testdata/regression/images-encoded-src/test.output @@ -1,22 +1,22 @@ { "attachments": [ { - "filename": "assets%2Fmy image.png", + "filename": "my image.png", "path": "/assets/my image.png", "source": "assets/my image.png" }, { - "filename": "assets%2Fcafé.png", + "filename": "café.png", "path": "/assets/café.png", "source": "assets/café.png" }, { - "filename": "assets%2F100%25.png", + "filename": "100%.png", "path": "/assets/100%.png", "source": "assets/100%.png" }, { - "filename": "assets%2Fplain.png", + "filename": "plain.png", "path": "/assets/plain.png", "source": "assets/plain.png" } @@ -24,6 +24,6 @@ "broken": [ "line 37: IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.png (outside the documentation root)" ], - "html": "

Encoded Image Sources

\n

A markdown image destination is a URL, not a path, so a filename with a space has to be percent-encoded to be referenced at all. This is the spelling most editors and previews produce, and the one this case exists for:

\n

\n

The angle-bracket form is the same image by another spelling, and resolves to the same attachment:

\n

\n

A bare space is not a valid destination, so this is not an image at all -- it stays literal text, exactly as GitHub and a local preview render it. Nothing is uploaded and nothing is reported broken, because no image was ever parsed:

\n

![bare space](assets/my image.png)

\n

Non-ASCII filenames encode the same way:

\n

\n

A literal "%" in a filename is not an escape sequence. It is left as written, so a file genuinely named "100%.png" still resolves:

\n

\n

An ordinary path is unaffected:

\n

\n

An encoded "../" is decoded before the documentation-root check runs, so the encoding cannot slip an escaping path past it -- compare the plain spelling in the images-broken case, which is refused for the same reason:

\n

line 37: IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.png (outside the documentation root)

\n", + "html": "

Encoded Image Sources

\n

A markdown image destination is a URL, not a path, so a filename with a space has to be percent-encoded to be referenced at all. This is the spelling most editors and previews produce, and the one this case exists for:

\n

\n

The angle-bracket form is the same image by another spelling, and resolves to the same attachment:

\n

\n

A bare space is not a valid destination, so this is not an image at all -- it stays literal text, exactly as GitHub and a local preview render it. Nothing is uploaded and nothing is reported broken, because no image was ever parsed:

\n

![bare space](assets/my image.png)

\n

Non-ASCII filenames encode the same way:

\n

\n

A literal "%" in a filename is not an escape sequence. It is left as written, so a file genuinely named "100%.png" still resolves:

\n

\n

An ordinary path is unaffected:

\n

\n

An encoded "../" is decoded before the documentation-root check runs, so the encoding cannot slip an escaping path past it -- compare the plain spelling in the images-broken case, which is refused for the same reason:

\n

line 37: IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.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 cc5d07a..b557f43 100644 --- a/internal/convert/testdata/regression/images-local/test.output +++ b/internal/convert/testdata/regression/images-local/test.output @@ -1,12 +1,12 @@ { "attachments": [ { - "filename": "assets%2Fdiagram.png", + "filename": "diagram.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/test.output b/internal/convert/testdata/regression/images-shared-parent/test.output index 33fca76..67db0bb 100644 --- a/internal/convert/testdata/regression/images-shared-parent/test.output +++ b/internal/convert/testdata/regression/images-shared-parent/test.output @@ -1,12 +1,12 @@ { "attachments": [ { - "filename": "assets%2Flogo.png", + "filename": "logo.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", + "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 89ff21b..6abc5c2 100644 --- a/internal/convert/testdata/regression/kitchen-sink/test.output +++ b/internal/convert/testdata/regression/kitchen-sink/test.output @@ -1,12 +1,12 @@ { "attachments": [ { - "filename": "assets%2Flogo.png", + "filename": "logo.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": [] } From 39bcd437210a195030cf87f01d4ad12e5017e167 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:04:24 -0400 Subject: [PATCH 03/34] docs(plans): sequence 029 into commits, checkpoints, and reviews 029 says what to build and why; it does not say what lands in what order, where the work is reviewed, or what has already turned out differently while building it. Splitting those apart keeps 029 a design document rather than a log that has to be rewritten every time an ordering assumption fails. The standing rule is that every commit passes make check on its own, which has already moved work earlier three times in commit 1 alone. Six reviews across twenty-four commits: four checkpoints where a complete piece of behaviour first exists, one solo review of the pagedoc position commit because its failure mode is silent, and one security review once page titles have become directory names -- an untrusted input the clamp's reasoning was never written against. 029's last two commits are swapped here: a property test that gates a docs commit has to land before it, or the gate can only be enforced by editing history. Refs #59. --- _plans/031_recursive-export-implementation.md | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 _plans/031_recursive-export-implementation.md diff --git a/_plans/031_recursive-export-implementation.md b/_plans/031_recursive-export-implementation.md new file mode 100644 index 0000000..bef172c --- /dev/null +++ b/_plans/031_recursive-export-implementation.md @@ -0,0 +1,132 @@ +# Plan: implementing 029 — sequence, checkpoints, reviews + +`_plans/029` is the design: what recursive export does, why an attachment is +named by its base name, and what each decision costs. This file owns only the +order the work lands in, where it is reviewed, and what has already diverged +from the plan while building it. When the two disagree about *what* something +does, 029 wins; when they disagree about *when*, this one does. + +## The standing rule + +**Every commit passes `make check` on its own.** Not the branch tip -- each +commit, so the history can be bisected and each step reviewed as a working +state. This has already forced one reordering (see Divergences), and it will +force more: when a commit breaks something a later commit was scheduled to fix, +the fix moves earlier rather than the suite going red. + +## Sequence + +### Naming (commits 1-10) + +The widest blast radius on the branch: it changes the attachment name on every +page markfluence has ever published. + +| # | commit | notes | +|---|---|---| +| 1 | `feat(convert)`: name an attachment by its base name | **done** | +| 2 | `feat(convert)`: refuse two same-base-name assets in one file | compares `rootRel`, not the raw src; includes the raw-storage `ri:filename` scan | +| 3 | `feat(check)`: report a base-name collision offline | no network needed to see it | +| 4 | `refactor(attachmentupload)`: refuse a collision across a batch of FILEs | shrunk; see Divergences | +| 5 | `refactor(convert)`: stop interpreting a stored name | deletes `AttachmentSource`, rewrites `TestRoundTripEncodedImageSources`, fixes `client.go:1062` | +| 6 | `test`: hand-authored fixtures and case renames | shrunk; see Divergences | +| 7 | `docs(confluence)`: `attachments.md`'s framing | the Verified probes stay | +| 8 | `docs`: L3's note, R2's note, `root-model.md:114` | R2's **label** is unchanged | +| 9 | `docs`: README and CLAUDE.md | including the now-void `--attachments-dir` rationale | +| 10 | `docs(plans)`: a correction note on 025 | | + +**Checkpoint A** — `/code-review` over commits 1-10, plus an amendment to this +file recording anything else that diverged. + +### Placement (commits 11-15) + +Where **L5** and the traversal clamp both live. + +| # | commit | +|---|---| +| 11 | `refactor`: `slugify` moves out of `cmd/export` | +| 12 | `feat(pagedoc)`: the page's position through `Options`/`StorageOptions` | +| 13 | `feat(attachfile)`: page-scope an attachment with no recorded path; `--flat` opts out | +| 14 | `feat(read)`: page-scoped positions | +| 15 | `feat(attachment-download)`: page-scoped by default, with the `GetPageOrNil` a slug needs | + +**Commit 12 is reviewed on its own**, mid-stretch, because its failure mode is +silent: a wrong position compiles, passes the suite, and breaks L5 in a way only +a live round-trip shows. Every other commit here fails loudly. + +**Checkpoint B** — `/code-review` over commits 11-15. + +### Export (commits 16-22) + +| # | commit | +|---|---| +| 16 | `feat(export)`: `--depth`, the walk, the mirrored layout, `parent:` paths, the `--file` refusal, `Args`, completion, `fix`'s help | +| 17 | `feat(export)`: `--space` | +| 18 | `feat(export)`: `markfluence.yaml` at `dest`, and `roots` | +| 19 | `feat(export)`: destination-conflict detection across pages | +| 20 | `feat(export)`: the slug pre-flight and its `-` suffixing | +| 21 | `feat(export)`: skip a rendered page whose file already exists | +| 22 | `feat(export)`: the multi-page summary, `parent_file`, the schema | + +**Checkpoint C** — `/code-review` over commits 16-22, then **`/security-review` +over the whole branch**. + +Security waits until here rather than running earlier, because the thing worth +reviewing does not exist until then: this feature turns **page titles into +directory names** and attachment names into files beneath them, and that span +crosses commits 11-22. `slugify` drops `/` so a title cannot traverse, and +`attachfile` has both the lexical clamp and `os.Root` -- but every line of that +reasoning was written against attachment *comments* as the untrusted input, and +a title has never been audited as one. + +**Live verification** follows immediately, against the personal space +(76646426): the standing three-level fixture 029 §Verification describes. Before +the closing commits, not after, because a finding here changes code rather than +prose. + +### Close (commits 23-24) + +| # | commit | +|---|---| +| 23 | `test(convert)`: the L5 property test | +| 24 | `docs`: the tree feature, L5/L6 to Holds, README, CLAUDE.md, `pagedoc`'s package comment | + +**These are 029's commits 23 and 24 in the opposite order**, deliberately. 029 +has the docs commit flipping L5/L6 to Holds and the property test after it, +"gating" it -- a gate that can only be enforced by going back and editing the +previous commit. Landing the test first means the docs commit cites something +that already passes, and if the test cannot be made to pass, the docs commit +simply says Partial and no history needs rewriting. + +**Checkpoint D** — whole-branch `/code-review`, then the PR. + +## Review policy, and why not more + +Six reviews across twenty-four commits: four checkpoints, one solo review of +commit 12, one security pass. + +Reviewing every commit was considered and rejected. Most of these commits are +mechanical -- regenerated goldens, a moved function, prose -- and a review finds +little in them. What has actually been productive on this work is reviewing a +*whole coherent artifact*: two reviews of 029 each found a design flaw that no +per-commit reading would have surfaced, because both were about how pieces +interact. The checkpoints are placed where a complete piece of behaviour first +exists. + +## Divergences from 029 + +Recorded as they happen, so 029 stays the design and this stays the log. + +1. **Commit 1 absorbed the five regenerable goldens** that 029 scheduled for + commit 6. Changing the naming scheme changes them immediately, and the + standing rule does not allow five commits of red suite in between. Commit 6 + keeps the hand-authored `storage2md` inputs and the `images-encoded-src` + renames. +2. **Commit 1 absorbed `localAttachments`' source fix** from commit 4. It + round-tripped `Source` through the attachment name, so a base name silently + recorded `path=x.png` for an asset at `docs/assets/x.png` -- destroying the + one copy of the path the whole scheme now depends on. 029 did not notice this + call site. Commit 4 keeps the in-batch refusal. +3. **`decodeName` was deleted in commit 1**, not commit 5: it wrapped + `AttachmentSource` for a test asserting the invariant that inverted, and an + unused function fails `lint`, which the standing rule does not permit. +4. **Commits 23 and 24 are swapped**, per the Close section above. From 2a07e1de0857e5f50219e45558019645bdeccff9 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:09:55 -0400 Subject: [PATCH 04/34] feat(convert): refuse two assets that want one attachment name An attachment name is unique per page, so two images whose base names agree cannot both be published: one upload would overwrite the other, the page would show a single image in both places, and only one of the two paths would be recorded. There is no correct output, so there is no output -- MdToConfluence returns an error and the file fails. Deliberately not a Broken entry, which is what 029 called for. Nothing blocks a publish on Broken (cmd/update appends it to the report and carries on), so reporting it that way would put the page up wrong and say so afterwards. The message names both paths and both lines, since "diagram.png is ambiguous" leaves the author hunting for the second one. seen now records the source path each name was claimed by, rather than the bare fact of a claim. That is what keeps the same image referenced twice a dedupe -- one upload, several references -- while two different assets are a collision. A pasted ri:filename gets a warning instead. The shield renames raw storage tags before goldmark, so renderImage never sees such a reference and cannot refuse it in the same breath; and unlike two images, a pasted reference may legitimately mean the very attachment being published. But it is not silent: publishing rebinds it, so a part of the page the author never edited starts displaying something else. Refs #59. --- internal/convert/convert.go | 3 + internal/convert/images.go | 64 ++++++++++- internal/convert/namecollision_test.go | 146 +++++++++++++++++++++++++ internal/convert/renderer.go | 20 +++- 4 files changed, 229 insertions(+), 4 deletions(-) create mode 100644 internal/convert/namecollision_test.go diff --git a/internal/convert/convert.go b/internal/convert/convert.go index 0720da0..1116833 100644 --- a/internal/convert/convert.go +++ b/internal/convert/convert.go @@ -83,6 +83,9 @@ func MdToConfluence( // a reported line matches what a reader sees opening the file, not // what the parser sees after Extract already removed the header. lineOffset: strings.Count(md.Content[:len(md.Content)-len(md.Body)], "\n"), + // Scanned from the unshielded body: after shielding, the tag names are + // sentinels and the attribute would no longer match. + pastedNames: pastedAttachmentNames(md.Body), } var buf bytes.Buffer if err := newMarkdown(r).Convert([]byte(shielded), &buf); err != nil { diff --git a/internal/convert/images.go b/internal/convert/images.go index 0c31e94..5084141 100644 --- a/internal/convert/images.go +++ b/internal/convert/images.go @@ -6,6 +6,7 @@ import ( "html" "os" "path/filepath" + "regexp" "strconv" "strings" @@ -102,11 +103,43 @@ func (r *storageRenderer) renderImage( default: filename := AttachmentFilename(rootRel) - if !r.seen[filename] { + prev, claimed := r.seen[filename] + switch { + case claimed && prev.source != rootRel: + // Two different assets, one attachment name. Refused rather than + // reported, because there is no correct way to publish it: an + // attachment name is unique per page, so one upload would overwrite + // the other and the page would show a single image in both places + // while recording a single path. Broken would not do -- nothing + // blocks a publish on it (see cmd/update), so the page would go up + // wrong. + // + // Comparing source paths and not just the name is what keeps the + // same image referenced twice a dedupe rather than a collision. + return ast.WalkStop, fmt.Errorf( + "%s%s and %s%s both publish as the attachment %q; rename one of them", + prev.line, prev.source, r.linePrefix(node, source), rootRel, filename) + + case claimed: + // The same asset again: one upload, two references. + + default: if r.seen == nil { - r.seen = map[string]bool{} + r.seen = map[string]claimedName{} + } + r.seen[filename] = claimedName{source: rootRel, line: r.linePrefix(node, source)} + if r.pastedNames[filename] { + // Raw storage in this same body already points at this name, and + // the shield means renderImage never sees it. Publishing rebinds + // that reference to this asset -- a part of the page the author + // did not edit now shows a different image. Warned rather than + // refused: unlike two images, a pasted reference is a reference + // and may well mean this very attachment. + r.warnings = append(r.warnings, fmt.Sprintf( + "%s%s publishes as the attachment %q, which raw storage in this "+ + "page already references", + r.linePrefix(node, source), rootRel, filename)) } - r.seen[filename] = true r.attachments = append(r.attachments, Attachment{ Filename: filename, Path: filepath.Join(r.root.Dir, rootRel), Source: rootRel, }) @@ -162,6 +195,31 @@ func (r *storageRenderer) parseImageTitle(prefix, titleRaw, src string) map[stri return attrs } +// pastedRiFilenameRE matches an ri:filename attribute in a markdown body -- raw +// storage the author pasted, which the shield hands through untouched. +var pastedRiFilenameRE = regexp.MustCompile(`ri:filename="([^"]*)"`) + +// pastedAttachmentNames is the set of attachment names raw storage in body +// already refers to. +// +// renderImage cannot discover these for itself: shieldStorage renames the tags +// before goldmark sees them, so a pasted reference is never an *ast.Image and +// never reaches the naming branch. Matching the literal attribute is exact -- +// it is the same string Confluence resolves -- and matching it anywhere rather +// than only inside ac:image is deliberate, for the reason cmd/export's own scan +// gives: a link target or a macro-internal reference counts too. +func pastedAttachmentNames(body string) map[string]bool { + m := pastedRiFilenameRE.FindAllStringSubmatch(body, -1) + if len(m) == 0 { + return nil + } + names := make(map[string]bool, len(m)) + for _, g := range m { + names[html.UnescapeString(g[1])] = true + } + return names +} + // acImage builds an referencing an attachment (riFilename) or a URL // (riURL). Attribute values are XML-escaped. func acImage(alt string, attrs map[string]string, riFilename, riURL string) string { diff --git a/internal/convert/namecollision_test.go b/internal/convert/namecollision_test.go new file mode 100644 index 0000000..f3e20b4 --- /dev/null +++ b/internal/convert/namecollision_test.go @@ -0,0 +1,146 @@ +package convert_test + +// Two assets in one file wanting one attachment name. The name is the base name +// (attachname.go), so this is reachable whenever two directories hold a +// same-named image -- and it is refused rather than reported, because there is +// no correct way to publish it. + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/convert" + "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/project" +) + +// convertBody renders body as root/main.md, returning the page or the error. +func convertBody(t *testing.T, root, body string, images ...string) (*convert.ConfluencePage, error) { + t.Helper() + for _, img := range images { + path := filepath.Join(root, filepath.FromSlash(img)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("PNG"), 0o644); err != nil { + t.Fatal(err) + } + } + md, err := frontmatter.Parse(filepath.Join(root, "main.md"), body) + if err != nil { + t.Fatal(err) + } + r, err := project.FromPath(root) + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.FS.Close() }() + return convert.MdToConfluence(md, r, testIndex(t, r), "https://wiki.example.net", "ENG", "vtest") +} + +// TestRefusesTwoAssetsWithOneName is the refusal itself. It names both paths, +// with the line each was written on, because "diagram.png is ambiguous" would +// leave the author hunting for the second one. +func TestRefusesTwoAssetsWithOneName(t *testing.T) { + root := t.TempDir() + body := "intro\n\n![arch](arch/diagram.png)\n\nmore\n\n![deploy](deploy/diagram.png)\n" + + page, err := convertBody(t, root, body, "arch/diagram.png", "deploy/diagram.png") + if err == nil { + t.Fatalf("want a refusal, got a page: %s", page.HTML) + } + for _, want := range []string{ + "arch/diagram.png", "deploy/diagram.png", `"diagram.png"`, "line 3: ", "line 7: ", + } { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + +// TestRefusalIsNotMerelyBroken guards the distinction the whole refusal rests +// on: Broken is reported and published anyway (nothing in cmd/update blocks on +// it), so a collision reported that way would put the page up with one image +// rendered twice and one of the two paths recorded. +func TestRefusalIsNotMerelyBroken(t *testing.T) { + root := t.TempDir() + body := "![a](a/x.png)\n\n![b](b/x.png)\n" + + if _, err := convertBody(t, root, body, "a/x.png", "b/x.png"); err == nil { + t.Fatal("a name collision must fail the conversion, not report a Broken entry") + } +} + +// TestSameAssetTwiceIsOneAttachment is the case the refusal must not catch: one +// image referenced twice is one upload, which is why seen compares source paths +// rather than only names. +func TestSameAssetTwiceIsOneAttachment(t *testing.T) { + root := t.TempDir() + body := "![one](assets/x.png)\n\n![again](assets/x.png)\n\n![spelled](./assets/x.png)\n" + + page, err := convertBody(t, root, body, "assets/x.png") + if err != nil { + t.Fatalf("MdToConfluence: %v", err) + } + if len(page.Attachments) != 1 { + t.Fatalf("attachments = %v, want exactly one", page.Attachments) + } + if got := page.Attachments[0].Filename; got != "x.png" { + t.Errorf("filename = %q, want x.png", got) + } + if got := strings.Count(page.HTML, `ri:filename="x.png"`); got != 3 { + t.Errorf("body references the attachment %d times, want 3", got) + } +} + +// TestSameNameInDifferentFilesIsFine keeps the refusal scoped to one page. +// Attachment names are unique per page, so two pages may each carry a +// diagram.png -- and a renderer is built per conversion, so nothing leaks. +func TestSameNameInDifferentFilesIsFine(t *testing.T) { + root := t.TempDir() + for _, src := range []string{"arch/diagram.png", "deploy/diagram.png"} { + if _, err := convertBody(t, root, "![d]("+src+")\n", src); err != nil { + t.Errorf("converting %s alone: %v", src, err) + } + } +} + +// TestWarnsWhenAConvertedImageTakesAPastedName covers the reference the shield +// hides from renderImage: raw storage naming an attachment that a converted +// image is about to publish over. Warned, not refused -- a pasted reference may +// legitimately mean this very attachment -- but not silent, since publishing +// changes what a part of the page the author never edited displays. +func TestWarnsWhenAConvertedImageTakesAPastedName(t *testing.T) { + root := t.TempDir() + body := "\n\n" + + "![arch](arch/diagram.png)\n" + + page, err := convertBody(t, root, body, "arch/diagram.png") + if err != nil { + t.Fatalf("MdToConfluence: %v", err) + } + if len(page.Warnings) != 1 || !strings.Contains(page.Warnings[0], "raw storage") { + t.Fatalf("warnings = %v, want one naming the raw-storage reference", page.Warnings) + } + if !strings.Contains(page.Warnings[0], "arch/diagram.png") { + t.Errorf("warning %q does not name the image's path", page.Warnings[0]) + } +} + +// TestNoWarningWhenPastedNamesDoNotCollide keeps the scan from firing on the +// ordinary case: a page mixing pasted storage and images that share no name. +func TestNoWarningWhenPastedNamesDoNotCollide(t *testing.T) { + root := t.TempDir() + body := "\n\n" + + "![arch](arch/diagram.png)\n" + + page, err := convertBody(t, root, body, "arch/diagram.png") + if err != nil { + t.Fatalf("MdToConfluence: %v", err) + } + if len(page.Warnings) != 0 { + t.Errorf("warnings = %v, want none", page.Warnings) + } +} diff --git a/internal/convert/renderer.go b/internal/convert/renderer.go index d63e9cf..8324eb3 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -46,7 +46,18 @@ type storageRenderer struct { attachments []Attachment broken []string warnings []string - seen map[string]bool + // seen maps an attachment name to the image that claimed it. It records the + // source path, not just the fact of a claim, because the name is now the + // base name: a second image reaching the same name is either the same asset + // again (deduped) or a different asset that cannot be published alongside it + // (refused), and only the path tells them apart. + seen map[string]claimedName + + // pastedNames are the attachment names referenced by raw ri:filename in the + // body -- storage the shield passes through untouched, which renderImage + // therefore never sees. A converted image landing on one of these rebinds a + // reference the author did not touch, which is worth a warning. + pastedNames map[string]bool // linkBrokenText is the literal replacement text for the *ast.Link // currently being rendered, set on entering when its target is Broken and @@ -101,6 +112,13 @@ func (r *storageRenderer) linePrefix(n ast.Node, source []byte) string { return "" } +// claimedName is an image that has taken an attachment name, kept so a later +// image reaching the same name can be reported against it by path and line. +type claimedName struct { + source string + line string // a "line N: " prefix, or "" when the position is unknown +} + // RegisterFuncs registers the node handlers this renderer overrides. func (r *storageRenderer) RegisterFuncs(reg renderer.NodeRendererFuncRegisterer) { reg.Register(ast.KindText, r.renderText) From 3d155f1806691f118f3b3a2e30f10eedcc18bc4a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:11:38 -0400 Subject: [PATCH 05/34] feat(check): report a name collision as a document defect A collision fails the conversion, and check's existing handling of a conversion failure is to report the file as failed with code CONVERT -- the bucket meaning the converter could not run, alongside unparseable frontmatter. That is the wrong shelf. Two images wanting one attachment name is a defect in the document, fixed by renaming a file, exactly like the dead link two lines above it. So the collision gets a typed error and check reports it as Broken: ok false, error and code null, the message in the broken list where an author looking for what to fix will find it. Publishing commands need no such distinction and get none -- either way the file does not go up. The test asserts the bucket through --json rather than the human output, which prints a broken file and a failed one identically. Refs #59. --- cmd/check/check.go | 11 +++++++++ cmd/check/check_test.go | 47 ++++++++++++++++++++++++++++++++++++++ internal/convert/images.go | 24 ++++++++++++++++--- 3 files changed, 79 insertions(+), 3 deletions(-) diff --git a/cmd/check/check.go b/cmd/check/check.go index 343ad92..4b322a6 100644 --- a/cmd/check/check.go +++ b/cmd/check/check.go @@ -142,6 +142,17 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache page, err := convert.MdToConfluence(mf, root, index, checkBaseURL, checkSpaceKey, buildinfo.Stamp()) if err != nil { + // Two assets wanting one attachment name is a defect in the document, + // not a failure of the converter: the author fixes it by renaming a + // file, exactly as they would fix a dead link. Reported as Broken so it + // reads that way and lands in the same list, rather than as a failed + // file whose error field a reader has to interpret. + var collision *convert.NameCollisionError + if errors.As(err, &collision) { + r.broken = []string{collision.Error()} + r.status = statusBroken + return r + } return r.fail(err, jsonout.CodeConvert) } r.broken = page.Broken diff --git a/cmd/check/check_test.go b/cmd/check/check_test.go index b379eb0..d36f78e 100644 --- a/cmd/check/check_test.go +++ b/cmd/check/check_test.go @@ -130,6 +130,53 @@ func TestRunBroken(t *testing.T) { } } +// TestRunNameCollisionIsBroken pins the bucket, not just the message. A +// collision fails the conversion, and every other conversion failure is +// reported as a failed file -- but this one is a defect in the document, the +// same kind of thing as a dead link, so it belongs in broken where an author +// looking for what to fix will find it. +func TestRunNameCollisionIsBroken(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "arch", "diagram.png"), "PNG") + write(t, filepath.Join(dir, "deploy", "diagram.png"), "PNG") + write(t, filepath.Join(dir, "main.md"), + "# Main\n\n![arch](arch/diagram.png)\n\n![deploy](deploy/diagram.png)\n") + + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error", err) + } + var env struct { + Results []struct { + Status string `json:"status"` + Broken []string `json:"broken"` + Error *string `json:"error"` + Code *string `json:"code"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if len(env.Results) != 1 { + t.Fatalf("results = %v, want one", env.Results) + } + got := env.Results[0] + if got.Status != "broken" { + t.Errorf("status = %q, want broken -- a collision is a document defect, not a failed file", got.Status) + } + if got.Error != nil || got.Code != nil { + t.Errorf("error/code = %v/%v, want both null (broken says it all)", got.Error, got.Code) + } + if len(got.Broken) != 1 || + !strings.Contains(got.Broken[0], "arch/diagram.png") || + !strings.Contains(got.Broken[0], "deploy/diagram.png") { + t.Errorf("broken = %v, want one entry naming both paths", got.Broken) + } +} + func TestRunFailed(t *testing.T) { dir := t.TempDir() write(t, filepath.Join(dir, "bad.md"), "---\npage_width: huge\n---\n# Bad\n") diff --git a/internal/convert/images.go b/internal/convert/images.go index 5084141..d3a1a27 100644 --- a/internal/convert/images.go +++ b/internal/convert/images.go @@ -116,9 +116,11 @@ func (r *storageRenderer) renderImage( // // Comparing source paths and not just the name is what keeps the // same image referenced twice a dedupe rather than a collision. - return ast.WalkStop, fmt.Errorf( - "%s%s and %s%s both publish as the attachment %q; rename one of them", - prev.line, prev.source, r.linePrefix(node, source), rootRel, filename) + return ast.WalkStop, &NameCollisionError{ + Name: filename, + First: prev.line + prev.source, + Second: r.linePrefix(node, source) + rootRel, + } case claimed: // The same asset again: one upload, two references. @@ -195,6 +197,22 @@ func (r *storageRenderer) parseImageTitle(prefix, titleRaw, src string) map[stri return attrs } +// NameCollisionError reports two assets in one file that want one attachment +// name. Typed rather than a bare error because `check` reports it as a document +// defect -- a Broken entry, like a dead link -- where an untyped conversion +// failure is reported as the converter having failed outright. Publishing +// commands need no such distinction: either way the file does not go up. +type NameCollisionError struct { + Name string // the attachment name both assets want + First string // the first image, as "line N: path" + Second string // the one that collided with it +} + +func (e *NameCollisionError) Error() string { + return fmt.Sprintf("%s and %s both publish as the attachment %q; rename one of them", + e.First, e.Second, e.Name) +} + // pastedRiFilenameRE matches an ri:filename attribute in a markdown body -- raw // storage the author pasted, which the shield hands through untouched. var pastedRiFilenameRE = regexp.MustCompile(`ri:filename="([^"]*)"`) From 2c4dbd120f427d12fe91ea05900e10f989585c7d Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:12:35 -0400 Subject: [PATCH 06/34] refactor(attachmentupload): refuse a name collision across a batch The converter refuses two images in one document that want one attachment name. The same collision is reachable through a batch of FILEs, where nothing would catch it: planAttachments reads what is already on the page once, before its loop, so two files claiming one name both plan "created", the second upload lands on top of the first, and both are reported as successes. Naming the same file twice is still one name claimed once, not a collision. Refs #59. --- cmd/attachmentupload/attachmentupload.go | 14 +++++++ cmd/attachmentupload/attachmentupload_test.go | 38 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/cmd/attachmentupload/attachmentupload.go b/cmd/attachmentupload/attachmentupload.go index 11de1fd..d544dd4 100644 --- a/cmd/attachmentupload/attachmentupload.go +++ b/cmd/attachmentupload/attachmentupload.go @@ -151,6 +151,14 @@ func forced(actions []client.SyncAction) []client.SyncAction { // download restored this one somewhere the markdown never references. func localAttachments(files []string, name string, roots *project.Cache) ([]client.LocalAttachment, error) { out := make([]client.LocalAttachment, 0, len(files)) + // An attachment name is unique per page, and a name is now a base name, so + // two FILEs in one batch can want the same one. The converter refuses the + // same thing for two images in one document; here nothing else would catch + // it: planAttachments builds its map of what is already on the page once and + // never updates it inside the loop, so both files would plan "created" and + // the second upload would quietly land on top of the first, both reported as + // successes. + claimed := map[string]string{} for _, f := range files { info, err := os.Stat(f) if err != nil { @@ -170,6 +178,12 @@ func localAttachments(files []string, name string, roots *project.Cache) ([]clie if filename == "" { return nil, fmt.Errorf("%q is not a usable attachment name", source) } + if prev, dup := claimed[filename]; dup && prev != source { + return nil, fmt.Errorf( + "%s and %s both upload as the attachment %q; rename one or upload them separately", + prev, source, filename) + } + claimed[filename] = source // source is recorded as given, not as a decode of the name. The name is // now the base name, so decoding it back would throw the path away and // record "x.png" for an asset at "docs/assets/x.png" -- which is the one diff --git a/cmd/attachmentupload/attachmentupload_test.go b/cmd/attachmentupload/attachmentupload_test.go index 646e191..2c0949a 100644 --- a/cmd/attachmentupload/attachmentupload_test.go +++ b/cmd/attachmentupload/attachmentupload_test.go @@ -4,6 +4,7 @@ import ( "os" "path" "path/filepath" + "strings" "testing" "github.com/mozilla/markfluence/internal/client" @@ -111,6 +112,43 @@ func TestLocalAttachmentsNameIsTheSourcesBaseName(t *testing.T) { } } +// TestLocalAttachmentsRefusesABatchCollision is the batch counterpart of the +// converter's refusal. Nothing downstream would catch it: planAttachments reads +// what is on the page once, before the loop, so two files claiming one name both +// plan "created" and the second upload lands on top of the first with both +// reported as successful. +func TestLocalAttachmentsRefusesABatchCollision(t *testing.T) { + root := t.TempDir() + a := writeFile(t, root, "arch/diagram.png") + b := writeFile(t, root, "deploy/diagram.png") + writeFile(t, root, "markfluence.yaml") + + _, err := localAttachments([]string{a, b}, "", project.NewCache(root)) + if err == nil { + t.Fatal("want a refusal when two files want one attachment name") + } + for _, want := range []string{"arch/diagram.png", "deploy/diagram.png", "diagram.png"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + +// TestLocalAttachmentsAllowsTheSameFileTwice keeps the refusal to genuine +// collisions: naming one file twice is one attachment, not two claims on a name. +func TestLocalAttachmentsAllowsTheSameFileTwice(t *testing.T) { + root := t.TempDir() + f := writeFile(t, root, "assets/x.png") + + got, err := localAttachments([]string{f, f}, "", project.NewCache(root)) + if err != nil { + t.Fatalf("localAttachments: %v", err) + } + if len(got) != 2 || got[0].Filename != got[1].Filename { + t.Errorf("got %v, want the same name twice rather than a refusal", got) + } +} + // TestLocalAttachmentsRejectsRootEscape covers --root naming a directory that // isn't an ancestor of the file at all: project.Resolve applies the override // uniformly with no containment check of its own, so rootRelativeSource must From f3c0646e1e500845451f054451b94ee806062416 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:15:25 -0400 Subject: [PATCH 07/34] refactor(convert): stop interpreting a stored attachment name AttachmentSource is gone, along with the escape constants it shared with the encoder. A stored name is no longer decoded anywhere: it is the base name, so there is nothing in it to decode, and a name containing "%2F" is a filename with a "%2F" in it. That retires a disagreement rather than only a function. attachfile.Resolve read a.Title verbatim while sourceFor decoded it, so an attachment whose name was encoded but whose comment had been lost landed at dest/assets%2Fbrand.png with markdown pointing at assets/brand.png -- a broken image, in single-page export, today. Both sides now read the recorded path and fall back to the same name. storage2md's images-encoded-src case is renamed to images-uninterpreted-names and rebuilt around what is worth pinning now: exotic filenames (a space, an accent, a literal percent, parentheses) are URL-encoded into a destination and never decoded, so a file called "100%25.png" comes back as "100%2525.png". The regression case of the same name keeps its name -- it is about encoded image *destinations*, which is unaffected. TestRoundTripEncodedImageSources keeps its property and loses its premise: it asserted a codec inverted itself, and now asserts that a destination decodes to the path recorded on the attachment. That matters more than it did, since a single recorded field is what stands between an export and a flattened tree. Refs #59. --- internal/client/client.go | 17 ++++++--- internal/convert/attachname.go | 26 -------------- internal/convert/attachname_test.go | 27 -------------- internal/convert/storage_to_md.go | 20 +++++++---- internal/convert/storage_to_md_test.go | 36 ++++++++++++------- .../images-encoded-src/input.storage | 6 ---- .../storage2md/images-encoded-src/output.md | 11 ------ .../images-uninterpreted-names/input.storage | 6 ++++ .../images-uninterpreted-names/output.md | 11 ++++++ .../testdata/storage2md/images/input.storage | 4 +-- .../testdata/storage2md/images/output.md | 4 +-- 11 files changed, 69 insertions(+), 99 deletions(-) delete mode 100644 internal/convert/testdata/storage2md/images-encoded-src/input.storage delete mode 100644 internal/convert/testdata/storage2md/images-encoded-src/output.md create mode 100644 internal/convert/testdata/storage2md/images-uninterpreted-names/input.storage create mode 100644 internal/convert/testdata/storage2md/images-uninterpreted-names/output.md diff --git a/internal/client/client.go b/internal/client/client.go index 9492cd0..5832163 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -1059,11 +1059,18 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt case meta.Source != "" && meta.Source != att.Source: // The bytes are unchanged but the recorded path is wrong, so re-upload // to restamp the comment -- otherwise a path mangled in transit would - // survive every later publish. The name is the encoding of the path, so - // the two move together: a disagreement under the same name means the - // stored comment does not say what we wrote. An empty Source is not a - // disagreement -- a comment with no source recorded at all is a normal - // case (see attachmentComment), not something to treat as mangled. + // survive every later publish. + // + // This is also how an asset that moved repairs itself. The name is the + // base name (convert.AttachmentFilename), so moving a file within the + // tree keeps its name and changes only its recorded path: one restamp, + // no second attachment, nothing orphaned. Under the encoded-path names + // this branch could not see such a move at all, because the name moved + // with the path and the old attachment was simply abandoned. + // + // An empty Source is not a disagreement -- a comment with no source + // recorded at all is a normal case (see attachmentComment), not + // something to treat as mangled. p.action = "updated" default: p.action = "skipped" diff --git a/internal/convert/attachname.go b/internal/convert/attachname.go index c2a3a27..378d62f 100644 --- a/internal/convert/attachname.go +++ b/internal/convert/attachname.go @@ -36,11 +36,6 @@ import ( "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 base name of the file, and nothing else. The src is normalized first // so that "a/./x.png" and "a/x.png" agree, and so a name is never derived from a @@ -59,27 +54,6 @@ func AttachmentFilename(src string) string { return path.Base(rel) } -// 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 diff --git a/internal/convert/attachname_test.go b/internal/convert/attachname_test.go index 9183ac0..a8f9024 100644 --- a/internal/convert/attachname_test.go +++ b/internal/convert/attachname_test.go @@ -75,30 +75,3 @@ func TestAttachmentFilenameNormalizes(t *testing.T) { } } } - -// 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/storage_to_md.go b/internal/convert/storage_to_md.go index e398d80..47e5034 100644 --- a/internal/convert/storage_to_md.go +++ b/internal/convert/storage_to_md.go @@ -106,17 +106,23 @@ type mdRenderer struct { headingSlugs 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. +// sourceFor resolves an attachment name back to the markdown image path to +// write: the path recorded on the attachment, or the name itself. +// +// A stored name is never interpreted. It used to be decoded, back when the name +// was an encoding of the path -- but the name is the base name now, so there is +// nothing in it to decode, and a name that happens to contain "%2F" is a +// filename with a "%2F" in it. The comment is the only place a path is written +// down, which also means placement (internal/attachfile) and the markdown +// written here cannot disagree about where an attachment belongs: both read the +// same field and fall back to the same name. +// +// An absolute recorded path is never something markfluence published, so it is +// refused and the 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 } diff --git a/internal/convert/storage_to_md_test.go b/internal/convert/storage_to_md_test.go index a00e3e4..c76e121 100644 --- a/internal/convert/storage_to_md_test.go +++ b/internal/convert/storage_to_md_test.go @@ -404,13 +404,19 @@ func TestRoundTripPassthrough(t *testing.T) { } } -// TestRoundTripEncodedImageSources closes the loop between the two halves of the -// image-source codec, on real emitted storage rather than a hand-written -// fragment: every destination `read`/`export` writes must decode back to exactly -// the path `update` published from. If it did not, exporting a page would rename -// its own image files, and re-publishing the export would upload them again under -// new attachment names. -func TestRoundTripEncodedImageSources(t *testing.T) { +// TestRoundTripImageSourcesViaRecordedPath closes the loop between publishing an +// image and reading it back, on real emitted storage rather than a hand-written +// fragment: every destination `read`/`export` writes must decode to exactly the +// path `update` published from. If it did not, exporting a page would rename its +// own image files, and re-publishing the export would upload them again. +// +// It used to test the two halves of a codec -- the name encoded the path, and +// this asserted the decode inverted the encode. There is no codec now: the name +// is the base name and the path lives only in the attachment's comment. So the +// property is the same sentence with a different mechanism behind it, and it +// matters more than it did, because a single recorded field is now the only +// thing standing between an export and a flattened tree. +func TestRoundTripImageSourcesViaRecordedPath(t *testing.T) { data, err := os.ReadFile(filepath.Join(regressionDir, "images-encoded-src", "test.output")) if err != nil { t.Fatalf("reading golden: %v", err) @@ -473,10 +479,11 @@ func imageDests(md string) []string { return out } -// 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. +// TestStorageToMarkdownPrefersRecordedSource checks the two ways an image path +// is recovered. The path recorded on the attachment wins because it is the only +// record there is; with none, the name is used as written and never +// interpreted -- a name containing "%2F" is a filename with a "%2F" in it, not +// a path in disguise. func TestStorageToMarkdownPrefersRecordedSource(t *testing.T) { const in = `

` @@ -484,8 +491,11 @@ func TestStorageToMarkdownPrefersRecordedSource(t *testing.T) { 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) + // The "%" is percent-encoded on the way out because a destination is a URL, + // not because the name is being decoded: the file really is called + // "assets%2Fx.png". + if want := "![d](assets%252Fx.png)\n"; got != want { + t.Errorf("name used verbatim: got %q, want %q", got, want) } // A recorded source overrides the name -- this is what makes an attachment diff --git a/internal/convert/testdata/storage2md/images-encoded-src/input.storage b/internal/convert/testdata/storage2md/images-encoded-src/input.storage deleted file mode 100644 index a7e4edc..0000000 --- a/internal/convert/testdata/storage2md/images-encoded-src/input.storage +++ /dev/null @@ -1,6 +0,0 @@ -

-

-

-

-

-

diff --git a/internal/convert/testdata/storage2md/images-encoded-src/output.md b/internal/convert/testdata/storage2md/images-encoded-src/output.md deleted file mode 100644 index 67110c1..0000000 --- a/internal/convert/testdata/storage2md/images-encoded-src/output.md +++ /dev/null @@ -1,11 +0,0 @@ -![spaced](assets/my%20image.png) - -![accented](assets/caf%C3%A9.png) - -![literal percent](assets/100%25.png) - -![parenthesized](shot%20%281%29.png) - -![plain](assets/plain.png) - -![remote](https://cdn.example.net/my%20image.png) diff --git a/internal/convert/testdata/storage2md/images-uninterpreted-names/input.storage b/internal/convert/testdata/storage2md/images-uninterpreted-names/input.storage new file mode 100644 index 0000000..c9153f7 --- /dev/null +++ b/internal/convert/testdata/storage2md/images-uninterpreted-names/input.storage @@ -0,0 +1,6 @@ +

+

+

+

+

+

diff --git a/internal/convert/testdata/storage2md/images-uninterpreted-names/output.md b/internal/convert/testdata/storage2md/images-uninterpreted-names/output.md new file mode 100644 index 0000000..2d19b0d --- /dev/null +++ b/internal/convert/testdata/storage2md/images-uninterpreted-names/output.md @@ -0,0 +1,11 @@ +![spaced](my%20image.png) + +![accented](caf%C3%A9.png) + +![literal percent](100%2525.png) + +![parenthesized](shot%20%281%29.png) + +![looks encoded](assets%252Fplain.png) + +![remote](https://cdn.example.net/my%20image.png) diff --git a/internal/convert/testdata/storage2md/images/input.storage b/internal/convert/testdata/storage2md/images/input.storage index f3f5e7b..c72c7b7 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 c3006f9..106b8a4 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](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](shot.png '{"title":"A Tooltip","width":300,"height":150,"align":"center"}') From 217fa0e886ca607eadea822d50d8711c3ff43601 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:16:35 -0400 Subject: [PATCH 08/34] test: retire encoded attachment names from the fixtures The suite was full of attachments named "assets%2Fx.png" carrying a recorded path of "assets/x.png" -- a pairing markfluence can no longer produce, since it would name that file "x.png". The tests passed either way; they were describing a scheme that no longer exists, which is how a reader ends up believing it does. attachfile's literal-name case keeps its encoded name, because that is the case: a file really called "a%2Fb.png" must not be scattered into a/b.png. What changed is why it holds. It used to be a judgement call about a name that might have been one markfluence wrote; now no name is ever interpreted, on either the placement or the markdown side, and the comment says so. Refs #59. --- internal/attachfile/attachfile_test.go | 35 +++++++++++++++----------- internal/client/client_test.go | 10 ++++---- internal/pagedoc/pagedoc_test.go | 4 +-- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/internal/attachfile/attachfile_test.go b/internal/attachfile/attachfile_test.go index 667dde2..866b122 100644 --- a/internal/attachfile/attachfile_test.go +++ b/internal/attachfile/attachfile_test.go @@ -19,7 +19,7 @@ func managed(title, source string) client.Attachment { func TestResolveUsesRecordedSource(t *testing.T) { root := filepath.Clean("/tmp/dest") - got, err := Resolve(root, managed("assets%2Fx.png", "assets/x.png"), false) + got, err := Resolve(root, managed("x.png", "assets/x.png"), false) if err != nil { t.Fatal(err) } @@ -30,8 +30,15 @@ func TestResolveUsesRecordedSource(t *testing.T) { } // TestResolveIgnoresNameWhenUnmanaged is why restoration reads the comment and -// never decodes the stored name: a hand-uploaded file literally named -// "a%2Fb.png" must not be scattered into a/b.png. +// never interprets the stored name: a file literally named "a%2Fb.png" must not +// be scattered into a/b.png. +// +// This used to be a judgement call -- a name that looked encoded might have been +// one markfluence wrote, and there was no telling. It is not one any more. +// markfluence names an attachment by its base name, so a name containing "%2F" +// is a filename, and convert.sourceFor answers the same way on the markdown +// side. The two agreeing is what keeps a downloaded file where the markdown +// says it is. func TestResolveIgnoresNameWhenUnmanaged(t *testing.T) { root := filepath.Clean("/tmp/dest") got, err := Resolve(root, client.Attachment{Title: "a%2Fb.png"}, false) @@ -46,11 +53,11 @@ func TestResolveIgnoresNameWhenUnmanaged(t *testing.T) { func TestResolveFlatIgnoresSource(t *testing.T) { root := filepath.Clean("/tmp/dest") - got, err := Resolve(root, managed("assets%2Fx.png", "assets/x.png"), true) + got, err := Resolve(root, managed("x.png", "assets/x.png"), true) if err != nil { t.Fatal(err) } - want := filepath.Join(root, "assets%2Fx.png") + want := filepath.Join(root, "x.png") if got != want { t.Errorf("Resolve = %q, want %q", got, want) } @@ -61,7 +68,7 @@ func TestResolveFlatIgnoresSource(t *testing.T) { // inside --dest it is fine. func TestResolveAllowsLegitimateParent(t *testing.T) { root := filepath.Clean("/tmp/dest") - got, err := Resolve(root, managed("..%2Fassets%2Flogo.png", "../assets/logo.png"), false) + got, err := Resolve(root, managed("logo.png", "../assets/logo.png"), false) if err == nil { t.Fatalf("Resolve = %q; a source escaping the root must be refused", got) } @@ -134,7 +141,7 @@ func withDownload(a client.Attachment) client.Attachment { func TestWriteCreatesNestedPath(t *testing.T) { root := t.TempDir() c := testClient(t, "BYTES") - got := Write(c, withDownload(managed("assets%2Fx.png", "assets/x.png")), + got := Write(c, withDownload(managed("x.png", "assets/x.png")), Options{Root: root}) if got.Status != StatusDownloaded { t.Fatalf("status = %q (%v), want downloaded", got.Status, got.Err) @@ -203,7 +210,7 @@ func TestWriteRemovesPartialFileOnDownloadFailure(t *testing.T) { func TestWriteDryRunCreatesNothing(t *testing.T) { root := t.TempDir() c := testClient(t, "BYTES") - got := Write(c, withDownload(managed("assets%2Fx.png", "assets/x.png")), + got := Write(c, withDownload(managed("x.png", "assets/x.png")), Options{Root: root, DryRun: true}) if got.Status != StatusDownloaded { t.Errorf("status = %q, want downloaded (the forecast)", got.Status) @@ -233,9 +240,9 @@ func TestWriteRefusesEscapeWithoutWriting(t *testing.T) { func TestWriteFlatUsesStoredName(t *testing.T) { root := t.TempDir() c := testClient(t, "BYTES") - got := Write(c, withDownload(managed("assets%2Fx.png", "assets/x.png")), + got := Write(c, withDownload(managed("x.png", "assets/x.png")), Options{Root: root, Flat: true}) - if want := filepath.Join(root, "assets%2Fx.png"); got.DestPath != want { + if want := filepath.Join(root, "x.png"); got.DestPath != want { t.Errorf("dest = %q, want %q", got.DestPath, want) } } @@ -292,7 +299,7 @@ func TestWriteRefusesSymlinkedDirectory(t *testing.T) { root, outside := symlinkFixture(t) c := testClient(t, "PWNED") - got := Write(c, withDownload(managed("assets%2Fx.png", "assets/x.png")), Options{Root: root}) + got := Write(c, withDownload(managed("x.png", "assets/x.png")), Options{Root: root}) if got.Status != StatusFailed { t.Errorf("status = %q, want failed", got.Status) } @@ -347,7 +354,7 @@ func TestWriteCreatesRootWhenMissing(t *testing.T) { root := filepath.Join(t.TempDir(), "new", "dest") c := testClient(t, "BYTES") - got := Write(c, withDownload(managed("assets%2Fx.png", "assets/x.png")), Options{Root: root}) + got := Write(c, withDownload(managed("x.png", "assets/x.png")), Options{Root: root}) if got.Status != StatusDownloaded { t.Fatalf("status = %q (%v), want downloaded", got.Status, got.Err) } @@ -414,7 +421,7 @@ func TestResolveNormalizesRoot(t *testing.T) { "/tmp/other/../dest", } for _, root := range roots { - got, err := Resolve(root, managed("assets%2Fx.png", "assets/x.png"), false) + got, err := Resolve(root, managed("x.png", "assets/x.png"), false) if err != nil { t.Errorf("Resolve(root=%q) errored: %v", root, err) continue @@ -440,7 +447,7 @@ func TestResolveNormalizedRootStillRefusesEscapes(t *testing.T) { func TestWriteNormalizesRoot(t *testing.T) { dir := t.TempDir() c := testClient(t, "BYTES") - got := Write(c, withDownload(managed("assets%2Fx.png", "assets/x.png")), + got := Write(c, withDownload(managed("x.png", "assets/x.png")), Options{Root: dir + string(os.PathSeparator)}) if got.Status != StatusDownloaded { t.Fatalf("status = %q (%v), want downloaded", got.Status, got.Err) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 18e88e1..0232a2c 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -785,7 +785,7 @@ func TestPlanAttachmentsClassifiesWithoutUploading(t *testing.T) { // extensions, and the download link is an API path, not /download/attachments. func TestListAttachmentsDecodesMetadata(t *testing.T) { list := `{"results":[{` + - `"id":"att99","title":"assets%2Fx.png",` + + `"id":"att99","title":"x.png",` + `"metadata":{"comment":"` + attachmentCommentPrefix + `sha256=abc path=assets/x.png"},` + `"version":{"number":3,"when":"2026-08-05T22:17:28.040Z"},` + `"extensions":{"mediaType":"image/png","fileSize":171},` + @@ -1236,7 +1236,7 @@ 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"}, + {Path: path, Filename: "x.png", Source: "assets/x.png"}, }) if err != nil { t.Fatal(err) @@ -1292,7 +1292,7 @@ func TestSyncAttachmentsLabelsTextPartsUTF8(t *testing.T) { source := "assets/probe-café.png" c, s := newServer(t, resp{200, `{"results":[]}`}, resp{200, `{}`}) _, err := c.SyncAttachments("1", []LocalAttachment{ - {Path: path, Filename: "assets%2Fprobe-café.png", Source: source}, + {Path: path, Filename: "probe-café.png", Source: source}, }) if err != nil { t.Fatal(err) @@ -1325,11 +1325,11 @@ func TestSyncAttachmentsLabelsTextPartsUTF8(t *testing.T) { func TestSyncAttachmentsRestampsMangledSource(t *testing.T) { path, sum := writeTempImage(t) // A comment stored double-encoded: "é" recorded as "é". - list := `{"results":[{"id":"a1","title":"assets%2Fprobe-café.png","metadata":{"comment":"` + + list := `{"results":[{"id":"a1","title":"probe-café.png","metadata":{"comment":"` + attachmentComment(sum, "assets/probe-café.png") + `"}}]}` c, s := newServer(t, resp{200, list}, resp{200, `{}`}) actions, err := c.SyncAttachments("1", []LocalAttachment{ - {Path: path, Filename: "assets%2Fprobe-café.png", Source: "assets/probe-café.png"}, + {Path: path, Filename: "probe-café.png", Source: "assets/probe-café.png"}, }) if err != nil { t.Fatal(err) diff --git a/internal/pagedoc/pagedoc_test.go b/internal/pagedoc/pagedoc_test.go index 28c0520..b3c7af3 100644 --- a/internal/pagedoc/pagedoc_test.go +++ b/internal/pagedoc/pagedoc_test.go @@ -45,7 +45,7 @@ func TestRenderFrontmatterQuotesWhenNeeded(t *testing.T) { // --- Sources ----------------------------------------------------------------- func TestSourcesFrom(t *testing.T) { - managed := client.Attachment{Title: "assets%2Fx.png"} + managed := client.Attachment{Title: "x.png"} managed.Metadata.Comment = "markfluence: sha256=abc path=assets/x.png" legacy := client.Attachment{Title: "assets_x.png"} legacy.Metadata.Comment = "mzcld:checksum: abc" @@ -55,7 +55,7 @@ func TestSourcesFrom(t *testing.T) { if len(got) != 1 { t.Fatalf("got %d sources, want 1: %v", len(got), got) } - if got["assets%2Fx.png"] != "assets/x.png" { + if got["x.png"] != "assets/x.png" { t.Errorf("sources = %v", got) } // An attachment with no recorded source contributes nothing, so the converter From aa91ba065ab7101d18a5e9c236e808f9c04e03fc Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:16:59 -0400 Subject: [PATCH 09/34] docs(confluence): attachment names are base names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The file opened by stating that markfluence percent-encodes a path into a name and that the mapping is bijective. It does not, since the naming change. The measurements stay. That a `%2F` name resolves and re-escapes to `%252F` in the image URL, that a space and an `é` survive byte-identically -- these are facts about Confluence, not about markfluence, and they remain useful: someone will meet such a name on a page published before the change. Separating the two is the whole edit here; nothing measured was deleted for having stopped being load-bearing. Two consequences were restated rather than dropped. The name-length limit now binds against a single path component rather than a path with its slashes expanded, so it binds even later than it did. And the recorded-path-disagrees branch turns out to do more than repair a mangled comment: it is how an asset that moved keeps its attachment, which under the old names was impossible because the name moved with the path. Refs #59. --- docs/confluence/attachments.md | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/docs/confluence/attachments.md b/docs/confluence/attachments.md index 99a8ec2..e00832f 100644 --- a/docs/confluence/attachments.md +++ b/docs/confluence/attachments.md @@ -4,14 +4,20 @@ `/` is not legal in an attachment name. Everything else we have tried is. -markfluence therefore flattens an image's path into a name by percent-encoding -`%` → `%25` first and `/` → `%2F` second — bijective, so distinct paths can never -collide and `read` can recover the original path exactly. See -`internal/convert/attachname.go`. +markfluence names an attachment by its **base name** — `assets/diagram.png` is +attached as `diagram.png` — and records the path in the attachment's comment. +See `internal/convert/attachname.go`. + +It used to flatten the whole path into the name by percent-encoding `%` → `%25` +then `/` → `%2F`, which was bijective. The measurements below were taken against +those names and are kept, because they are facts about Confluence rather than +about markfluence: a name containing `%2F` still resolves, and someone will +still encounter one on a page published before the change. What they no longer +describe is what markfluence produces. | in the name | result | | |---|---|---| -| `/` | illegal — hence the encoding | Transcribed | +| `/` | illegal — hence the base name | Transcribed | | `%2F` (encoded slash) | resolves and renders; the image URL re-escapes it to `%252F` | **Verified** | | a literal space | stored and returned byte-identically; renders | **Verified 2026-08-07** | | non-ASCII (`é`) | stored and returned byte-identically, NFC preserved | **Verified 2026-08-07** | @@ -54,8 +60,9 @@ The comment carries fixed overhead — `markfluence: ` + `sha256=` + 64 hex + ` path=` is **90 characters** — so a recorded source path may be at most **165 characters**. -The name limit never binds first. A name is the path with each `/` expanded to -`%2F`, so a 165-character path would need 45 slashes before it reached 255. +The name limit never binds first, and by a wider margin than it used to: a name +is now a single path component, so it is bounded by the filesystem's own name +limit (255 on every filesystem markfluence runs on) long before Confluence's. Measured against realistic paths: @@ -120,9 +127,15 @@ is no reason to take either away. A skip does not rewrite the comment, so an attachment whose stored `path=` is wrong would keep it until its bytes happened to change. `planAttachments` therefore treats a *recorded path that disagrees with the local source* as an -update even when the checksum matches. The name is the encoding of the path, so -the two are always in lockstep — under a matching name, a differing path means -the stored comment does not say what markfluence wrote. +update even when the checksum matches: under a matching name, a differing path +means the stored comment does not say what markfluence wrote. + +Since the name became the base name this branch does more than repair a mangled +path — it is how an asset that **moved** settles. Moving `assets/flow.png` to +`img/flow.png` keeps the name `flow.png`, so the attachment is found, the bytes +match, and only the path disagrees: one re-upload to restamp the comment, and +nothing orphaned. Under the encoded names the move changed the name too, so the +old attachment was simply abandoned and this branch never saw it. A legacy comment records no path at all. That is not a disagreement and stays a skip; re-uploading every one of those is the churn the checksum comparison From 36a7377a2dbd7f6fadc4f01e61c56d41bc479a22 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:17:36 -0400 Subject: [PATCH 10/34] docs(guarantees): L3 after the naming change, and R2's new first step L3's note said two things that the base-name scheme falsifies: that moving an asset still changes its identity, and that fixing that would cost the ability to reconstruct a tree on export. Moving an asset within the tree now keeps its identity and restamps its recorded path, and reconstruction was never the name's job -- the comment carries the path. What identity follows now is the asset's file name, so renaming the file is what mints a new attachment; the note says which claims died rather than quietly dropping them. R2 grows a step at the front. An attachment that cannot be *named* -- two assets in one document whose base names agree -- is as unusable as one that cannot be placed, and it is reported in three places: the converter refuses the file, attachment-upload refuses the batch, check reports it as Broken. The label does not change. This file says labels are as permanent as ids because a renamed label makes an old citation silently wrong, so the widening lands in the note and report-unplaceable-attachments stays. root-model.md's attachment-identity paragraph described Source as what the name encodes. It is now the only record of the path, which is a stronger claim than the one it replaces. Refs #59. --- docs/guarantees.md | 29 +++++++++++++++++++++++++---- docs/root-model.md | 7 ++++--- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/docs/guarantees.md b/docs/guarantees.md index 97641f0..d0ef5e7 100644 --- a/docs/guarantees.md +++ b/docs/guarantees.md @@ -148,12 +148,19 @@ finds the root by walking up from each file's own directory, independent of the working directory and of what else is in the same command (`_plans/026` commits 1–4). -**L3** is what makes moving a page free. Moving an *asset* still changes its -identity; buying that back would need content-addressed names, at the cost of -being able to reconstruct a tree on export. `images.go` records an attachment's +**L3** is what makes moving a page free. `images.go` records an attachment's `Source` relative to the root rather than to the referencing page, so identity follows the asset alone (`_plans/026` commit 4). +Both halves of the sentence that used to follow are now wrong, and it is worth +saying how. It read that moving an *asset* still changes its identity, and that +fixing that would need content-addressed names at the cost of being able to +reconstruct a tree on export. Neither survives `_plans/029`: an attachment is +named by its base name, so moving an asset within the tree keeps its identity +and restamps its recorded path, and reconstruction was never the name's job +anyway — the comment carries the path. What identity still follows is the +asset's *file name*, so renaming the file is what creates a new attachment. + **L5** and **L6** stay Partial, deferred to #59 (multi-page export). Two separate things kept them there, and both are now repaired. Since `_plans/026` commit 4 records an attachment's `Source` relative to the root, @@ -219,7 +226,7 @@ nothing is computing a wrong answer. | | label | guarantee | status | |---|---|---|---| | **R1** | `report-unresolved-references` | Every reference markfluence could not resolve is reported. | Holds | -| **R2** | `report-unplaceable-attachments` | Every attachment markfluence could not place is reported. | Holds | +| **R2** | `report-unplaceable-attachments` | Every attachment markfluence could not name or place is reported. | Holds | **R1** was false by design and documented as such: the README said an unresolved link was "published as-is, which on Confluence is a dead relative @@ -250,6 +257,20 @@ first place, by design, since only images are uploaded and a relative href to anything else would be dead regardless. That sits outside R1's claim rather than inside it unmet, so it does not block Holds. +**R2** covers naming as well as placement, which is a widening of the note and +not of the label — labels are permanent (see *Changing this document*), so +`report-unplaceable-attachments` stays as it is even though the guarantee now +reaches one step earlier in the pipeline. + +The step is new. Since `_plans/029` an attachment is named by its base name, so +two assets in one document can want one name — and there is no correct way to +publish that, since a name is unique per page. The converter refuses the file +and names both paths and both lines; `attachment-upload` refuses the same +collision across a batch; `check` reports it offline as a Broken entry, in the +same list as a dead link, because renaming a file is the fix in either case. +An attachment that cannot be named is as unusable as one that cannot be placed, +and the obligation to say so is the same one. + ## Non-goals Decisions about what markfluence will not do. They live here because they diff --git a/docs/root-model.md b/docs/root-model.md index a55b318..ed8adb1 100644 --- a/docs/root-model.md +++ b/docs/root-model.md @@ -111,9 +111,10 @@ control, a generated snapshot). ## Attachment identity -An image's recorded `Source` — what its Confluence attachment name encodes, -and what `read`/`export` use to put it back where it came from — is relative -to the root, not to the page that references it (`_plans/026` commit 4). Two +An image's recorded `Source` — what `read`/`export` use to put it back where it +came from, and the only record of its path there is — is relative to the root, +not to the page that references it (`_plans/026` commit 4). The attachment +*name* is the file's base name and carries none of this (`_plans/029`). Two pages at different depths referencing the same file now record the same source and get the same attachment; before, each recorded the reference as written, and the same file had two identities in Confluence. This is L3 From 4f282262d02dbc745702fded920f1e7cf8ff1ef6 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 5 Sep 2026 08:19:14 -0400 Subject: [PATCH 11/34] docs: README, CLAUDE.md, and attachment-upload's help for base names Four places in the README described the encoding as current behaviour: the attachment-naming paragraph under Body, --name "which markfluence encodes for you", attachment-list's example row, and export's refusal of --attachments-dir. That last one had to be re-reasoned rather than reworded. It argued the flag was unsafe because rewriting an image's src would change its attachment name and orphan the original -- which is no longer true, since moving assets/x.png to attachments/x.png keeps the name x.png. The flag stays unimplemented for a different reason: collecting everything into one directory reintroduces exactly the collision a base name has to refuse, because two pages' diagram.png cannot share a directory. The naming paragraph and the CLI's own --name help now document the refusal as part of the scheme rather than leaving it to be discovered on a failed publish. attachment-list's note about "attachments left behind by the encoding change" was already there from a previous naming change and now covers this one too. Refs #59. --- CLAUDE.md | 4 +- README.md | 64 ++++++++++++++---------- cmd/attachmentupload/attachmentupload.go | 19 ++++--- 3 files changed, 50 insertions(+), 37 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c823bd4..bb2d8e4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -56,7 +56,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `cmd/children/` — `children`: list the pages and folders under a page or folder, via `internal/pagetree`. `--depth` is a **string** vocabulary (a positive number or `all`, default `1`), not an int: `all` is not a number, and `0` is refused rather than read as "unlimited" the way it is elsewhere, because silently walking a whole space for someone who meant "none" is worse than an error that names `all`. Folder rows are emitted with a `type` column, which is what makes "a folder counts as a level" safe. Empty is a success: `No children.` and exit 0. **`--space KEY` lists a whole space instead of a page** (#98), which makes `PAGE` optional — exactly one of the two, checked before credentials. Depth 1 is then the space's **root pages**, not the homepage's children: a space can have several roots (`create` with a null parent makes one), so seeding the walk from `homepageId` would drop a root and its whole subtree, and there is no root-level *folder* to miss because a folder created with no parent lands under the homepage ([docs/confluence/spaces.md](docs/confluence/spaces.md)). A root row's `parent_id` is `null` — the one place `childrenResult` needs `stringOrNull` for it — since a space is not a node. The key is resolved through `ResolveSpaceID` before the walk even though the v1 route it feeds takes a key: an unknown key must fail as a typo (exit 2) the way it does for `find`/`search`, and the v1 route reports one as a 404, which is also what a rejected credential looks like. Because a space's top level is usually one row, human output adds a `--depth` reminder when `--depth` was left at its default — on **stderr**, via `ui.Hint`, so the table stays pipeable; `--json` never sees it. A failing space walk likewise reports an `errorObject` on stderr rather than a `results[0]` failure, since `SingleOpFailure.page_id` would otherwise carry a space key. - `cmd/find/` — `find`: resolve a title to the ids carrying it, via `client.FindByTitle`. A title is the one handle `internal/pageref` cannot resolve. It reports **current pages, archived pages, and folders**, which takes two requests because no single API sees all three — and the three-way split is the thing to keep straight before touching it ([docs/confluence/search.md](docs/confluence/search.md)). An **archived** page is reported, with a `status` column, because it is absent from the page tree yet still reserves its title; a **folder** is reported because a folder id is a legitimate `parent`, but a folder reserves nothing, so a folder row must never be treated as a naming conflict. `--space` is a space **key**, and an unknown one is a hard error rather than an empty result — CQL answers an unknown key with zero rows, which reads exactly like "no such page". Either half failing fails the whole command: a partial answer reads as "nothing found", and the caller's next move on that is to create a duplicate. Empty is a success: `No matches found.` and exit 0. Its operational failure is an `errorObject` on stderr rather than a `results[0]` entry — there is no page id to name — which it shares with `search` and with `children --space`, and nothing else. - `cmd/search/` — `search`: find pages by **full text**, via `client.SearchText` (or `SearchRawCQL` under `--cql`). The complement to `find`: `find` needs the exact title, `search` is for when it is unknown. The evidence for every choice here is in [docs/confluence/search.md](docs/confluence/search.md), and two things there must be understood before touching the query. First, **`text ~` — the field Atlassian documents — ranks uselessly**: for "deploy runbook" it returned six unrelated pages above every page titled with both words, where the undocumented `siteSearch ~` returned them in order. Second, **`siteSearch` is silently discarded when it is the middle clause of three**, which turns a `--space` search into a listing of the entire space with no error — so `buildTextCQL` puts it **first** and adds a redundant `text ~` clause as a floor, and both are pinned by tests. Do not reorder that query. There is no client-side recovery, because the API reports `score` as `0.0` on every row, so **the server's order is the only ranking and nothing may re-sort a result set** (this is the first command whose result order is not its own). Output is a block per hit rather than a table: the excerpt answers "why did this match?" and is too long for a column. Matched terms in that excerpt are **reverse-videoed from the server's own `@@@hl@@@` markers**, which `cleanExcerptSpans` keeps as `SearchMatch.Spans` instead of discarding — highlighting by matching the query text would have to reimplement Confluence's stemming and would have nothing to work from under `--cql`. `Excerpt` stays the canonical string the schema pins and the spans reassemble to it exactly, so the human and `--json` paths cannot disagree; the flags are built *during* cleaning, since unescaping and whitespace-collapse both change length and marker offsets taken beforehand do not survive them. `renderSpans` takes the highlighter as a parameter rather than calling `ui.Match`, because tests run with stdout not a terminal where lipgloss emits nothing at all — a test wired to the real style would pass against unhighlighted text. `--limit` is a **string** vocabulary (a positive number or `all`, default `10`) refusing `0` exactly as `children --depth` does — and the default is a bound rather than "all" because a full-text query matches thousands of pages where a title lookup matches a handful, kept as low as `10` because a hit is a 5-6 line block rather than a row; the pager fetches one extra row so "more exist" is reported without claiming a count `totalSize` cannot supply. `--type` (`page` default, `blogpost`, `all`) exists because an untyped query returns attachment, comment and database ids no verb accepts; `folder` is **refused** with a pointer to `find`, since full text cannot match a folder at all and always answering "no matches" is worse than an error. `--cql` passes the query through verbatim and **refuses `--space` and an explicitly-set `--type`** (via `Flags().Changed`, which only `children`'s `--depth` hint also uses), because ANDing a clause onto a query containing `or` regroups it and silently answers something else. Two things it cannot see, both `find`'s job: **archived pages and folders**. A blank query is refused locally, because the API answers one with a 500 rather than a 400. -- `cmd/attachment{list,upload,download}/` — the flat `attachment-list`/`attachment-upload`/`attachment-download` commands (noun-first so cobra's alphabetized help keeps them together and `attachment-` completes as a group). `upload` reuses the checksum skip/update logic, with `--force` (`client.ForceUploadAttachments`) and `--dry-run` (`PlanAttachments`); its `--name` takes a *path* and encodes it, and the recorded `path=` is always the decode of the stored name, so a later publish can't create a duplicate under a different name. `download` restores an attachment to its recorded `path=` (never a decode of the stored name — a hand-uploaded `a%2Fb.png` is indistinguishable from a published one), with `--flat` to opt out; `destPath` is the only place server data becomes a filesystem path and clamps to `--dest`, refusing rather than clipping an escape, since `..` is legitimate in a source path. +- `cmd/attachment{list,upload,download}/` — the flat `attachment-list`/`attachment-upload`/`attachment-download` commands (noun-first so cobra's alphabetized help keeps them together and `attachment-` completes as a group). `upload` reuses the checksum skip/update logic, with `--force` (`client.ForceUploadAttachments`) and `--dry-run` (`PlanAttachments`); its `--name` takes a *path* whose base name becomes the stored name, and the recorded `path=` is the path as given, so a later publish can't create a duplicate under a different name; a batch whose base names collide is refused, since `planAttachments` reads the page's attachments once before its loop and would otherwise plan two creates for one name. `download` restores an attachment to its recorded `path=`, using the stored name verbatim when there is none (never interpreting it — a file really called `a%2Fb.png` must not be scattered into `a/b.png`, and `convert.sourceFor` answers the same way on the markdown side, which is what keeps a downloaded file where the markdown says it is), with `--flat` to opt out; `destPath` is the only place server data becomes a filesystem path and clamps to `--dest`, refusing rather than clipping an escape, since `..` is legitimate in a source path. - `cmd/schema/` — `schema`: print the embedded `--json` schema to stdout verbatim (no args, no credentials, no Confluence call). `--json` is deliberately a no-op — the output is already the schema document, not an envelope — which is also why `schema` is absent from the schema's own `command` enum. - `schema/` — the published JSON Schema (`json-output/v1.json`) *and* the `schema` Go package that embeds it (`V1`). The Go file lives beside the schema because `go:embed` cannot reach outside its own directory, and the schema stays at a top-level path a non-Go consumer can browse, mirroring its own `$id`. `internal/schematest` validates against the embed rather than reading the file, which is what makes "what ships" and "what the tests checked" the same bytes — do not reintroduce a disk read or a second copy. The version number is **not** restated here: `jsonout.SchemaVersion` and the document's own `schema_version` const are the two copies, tied together by a test in `cmd/schema`. - `internal/pagedoc` — a fetched page as a markdown document: `Render` (frontmatter + converted body), `Frontmatter`, and the two lookups the converter can't do for itself — `Sources`/`SourcesFrom` (attachment name → recorded source path) and `PageLinks` (the page an `` points at → its URL). Shared by `read` and `export`, which must emit byte-identical markdown, and **both build their `convert.StorageOptions` through `Options`** rather than assembling their own — options built in two places are options that can disagree. It needs a client (page width, attachment list, title lookups), which is why it isn't in `internal/convert` — that package is deliberately client-free, and it's why `StorageToMarkdown` takes those maps rather than fetching them. Every one of them is best-effort in the same shape: no references in the body means no request at all, and a lookup that fails is omitted rather than fatal (an omitted page link renders as raw storage, not as a link with no destination). `PageLinks` resolves a space id **once per space key**, not once per link, and refuses to search site-wide when it can't scope a title to a space — a same-titled page in the wrong space is a wrong answer, which is worse than the passthrough a miss produces. @@ -64,7 +64,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `internal/pagetree` — `Walk`, the traversal of pages *and folders* under a node, plus `WalkSpace` (the same traversal seeded from a space's root pages, via `client.ListSpaceRootPages`) and `AllDepths`. Both go through one `walker`, so the depth rule and the visited guard exist in a single copy. It is a package rather than command-local because listing a subtree and exporting one (#59) need the identical walk, and its rules must not exist in two copies: siblings arrive from two requests (`/child/page`, `/child/folder`) and are **merged by `extensions.position`**, or the output loses the order Confluence displays; a folder **counts as a level** like a page, which is only reasonable because folders are reported rather than silently traversed; and the walk descends folders even when only pages matter, since a folder may hold the only pages in a subtree. `nodeURL` uses `SiteURL()` — a v1 child row carries `webui` but no `base`. A visited set guards the unbounded case. - `internal/pageref` — `Resolve`, the single page-argument resolver: a numeric id, a Confluence page **or folder** URL (`pagePathRE` matches both `/pages/` and `/folder/`, since `children` takes a folder and a folder URL is what a browser hands you — the id is all it returns, so a command that can only use a page reports its own not-found), or a `.md` file whose frontmatter has a `page_id` (stat'd first, so `123.md` is a file). Every command taking a page uses it. `message.go` also owns the wording for the two ways a *frontmatter* `page_id` is wrong — `NotFoundMessage` (caller supplies the remedy, which differs per command) and `NotNumericMessage` — because `create`, `update`, and `fix` all report them and a reader should recognize the same problem across all three. They return strings, not errors: `create` wraps the text in its typed `pageIDFailure` (which also carries the `--json` fields), the others want a plain error. Anything checking a `page_id` before a request uses `IsDigits`, since the API answers a non-numeric id with a 400 whose body says nothing useful. - `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/...`). A **folder** — the Cloud content type that can parent a page — has its own v2 route, `GetFolderOrNil` against `/wiki/api/v2/folders/{id}`, because every v2 *page* route answers a folder id with 404; enumerating children, if it is ever added, must be v1, since v2 cannot list inside a folder at all and its page-children route silently omits folders ([docs/confluence/folders.md](docs/confluence/folders.md)). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send`. `HTTPError.Error()` appends a **hint** for the three auth failures whose status misleads, matched on the response *body* rather than deduced from the status and always **appended** to it, never replacing it. The one that matters: **a rejected credential is a 404 on every v2 route**, so a revoked token used to make `read` answer `page ... not found` about a page that exists. `RejectedCredential` tells it apart by the fact that every genuine v2 404 *names* what it could not find and the auth one does not, `notFound` gates the three `…OrNil` helpers on it so they stop reading it as "absent", and `jsonout.CodeFor` checks it before the status switch so `--json` reports `AUTH` rather than `NOT_FOUND`. A 403 that is not one of the two measured credential phrasings gets no hint, because that is what a genuine permission denial looks like ([docs/confluence/api.md](docs/confluence/api.md#scopes)). **Retry rules**: 429 for any method; 502/503/504 for idempotent methods; **any other 5xx only when the response carries `Retry-After`** — that is how a 500 becomes retryable, and it is why `parseRetryAfter` reports the header's *presence* apart from its delay (`Retry-After: 0` means "retry now", not "no header"). The exponential delay is jittered, a server-supplied `Retry-After` never is. Decisions go to a package-level hook (`SetRetryLogger`, set once in `root.go` beside `ui.SetDebug`) and fire whichever way they went, because `internal/client` prints nothing and a silent twelve-minute retry storm is otherwise indistinguishable from a hang. **A versioned PUT is not as idempotent as its method**: `SetContentProperty` retry-once on top (recovers a lost create-POST response) and `UpdatePage`'s `updateLanded` both exist for the same reason — a write whose response was lost gets re-sent, and the re-sent version is refused. `updateLanded` requires version *and* title *and* body to match what was sent, since a concurrent edit could have produced the version alone and claiming success over someone else's content is worse than a false failure ([docs/confluence/api.md](docs/confluence/api.md)). `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; only the current comment form is parsed — an attachment stamped by a markfluence predating a comment-format change reads as unmanaged and is re-uploaded once, the same as any hand-uploaded file — except that a *recorded path disagreeing with the local source* is an update even when the checksum matches, so a mangled path repairs itself instead of surviving every later publish; a comment with no source recorded at all is not a disagreement. Every text part of the upload form must go through `writeTextField`, never `multipart.Writer.WriteField`, which emits no charset and gets decoded as Latin-1), `_links.next` pagination. **Three pagination schemes, and picking the wrong one truncates silently.** v1 *child/attachment* collections page through the generic `listV1` helper by `start`/`limit` offset, never `_links.next` (absent when the results fit one page, so it cannot terminate a loop); `ListAttachments`, `ListChildPages`, and `ListChildFolders` all go through it. v2 collections page through `listV2` by the cursor in `_links.next`, which is a `/wiki`-prefixed absolute path `resolveNext` handles unchanged; `ListContentProperties` and `SearchPagesByTitle` share it. **`/wiki/rest/api/search` is neither**: it ignores `start` outright, its `next` is context-relative so it needs the `/wiki` prefix `resolveNext` does not add, a short page does *not* mean the end, and `totalSize` can be nonzero against an empty `results` — so `searchCQL` terminates only on a missing `next` and nothing may branch on `totalSize` ([docs/confluence/search.md](docs/confluence/search.md)). `searchCQLBounded` adds a row bound under it (`SearchCQL` is that call with no bound, which is why `find` is unaffected): it asks for `max+1` and reports the surplus as `more`, since `totalSize` cannot supply a count. Full text goes through `SearchText`/`SearchRawCQL`, which return the cleaned `SearchMatch` the way `FindByTitle` returns `TitleMatch` — and **every field of a match comes from the row's `content` object**, because the row-level `title` is HTML-escaped *and* wrapped in `@@@hl@@@` markers where `content.title` is neither. The `excerpt` exists only at row level, so `cleanExcerpt` strips those markers, unescapes once, and collapses to one line — in the client, so the human and `--json` paths cannot disagree about it. `excerpt=highlight` is passed explicitly and **re-attached when following the cursor** (the `next` link carries `cql` and `limit` but not `excerpt`, and `doJSON` appends params with a bare `?`); an unrecognized value there yields an empty excerpt with a 200, so a rename by Atlassian degrades to no excerpts rather than an error. A row with no `content` object is skipped and **counted** — `type = space` answers with hundreds of them, and a silent skip would report a successful empty result. A bare v1 child row already carries `webui`, `status`, and `extensions.position`, so child listing needs no `expand`. `DownloadAttachment` goes through `send` (inheriting retry/backoff) against `_links.download`; **never** add a `CheckRedirect` that forwards headers — it would leak site credentials to Atlassian's media host, which neither needs nor wants them. `config.go` holds `Resolve` and the `.env` reader. Why each of these is shaped this way, with the evidence: [docs/confluence/api.md](docs/confluence/api.md) and [attachments.md](docs/confluence/attachments.md). -- `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, root *project.Root, index *linkindex.Index, baseURL, spaceKey, version string) (*ConfluencePage, error)`. `root` bounds which images and parent references may be read (S1/S2) and is what an image's recorded `Source` is relative to; `index` is the tree-wide link/anchor index for `root` (`internal/linkindex.Build`), built once and shared across every file converted under it rather than rebuilt per conversion — both are discovered/built by the caller (`internal/project`/`internal/linkindex`), which is why this package stays client-free. 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; `aclink.go` is the *inverse* direction's one element with enough shape to need its own file — ``, which the editor writes for every internal link and `MdToConfluence` never emits, so nothing in the regression suite covers it. One rule decides its whole mapping: **convert when the markdown republishes to a link resolving to the same target, pass the storage through when it would not** — so a page link and a space link convert, while a mention (80% of all real usage), an attachment link (only images are uploaded, so a relative href would be dead) and an unresolvable target stay raw, which the shield republishes byte-identical. A page target is a **title, never an id**, so `PageLinkTargets` reports what needs resolving and `StorageOptions.PageLinks` carries the answers back. An `ac:anchor` is **percent-encoded** where `confluenceSlug` output is not: decode it before matching a heading, leave it encoded inside a URL. A same-page anchor recovers its heading from the document rather than inverting the slug, which is impossible — `confluenceSlug` turns both a space and a hyphen into `-`. The survey the mapping rests on, and the `xml.HTMLAutoClose` trap that made `` crash the parser outright (#88), are in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); `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; what names Confluence accepts is in [docs/confluence/attachments.md](docs/confluence/attachments.md)); `destination.go` owns the **other** codec, destination↔path (`decodeDestination`/`encodeDestination`), shared by images *and* doc links — a markdown destination is a URL, so decode inbound (**before** `withinRoot`, or an encoded `..%2F` slips the clamp) and encode outbound in `storage_to_md.go` (or `export` emits markdown that no longer parses, and `sourceFor`'s absolute-path refusal is undone by the next read); an undecodable destination is a literal `%` in a filename, not an error; the reasoning is in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); `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` (GitHub/Confluence slugs, doc-link + anchor rewriting against `internal/linkindex`'s tree-wide index; `resolveDocKey` resolves a destination to the index's root-relative key and reports `escapes` — a purely lexical check on the *query* side, since the index itself needs no clamp: an escaping key can never be in it, built by walking downward from root). A doc-link target is one of four severities, #42: missing entirely or escaping root is **Broken** (`LINK BROKEN: … (not found|outside the documentation root)`) and replaces the whole `` element — tags and visible text alike — with that literal message, matching `images.go`'s precedent for a missing image (`renderLink` needs a small per-node flag, `linkBrokenText`, since goldmark still invokes a container node's renderer on the matching leaving call regardless of `WalkSkipChildren` on entering, and there is no `` to write in the broken case); existing on disk with no `page_id` yet is unchanged — a **warning**, the normal state of an unpublished tree; a `#fragment` matching no heading on an otherwise-resolving target also **warns**, gated on `linkindex.Index.FileExists` so a missing/escaping target isn't double-reported. `tables.go` (the `` tag, stamped with `data-layout="align-start"` so tables auto-size and left-align — this must stay if column widths are ever emitted, or a `` silently induces a layout; plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour`; `storage_to_md.go`'s `cellTexts` reverses this, reading `data-highlight-colour` back into a `bg:` marker (`cellBGNames`, the reverse of `tables.go`'s swatch map — a hex outside the 21 swatches round-trips as the literal hex, and where two names share a hex the British spelling wins, matching Confluence's own `-colour`), and a column's GFM alignment becomes a `

` wrapper **inside** the cell — never the `align` attribute the GFM renderer would emit, which is the one form Confluence discards. Only center and right are emitted: Confluence has no explicit left, so `:---` publishes bare and `read` recovers it as `---`. Since alignment is per-paragraph there and per-column in GFM, `columnSeparators` in `storage_to_md.go` takes each column's most common declared alignment (ties to the first seen) and drops the rest. Rows still fall through to the GFM renderer. A multi-line cell is one `

` per line — Enter in the editor starts a new `

`, it does not insert a `
` — so `renderCellLines` in `storage_to_md.go` joins sibling `

` children with a literal `
` rather than nothing: a GFM table row is exactly one physical line, so a real newline isn't an option, and the same substitution catches a bare mid-line `
` (Shift+Enter) that would otherwise render as the two-space hard break valid in ordinary block content but not inside a table row. A `

` tag, stamped with `data-layout="align-start"` so tables auto-size and left-align — this must stay if column widths are ever emitted, or a `` silently induces a layout; plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour`; `storage_to_md.go`'s `cellTexts` reverses this, reading `data-highlight-colour` back into a `bg:` marker (`cellBGNames`, the reverse of `tables.go`'s swatch map — a hex outside the 21 swatches round-trips as the literal hex, and where two names share a hex the British spelling wins, matching Confluence's own `-colour`), and a column's GFM alignment becomes a `

` wrapper **inside** the cell — never the `align` attribute the GFM renderer would emit, which is the one form Confluence discards. Only center and right are emitted: Confluence has no explicit left, so `:---` publishes bare and `read` recovers it as `---`. Since alignment is per-paragraph there and per-column in GFM, `columnSeparators` in `storage_to_md.go` takes each column's most common declared alignment (ties to the first seen) and drops the rest. Rows still fall through to the GFM renderer. A multi-line cell is one `

` per line — Enter in the editor starts a new `

`, it does not insert a `
` — so `renderCellLines` in `storage_to_md.go` joins sibling `

` children with a literal `
` rather than nothing: a GFM table row is exactly one physical line, so a real newline isn't an option, and the same substitution catches a bare mid-line `
` (Shift+Enter) that would otherwise render as the two-space hard break valid in ordinary block content but not inside a table row. A `