From 65965a18c8cc6eed1b692dab5bbc0035012b43ee Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 17:07:23 -0400 Subject: [PATCH] fix(convert): recover table cell background colors on export/read storage_to_md.go never read data-highlight-colour, so a page's cell background colors were silently dropped on export/read even though MdToConfluence fully supports writing them from a bg: marker. Verified live against MIR/1891074056 (60 colored cells, 0 recovered). Fixes #109. --- CLAUDE.md | 2 +- internal/convert/storage_to_md.go | 27 ++++++++++++++++++- internal/convert/storage_to_md_test.go | 36 ++++++++++++++++++++++++++ internal/convert/tables.go | 15 +++++++++++ 4 files changed, 78 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 78ce4bb..e027b3e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,7 +63,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 `AllDepths`. 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, baseURL, spaceKey, version string) (*ConfluencePage, error)`. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `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` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting; `docKey` is the single lookup key for both the page and anchor maps. Failure here is **silent** — an unresolved link publishes as a dead relative href with no warning), `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`, 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. Layout values, cell alignment, and colors: [docs/confluence/storage-format.md](docs/confluence/storage-format.md)), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. +- `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version string) (*ConfluencePage, error)`. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `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` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting; `docKey` is the single lookup key for both the page and anchor maps. Failure here is **silent** — an unresolved link publishes as a dead relative href with no warning), `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. Layout values, cell alignment, and colors: [docs/confluence/storage-format.md](docs/confluence/storage-format.md)), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. - `internal/frontmatter` — flat YAML frontmatter parse/quote/`UpdateField`, and the `MarkdownFile` type (`Parse`/`ParseFile`, exported `Filename`/`Content`/`Frontmatter`/`Body`, and `Title`/`PageID`/`Space`/`Parent` accessors that normalize missing/blank/`"null"`). - `internal/pagewidth` — the `page_width` `Width` enum (`narrow`/`wide`/`max`, default `max`), `Declared`, the vocab↔content-property maps, `WidthFromProperties`, and `Apply`/`Read` against the client. Width lives in two content properties and **both** must be written or the reader and the editor disagree: [docs/confluence/page-width.md](docs/confluence/page-width.md). - `internal/schematest` — the `--json` drift guard, and the reason the schema can't fall behind the code. `ValidateEnvelope`/`ValidateError` validate an emitted document against the embedded schema; `document.go` checks the schema *document* instead (`Commands`, plus tests that every name in the `command` enum has an `if/then` branch that constrains `results.items` **and** `summary`). That last one matters because outside a branch the schema says only "results is an array": a command added to the enum without a branch is completely unvalidated, and adding just the enum entry is exactly how a new command's conformance test goes green. `cmd`'s `TestCommandEnumMatchesRegisteredCommands` closes the loop from the other side — every registered subcommand is in the enum or in that test's `noJSONEnvelope` list. Two rules keep all this working: **every result field lives on a typed struct and nothing uses `omitempty`** (so every field always marshals and `additionalProperties:false`/`required` catch an added, renamed, or removed one no matter what a fixture sets — never build a result as a `map[string]any`), and **a conformance test builds its document with the command's own builder** (`failEnvelope`, `jsonResult`) rather than a hand-copied literal, or it validates a copy while the real output drifts. diff --git a/internal/convert/storage_to_md.go b/internal/convert/storage_to_md.go index 7218591..7a3df50 100644 --- a/internal/convert/storage_to_md.go +++ b/internal/convert/storage_to_md.go @@ -428,18 +428,43 @@ func rowHasHeaderCell(tr *snode) bool { return false } -// cellTexts renders a row's cells to inline strings with pipes escaped. +// cellTexts renders a row's cells to inline strings with pipes escaped, +// prefixed with a bg: marker for a cell carrying a background color. func (r *mdRenderer) cellTexts(tr *snode) []string { var cells []string for _, c := range tr.kids { if c.name == "th" || c.name == "td" { text := strings.ReplaceAll(r.renderInlineChildren(c), "|", `\|`) + if marker := cellBGMarkerComment(c); marker != "" { + if text == "" { + text = marker + } else { + text = marker + " " + text + } + } cells = append(cells, text) } } return cells } +// cellBGMarkerComment recovers a "" marker from a cell's +// data-highlight-colour, the inverse of resolveCellBG. A hex outside the 21 +// swatches -- set directly in hand-edited storage, never by markfluence -- +// round-trips as the literal hex rather than a name. +func cellBGMarkerComment(c *snode) string { + hex, ok := c.attrs["data-highlight-colour"] + if !ok || hex == "" { + return "" + } + hex = strings.ToLower(hex) + name, ok := cellBGNames[hex] + if !ok { + name = hex + } + return "" +} + // renderMacro renders an : the code/toc/callout macros // MdToConfluence emits become their markdown equivalents; any other macro passes // through as raw storage. A block-context unknown macro uses the round-trip-safe diff --git a/internal/convert/storage_to_md_test.go b/internal/convert/storage_to_md_test.go index c25a024..4125f7d 100644 --- a/internal/convert/storage_to_md_test.go +++ b/internal/convert/storage_to_md_test.go @@ -157,6 +157,42 @@ func TestRoundTripTableAlignment(t *testing.T) { } } +// TestRoundTripTableCellBG checks that a cell background marker survives +// md -> storage -> md, including the swatch-name normalization (a #hex that +// matches a named swatch comes back as the name) and the gray/grey collision +// (both spellings resolve to the same hex, and grey wins on the way back). +func TestRoundTripTableCellBG(t *testing.T) { + src := strings.Join([]string{ + "| status | note |", + "| --- | --- |", + "| down | also down |", + "| unknown | |", + }, "\n") + "\n" + want := strings.Join([]string{ + "| status | note |", + "| --- | --- |", + "| down | also down |", + "| unknown | |", + }, "\n") + "\n" + + md := frontmatter.Parse("main.md", src) + root := testRoot(t, "") + page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") + if err != nil { + t.Fatalf("MdToConfluence: %v", err) + } + if !strings.Contains(page.HTML, `data-highlight-colour="#ffebe6"`) { + t.Errorf("published storage missing data-highlight-colour:\n%s", page.HTML) + } + got, err := convert.StorageToMarkdown(page.HTML, convert.StorageOptions{}) + if err != nil { + t.Fatalf("StorageToMarkdown: %v", err) + } + if got != want { + t.Errorf("round-trip mismatch\n--- got ---\n%s\n--- want ---\n%s", got, want) + } +} + // TestStorageToMarkdownStripsGeneratedIDs checks that the server-generated // ac:macro-id and ac:local-id attributes are dropped from passthrough output. func TestStorageToMarkdownStripsGeneratedIDs(t *testing.T) { diff --git a/internal/convert/tables.go b/internal/convert/tables.go index e750a54..7eeb9e8 100644 --- a/internal/convert/tables.go +++ b/internal/convert/tables.go @@ -92,6 +92,21 @@ var ( cellBGHexRE = regexp.MustCompile(`^#[0-9a-f]{6}$`) ) +// cellBGNames is the reverse of cellBGSwatches, hex -> name, for reconstructing a +// bg: marker on export/read. "grey" and "gray" (and their light- variants) share a +// hex; the British spelling wins since that's what Confluence's own +// data-highlight-colour and docs/confluence/storage-format.md use. +var cellBGNames = func() map[string]string { + names := make(map[string]string, len(cellBGSwatches)) + for name, hex := range cellBGSwatches { + if strings.Contains(name, "gray") { + continue + } + names[hex] = name + } + return names +}() + // tableCellBGTransformer implements the cell background color marker: an HTML // comment at the start of a table cell, //