From 379da87c9a3ee19bd8f6e13300735e7fbf9867ca Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:42:07 -0400 Subject: [PATCH 01/16] docs: plan for publishing page labels from frontmatter Plans #138: a `labels` frontmatter field that `update`/`create` assert against the live page, `fix`/`read`/`export` recover, `info` displays, and `check` validates offline. Two layers, and the lower one is the larger job. `internal/frontmatter` errors on any sequence value today, so `labels: [a, b]` breaks every command that reads the file, not just the label-aware ones -- sequence support has to land first, and has to be general enough that `reviewers: [ana, bo]` works too. Records what was probed rather than assumed. Confluence splits a label name on spaces and commas (so `[Runbook Two]` publishes as two labels that read back as neither, and re-add forever under assert-exactly), refuses a colon, caps names at 255 UTF-16 units, and only accepts label writes on v1 with removal via `?name=` -- the path form 400s on the real `ci/cd` label. goccy reads both sequence styles correctly and a block list survives Normalize intact, so both are accepted and an author's style is preserved on rewrite; the write-side self-check has to run per style, because mapping context passes `x,y` and `has]bracket` while block context turns `? q` into a mapping. --- _plans/036_labels.md | 501 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 501 insertions(+) create mode 100644 _plans/036_labels.md diff --git a/_plans/036_labels.md b/_plans/036_labels.md new file mode 100644 index 0000000..2e26fdf --- /dev/null +++ b/_plans/036_labels.md @@ -0,0 +1,501 @@ +# Plan: publish page labels from frontmatter + +Add a `labels` frontmatter field that `update`/`create` assert against the live +page, `fix`/`read`/`export` recover from it, `info` displays, and `check` +validates offline. Implements #138. + +Labels are how Confluence content is actually organized and searched — the SRE +space carries 94 distinct labels across 1123 pages — and today markfluence +cannot see them at all: a published page must be labeled by hand in the UI +afterward, and `read`/`export` silently drop the labels a page already carries. +The README's own `search --cql 'label = "runbook"'` example only pays off for +pages someone labeled by hand. + +## Two layers, and the lower one is the real work + +The feature splits cleanly, and the lower half is both larger and more +dangerous than the label logic on top of it. + +**`internal/frontmatter` does not support sequences at all.** `toMap` calls +`scalarValue` on every mapping value, and `scalarValue` is a whitelist of +scalar node kinds — so a `*ast.SequenceNode` returns `frontmatter "labels" must +be a single scalar value, found Sequence`. That error comes out of `Parse`, +which means the moment anyone writes `labels: [a, b]` **every** command that +touches the file fails, not just the label-aware ones: `update` on that file +reports a frontmatter error instead of publishing. So sequence support has to +land first and has to be general — a parser that special-cases the key `labels` +would leave `reviewers: [ana, bo]` breaking the same way, and #21 and #100 both +want the same door opened. + +**The label vocabulary on top is ordinary**, modeled on `internal/pagewidth`: +validate, normalize, diff against the live set, apply. + +## What Confluence does — verified 2026-09-08 + +Probed against `mozilla-hub.atlassian.net` with a scratch page in a personal +space: label names POSTed one at a time via v1, read back through both v1 and +v2, both delete forms exercised, page purged. The full table goes into +`docs/confluence/labels.md`; the four findings that shape the design: + +1. **A space or comma is a separator, not a character.** `"Runbook Two"` posts + as two labels, `runbook` + `two`. `"a,b"` posts as `a` + `b`. HTTP 200, no + warning. Under assert-exactly this is a permanent non-convergence: + `labels: [Runbook Two]` publishes as two labels, reads back as neither, and + is re-added on every `update` forever with no way to remove it. Two rows in + the SRE space look like this having already happened to a human — + `continuous` + `delivery` and `url` + `shortener`, each once, on pages where + someone plainly typed a two-word label. **This is what the validation exists + to prevent, and it is why an invalid label is a hard failure rather than a + repair.** +2. **The reject set is the server's own**: a 400 `label.contains.invalid.chars` + names `space ! # & ( ) * , . : ; < > ? @ [ ] ^`. A colon is in it, so there + is no silent prefix-splitting and no frontmatter syntax for a prefix. +3. **Removal must use `?name=`, not `DELETE /label/{name}`.** The path form + works until the name holds a `/`: `a%2Fb` → 400 (Tomcat HTML, no JSON), + while `?name=a/b` → 204. Not hypothetical: `ci/cd` is a real label in the + SRE space. +4. **Writes are v1; v2 is read-only for labels** (`POST`/`DELETE` on + `/wiki/api/v2/pages/{id}/labels` → 405). Exactly the attachment split. + +Plus: names are lowercased server-side (Unicode-aware), the cap is **255 +UTF-16 code units** (255 × `é` → 200, 256 × `é` → 400; 128 emoji → 400), POST +is additive and idempotent with no bulk-set route, a DELETE of an absent name is +404, and neither GET returns a useful order. + +A survey of the SRE space found **every label is `global:`**, which is the +evidence for managing that prefix and nothing else. + +## What goccy does with sequences — verified 2026-09-11 + +Probed with the pinned `goccy/go-yaml`, the same way #130's scalar traps were +found. Both spellings are accepted on read (see Decisions), so both were +measured. + +**Both forms read correctly, and a block sequence survives the write path.** +Parsed, reordered the way `Normalize` does, run through `dropBlankLines`, and +re-parsed: a block list comes back as a block list with the same elements, a +`# comment` inside it survives, and a blank line between two items is dropped +without changing the value (a blank line between items means nothing in YAML, +unlike inside a `|-` block, where it is content). So there was never a +correctness reason to refuse block form. + +**Flow style is a field, not something to infer from the token.** A +`*ast.SequenceNode` carries `IsFlowStyle`. The sequence node's own +`GetToken().Origin` is just `" ["` — it does **not** cover the sequence's text +— so a `spansLines` check on the outer node proves nothing either way. + +**The line check has to move to the element interior.** `spansLines` trims only +the right side, which is correct for a scalar but wrong for an element, where a +*leading* newline means "this element began on a new line" — structure, not +content. Measured element origins: + +| source | element origin | `spansLines` | interior check | +|---|---|---|---| +| `[a, b]` | `" b"` | false | false | +| `[a,`⏎` b]` (wrapped flow) | `"\n b"` | **true** | false | +| `- a` (block) | `" a\n "` | false | false | +| `- a plain scalar`⏎` continued` | `" a plain scalar\n continued\n "` | true | **true** | +| `- 'sq`⏎` folded'` | `" 'sq\n folded'"` | true | **true** | + +So the rule is **a sequence may span lines; every element must be a +single-line scalar**, enforced as the node-kind whitelist plus a line check on +the origin trimmed at *both* ends. Reusing `spansLines` unchanged would refuse +a wrapped flow sequence for no reason. + +**Elements report indicator characters exactly as scalars do.** `&anch a` +reports node type `Anchor` with token value `&`, `*anch` reports `Alias`/`*`, +`!!str a` reports `Tag`/`!!str`, and a `- |-` element reports `Literal`/`|-`. +Note the last one: its origin does **not** span lines, so the whitelist is the +only thing that catches it — exactly as for scalars. A blacklist would silently +read `|-`. + +**Write-side quoting must be verified in the form being emitted, and the two +forms have different traps.** The existing `readsBackAs` checks a scalar in +*mapping* context. Measured, emitting with goccy's chosen style and re-reading: + +| value | mapping | flow `[…]` | block `- …` | +|---|---|---|---| +| `x,y` | reads back | **splits into two elements** | reads back | +| `has]bracket` | reads back | **ends the sequence; unparseable** | reads back | +| `? q` | fails (#130) | fails | **parses as a `Mapping`** (`- ? q` is YAML's explicit-key syntax) | +| `.inf` | fails | fails | **reads back as `Infinity`, not a string** | +| tab | fails (dropped) | fails | fails | +| `#hash`, `{braces}`, `true`, `""`, `ci/cd` | reads back | reads back | reads back | + +Neither form's check substitutes for the other, and mapping context passes two +values that flow context corrupts. With a per-element fallback to a +double-quoted scalar, every probed value round-trips in the form it was +checked in. + +**Indentation is controllable, so emitting block form is not a problem.** +`ast.Sequence(tok, false)` with default token positions renders `labels:`⏎`- a` +— valid YAML, but not the ` - a` convention. Setting the sequence token's +`Column` fixes that precisely: `3` yields ` - a`, `5` yields ` - a`. So a +hand-built block sequence can be emitted in the conventional shape, which is +what makes preserving the author's *form* cheap. + +Reindenting an author's list is explicitly **not** a concern — correct YAML and +the right form are the requirements, matching indentation is not one — so +markfluence does not try to reproduce a 4-space list at 4 spaces. It carries +over `IsFlowStyle` and emits block items at column 3. (Replacing only the +parsed node's elements, borrowing each original item's position, also works and +preserves the indent exactly; it is more machinery for a property nobody needs.) + +**`yaml.ValueToNode([]string{...})` returns a block sequence** +(`IsFlowStyle == false`), so it cannot be used to build the value. The node is +assembled by hand: `ast.Sequence(tok, true)` with per-element nodes appended. +An empty sequence renders `[]` in both styles. + +**Re-emission of a parsed sequence is faithful**, including each element's +original quoting style (`'x,y'` and `"has]bracket"` survive as written), which +is what lets an untouched `labels:` key — in either form — pass through a `fix` +write unchanged. One mutation: a `~` element re-emits as `null`. Harmless — +both read as the empty string, and for `labels` both are invalid anyway. + +Worth noting how the quoting work interacts with the label rules: every +character that breaks flow context (`,` `]` `[` `?` `#` space `.`) is *also* in +Confluence's reject set, so for `labels` specifically the trap is unreachable — +a value needing the fallback is refused before anything is written. The quoting +work is load-bearing for the **general** field, which is why the generality +test below is not optional. + +## Decisions + +**Sequences get their own map, not a re-typed `Frontmatter`.** +`MarkdownFile.Frontmatter` stays `map[string]string` and a sequence-valued key +is **omitted** from it, landing in a parallel `Lists map[string][]string`. +Changing the scalar map's type would touch every caller for no gain, and +leaving a sequence key in it with some flattened spelling is worse than absent: +`MarkdownFile.field()` would hand `update` a title of `[a, b]`. The parser +learns a *kind*, not a name. + +**Both sequence forms are accepted on read, and an author's form is +preserved on write.** The block form is valid YAML that goccy parses correctly +and that survives `Normalize` intact, so refusing it would mean rejecting a +file markfluence understood perfectly — and a long label list genuinely reads +better as a block list, which is the case that matters, since a set large +enough to want block form is exactly the set whose flow spelling is an +unreadable 200-column line. + +What is preserved is the **form**, not the formatting: a rewrite reads the +existing value's `IsFlowStyle` and emits the same style, with block items at a +fixed column 3. Reindenting a list an author wrote with some other indent is +accepted — valid YAML in the right form is the bar, byte-preservation is not. +A `fix` that changes a block list's contents writes a block list back. The +alternative — always emitting flow — was in an earlier draft on the false +premise that a hand-built block node could not be indented at all; it can. + +The real cost is that markfluence can now emit **both** forms, so the +write-side self-check has to run in whichever form is about to be written. The +trap sets differ (see above: `? q` becomes a `Mapping` in block form, `.inf` an +`Infinity`, while `x,y` and `has]bracket` are flow-only hazards), so one +context's check does not stand in for the other's. That is a real addition to +the surface keeping C2 true, and it is the price of not mangling the field this +feature exists to write. + +**A brand-new `labels:` key is written flow**, since `read`/`export`/`create +--persist` have no author choice to honour and a generated list is usually +short. A page carrying enough labels to want block form gets one long line the +first time, and keeps whatever the author changes it to thereafter. + +**A scalar `labels:` is refused rather than read as a one-element list.** +`labels: runbook` is legal YAML and the friendly reading is obvious, but the +field is destructive — declaring it *removes* every label not listed — and +`labels:` with a null value would then mean "remove every label" on a file +where the author most likely typed a key and stopped. Both forms error with a +message naming both sequence spellings, and `labels: []` stays the one way to +say "remove them all". Rejected alternative: accept a scalar and rewrite it as +a list on the next `fix`; it saves the author one pair of brackets and costs +the ability to tell an unfinished edit from an instruction to strip the page. + +**Validation refuses, never repairs — except for case.** Lowercasing is the one +normalization, and it comes with a warning naming the label, because Confluence +lowercases server-side and a file that disagrees would never converge. Anything +else invalid is a hard failure before any write. A leading or trailing space is +refused like an inner one (`TrimSpace` is used only to give an all-whitespace +value the clearer "empty" message), since a space is a separator server-side and +trimming would be a silent repair of the exact class of input that produced +`continuous` + `delivery`. + +**The reject set is mirrored, not an allowlist.** An allowlist would refuse +`ci/cd`, `dataops_reports` and `héllo-wörld`, all of which publish fine. The +error message quotes the set, because `.` being invalid means a version-shaped +label (`v1.2`) is impossible and no author will guess that. + +**`internal/labels` owns the set arithmetic**, so no command reimplements +"what to add, what to remove". Callers filter to `prefix == "global"`; the +client does not, because `info` needs the unfiltered list. + +**Absent means untouched, declared means asserted.** This mirrors `update`'s +existing asymmetry for `page_width`, and it is what keeps a hand-labeled page +from being silently stripped by a run that never mentioned labels. Pinned by a +test asserting that an absent `labels:` key produces **no label request at +all** — not merely no write. Without that, "untouched" is an implementation +detail rather than a property. + +**Labels are applied after the body and after the width**, non-fatally: a +failure there is a warning on an `ok: true` result, exactly as +`pagewidth.Apply` failures already are. The page is published by then; failing +the result would say the publish did not happen. Ordering after width keeps the +human output and the `--json` field order matching `info`'s layout. + +**`create` validates labels in preflight** alongside #127's converter check, so +a bad label cannot leave a created page behind, and applies them in the publish +phase. The preflight failure carries `CodeValidation` (a bad label is a property +of the file, not of the conversion, so not `CodeConvert`). + +**`fix` rewrites `labels:` only when the set differs.** Compared as sets, so a +hand-written list keeps the author's order and any duplicate; when markfluence +generates the list itself (`read`, `export`, `fix` making a change) it emits +sorted and deduped. Neither GET returns a useful order, so sorting locally is +not a preference — unsorted output is unstable across runs. + +**A new law, `L9`, for the assert-exactly rule**, with an honest **Partial**: +`update` leaves an undeclared field alone, but `create` asserts a *default* +`page_width` for a file that declares none, so "omitted means untouched" holds +for labels and not yet for width. + +## Implementation + +### `internal/frontmatter` (the general half) + +- `sequenceValue(key string, n *ast.SequenceNode) ([]string, error)` — accepts + either style, reading each element through `elementValue` with an indexed key + (`labels[1]`) so a message points at the offending item. +- `elementValue(key string, n ast.Node) (string, error)` — `scalarValue`'s + sibling, sharing the node-kind whitelist (so `Anchor`/`Alias`/`Tag`/`Literal` + are refused rather than read as their indicator characters) but checking the + line rule on the origin trimmed at **both** ends, since a leading newline in + an element origin is structure. Factor the whitelist into one helper both call + rather than copying the type switch — a second copy is how one of them + silently stops refusing anchors. +- `toMap` becomes `toMaps(m) (map[string]string, map[string][]string, error)`, + branching on `*ast.SequenceNode` before falling through to `scalarValue`. A + sequence key is absent from the scalar map. +- `MarkdownFile` gains `Lists map[string][]string`, always non-nil, with the + same "exported so callers can distinguish absent from empty" rationale the + `Frontmatter` doc comment already gives. +- `Field` gains `List []string`; **non-nil means the field is a sequence** and + `Value` is ignored, so `[]string{}` renders `labels: []` and a nil list stays + a scalar. `Render` and `mappingValue` branch on it. +- `sequenceNodeFor(values []string, flow bool) ast.Node` — + `ast.Sequence(tok, flow)` with each element from `elementNodeFor`, block + items emitted at column 3. `elementNodeFor` is `valueNodeFor`'s sibling: + goccy's chosen style when `readsBackInSeqAs(n, want, flow)` accepts it, else + `doubleQuoted`. +- `readsBackInSeqAs(n ast.Node, want string, flow bool) bool` — wraps the node + in a one-element sequence **of the style about to be emitted**, inside a + one-key mapping, re-parses, and requires a `*ast.StringNode` element whose + text matches. Both halves are load-bearing: the node-kind check for the same + reason it is for scalars (`.inf` reads back as `.inf` either way while saying + "float" to every other tool), and the style parameter because the two + contexts have different traps — mapping context passes `x,y` and + `has]bracket` which flow corrupts, and block context turns `? q` into a + `Mapping`. A single fixed context would be a check that passes while the + write is wrong. +- `UpdateListField(content, key string, values []string) (string, error)` — + `UpdateField`'s list form, sharing `setField` so the surgical key-node + preservation is not duplicated. `setField` gains one lookup: if the key + already holds a sequence, its `IsFlowStyle` is carried into the replacement, + so a block list stays a block list. An untouched key is never re-emitted at + all, so a list nothing changes keeps its exact bytes. +- The "flat mapping" error text in `parseBlock` and the package doc comment both + say sequences are allowed in either style, with single-line elements. +- `Normalize`/`dropBlankLines` need no code change, but the safety comment on + `dropBlankLines` does: its justification is currently "nothing this package + emits spans more than one line", which a passed-through block list makes + false. The accurate statement is that no value markfluence can emit carries a + *meaningful* blank line — a `|-` block would, and is refused on read. Pinned + by a test that runs a block list through `Normalize`. + +### `internal/labels` (new) + +```go +const RejectChars = ` !#&()*,.:;<>?@[]^` // the server's own set + +// Set is what a file asserts: the normalized names, whether the key was +// present at all, and any warning normalization raised. +type Set struct { + Names []string // lowercased, sorted, deduped + Declared bool + Warnings []string +} + +func Declared(lists map[string][]string, fm map[string]string) (Set, error) +func Validate(name string) error +func Normalize(name string) (norm string, changed bool) +func Diff(declared, live []string) (add, remove, unchanged []string) +func Global(live []client.Label) []string +func Apply(c *client.ConfluenceClient, pageID string, s Set) ([]Action, error) +func Read(c *client.ConfluenceClient, pageID string) ([]client.Label, error) +``` + +`Declared` takes both maps so it can refuse the scalar form with a useful +message rather than reporting "not declared" for `labels: runbook`. A `Set` +rather than four return values because `Declared` is the one call every command +makes, and `(names, declared, warnings, err)` at five call sites is four +opportunities to drop the warnings. Length is +`len(utf16.Encode([]rune(s))) <= 255` — not bytes, not runes. + +`Action` is `{Name, Action string}` with `added`/`removed`/`unchanged`, which is +what the `--json` field reports; `Apply` returns the **full** declared set's +worth of actions, not just the changes. + +### `internal/client` + +- `Label` struct (`id`, `name`, `prefix`). +- `ListLabels(pageID)` — v2 `GET /wiki/api/v2/pages/{id}/labels` via `listV2`, + unfiltered. +- `AddLabels(pageID, names []string)` — v1 `POST + /wiki/rest/api/content/{id}/child/label`, batched (the route takes an array). +- `RemoveLabel(pageID, name)` — v1 `DELETE + /wiki/rest/api/content/{id}/child/label?name=…`, with a comment recording why + the path form is wrong and citing `ci/cd`. A 404 is "already gone", not a + failure — via `notFound`, so a rejected credential is not mistaken for it. + +### Commands + +| command | change | +|---|---| +| `update` | assert the declared set after width; validation fatal before any write; apply failure is a warning on `ok: true`; an mtime-skipped file skips labels too; dry-run reads live labels and reports would-be actions, a read failure being a warning (mirrors `previewWidth`) | +| `create` | validate in preflight (after every server check, like #127's convert), apply in publish | +| `fix` | one `change{field: "labels"}` rendered `[a, b]`, `(none)` when the file has none; written through `UpdateListField`. `change` gains a `newList []string` | +| `info` | `labels` and `labels/unmanaged` rows, the second only when non-empty, per the existing "empty fields omitted" rule | +| `read`, `export` | `labels:` in the rendered frontmatter, global-only and sorted, via `pagedoc.Frontmatter`/`RenderFrontmatter` | +| `check` | `labels.Declared` + `Validate` beside the `pagewidth.Declared` call, `status: failed` with `code: VALIDATION`; case warnings land in `warnings` | + +`pagedoc.Frontmatter` fetches the labels best-effort in the shape every other +lookup there already uses: omitted on failure, never fatal. `read`'s `--json` +re-reads them in `buildResult`, which duplicates a request — the same thing it +already does for `page_width`, and consistency with the existing pattern beats +threading a value through `Render` for one command. + +### `--json` and the schema + +New `$defs`: `labelAction` (`{action, name}`), `labelInfo` +(`{name, prefix, managed}`), and the `…OrNull` wrappers. Per-command fields per +#138's table — `update`/`create` get `labels` (full declared set, `null` when +the file declares no key, would-be actions in a dry-run), `info` gets +`labels` as `labelInfo[]` (`null` when the fetch failed, `[]` when the page has +none), `read` gets `labels` as a plain sorted string array (`null` when the +fetch failed, so "none" and "unknown" stay distinguishable). `fix`, `check` and +`export` get nothing new. + +Every field on a typed struct, no `omitempty`, nullability by pointer — +`*[]jsonout.Label`, not a slice that happens to be nil. Schema edits land in the +**same commit** as the command change or `TestSchemaConformance` fails. + +## Tests + +Beyond the per-function unit tests, the ones that pin a decision: + +- **Generality.** A file with `reviewers: [ana, bo]` — a list key markfluence + knows nothing about — survives a `fix` write untouched. Without it, nothing + stops `toMaps` from hardcoding `labels` and satisfying every other test here. +- **Per-style quoting.** The probe corpus (`x,y`, `has]bracket`, `? q`, + `.inf`, a tab, `#hash`, `true`, `""`) written through `UpdateListField` reads + back as the same strings, **in both styles** — a table test over + `flow ∈ {true, false}`. `x,y`/`has]bracket` are the values a mapping-context + check wrongly passes; `? q` is the one a flow-context check wrongly passes for + a block write. Between them they are what stops the three `readsBack` helpers + being collapsed into one. +- **Element node kinds.** `[&anch a]`, `[*anch]`, `[!!str a]` and a `- |-` + element are refused, not read as `&`/`*`/`!!str`/`|-`. The `|-` case is the + one whose origin does not span lines, so it proves the whitelist is doing the + work rather than the line check. +- **Both styles read the same.** A block list and the equivalent flow list + produce identical `Lists` output, as does a flow list wrapped across lines. +- **Element line rule.** A block element continued onto the next line, and a + multi-line single-quoted element, are both refused; a wrapped flow sequence + is **not** — that pair is what distinguishes the interior check from + `spansLines`. +- **A block list survives `Normalize`.** Reordering a file whose `labels:` is a + block list leaves the list intact (items, indentation, an inline comment) and + the result re-parses — the property `dropBlankLines`' reworded comment now + claims. +- **Form survives a rewrite.** `fix` changing the label set on a file whose + `labels:` is a block list writes a **block** list back (and a flow list stays + flow). A file whose labels already match is not rewritten at all, so its + bytes are untouched. Indentation is *not* asserted beyond "parses and is the + right form" — reindenting is accepted deliberately. +- **Render/UpdateListField agreement** on the same list, extending the existing + test that pins the two writers against each other. +- **Absent means no request.** `update` on a file with no `labels:` key makes + zero label calls (httptest, asserting on paths seen). +- **Length boundary.** 255 × `é` valid, 256 × `é` invalid, 128 emoji invalid — + the three server-measured points, so a rune- or byte-based check fails. +- **The separator class.** `Runbook Two`, `a,b`, `my:foo` all refused, with the + reject set in the message. +- **Client shapes.** `RemoveLabel` uses `?name=` and never a path segment + (pinned on the request URL, with `ci/cd`); a 404 from it is success; a + rejected-credential 404 is not; `ListLabels` follows a `_links.next` cursor. +- **`fix` convergence.** Reconciling a page whose labels differ writes + `labels: [a, b]`, and a second run reports `consistent` — the property the + `continuous` + `delivery` case is the absence of. + +## Docs + +- `docs/confluence/labels.md`, new: the verified table above, marked + **Verified 2026-09-08**, plus a pointer from `docs/confluence/README.md`'s + index. +- `docs/confluence/api.md`: the three label routes in the scope table. The v1 + write scope is **unverified** — presumably `write:confluence-content`, which + that document already records as unlookupable for v1 routes. +- `docs/guarantees.md`: C2's wording gains sequences — a value is a + single-line scalar, or a sequence (either style) whose every element is one — + with the flow-context finding recorded as the reason the writer's self-check + needed one check per style, and the fact that markfluence now emits two + shapes stated plainly rather than left to be discovered; new **L9** + `declared-metadata-is-asserted`, status **Partial**, with the + `create`-default-width exception named. +- `README.md`: a `labels` row in the frontmatter table, notes in the + `update`/`create`/`fix`/`check` sections, and a pointer from the existing + `--cql 'label = "runbook"'` example (line 621) noting labels are now + publishable. +- `CLAUDE.md`: `internal/labels` in the layout list, and the frontmatter + bullet's "flat key: value" wording. + +## Commits + +1. `docs: plan for publishing labels from frontmatter` (this file, on `main`) +2. `docs(confluence): record how page labels behave` — `labels.md`, the README + index row, the `api.md` scope rows. First, so the client's comments can cite + it. +3. `feat(frontmatter): read and write sequences in either style` — the + general half, including the generality, both-styles, and quoting tests. +4. `feat(client): list, add, and remove page labels` +5. `feat(labels): validate and reconcile a page's label set` +6. `feat(check): validate labels offline` +7. `feat(update): assert the declared label set` +8. `feat(create): validate labels in preflight and apply them on publish` +9. `feat(fix): reconcile a page's labels into the file` +10. `feat(info): show a page's labels, managed and not` +11. `feat(read): emit labels in rendered frontmatter` — `pagedoc`, so `read` and + `export` change together +12. `docs: labels in the README and guarantees` + +`make check` before each. + +## Out of scope + +Per #138: no `label-*` subcommand family, no `--labels` flag (a label set is +multi-valued and the only natural CLI separators are exactly the two characters +Confluence splits on; if a flag is ever wanted the spelling is a repeatable +`--label ci/cd --label howto`), no `--label` filter on `find`/`search` (`search +--cql 'label = "runbook"'` covers it), and no space- or project-wide default +labels (#100). + +Not settled here: the OAuth scope for the v1 label routes (a scoped-token run +will find out) and whether the 255-unit cap is enforced identically on the v2 +read path (irrelevant unless another client bypassed it). + +## Follow-ups + +- #100 — project-wide settings; default labels for a tree are the obvious next + step +- #21, #38 — both want frontmatter to carry more than flat scalars; the + sequence support here is the door they need opened +- #73 — the label paths are v1 writes against a live server that no unit test + can prove; they want a smoke test +- #29 — the GitHub Action, and the reason supplying frontmatter out of band + matters From 28bf7cc08f7b524cf15f956eaf3fa8c371517a6c Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:47:05 -0400 Subject: [PATCH 02/16] feat(frontmatter): read and write sequences in either YAML style toMap errored on any sequence value, so `labels: [a, b]` -- or any other list field -- failed out of Parse and broke every command that read the file, not just the ones that knew about the key. #138 needs the field, and #21/#100 need the same door opened, so this supports sequences generally: the parser learns a kind, not a name, and a test pins that an unrelated `reviewers: [ana, bo]` survives a write untouched. Scalars stay in Frontmatter and sequences land in Lists, so a key is in exactly one map -- leaving a flattened sequence in the scalar map is how MarkdownFile.field would hand update a title of "[a, b]". An empty list is present-and-empty rather than absent, which a destructive field like labels depends on: "[]" means remove them all. Both YAML spellings are read, because goccy parses both correctly and a block list survives Normalize intact, so refusing one would reject a file that was understood perfectly. A rewrite keeps the style it found -- a set large enough to want block form is exactly the set whose flow spelling is an unreadable single line -- but style only, not formatting: block items are re-emitted at this package's own indent, not the author's. Three findings from probing goccy, each now pinned by a test: - An element's origin carries a *leading* newline whenever the element began on a new line, which is every block item and a wrapped flow list. spansLines trims only the right, so reusing it would refuse "[a,\n b]" for no reason. elementValue applies the line rule to the origin trimmed at both ends, and still refuses an element whose own value runs past its line. - A "- |-" element reports Literal/"|-" with an origin that does *not* span lines, so the node-kind whitelist is the only thing that catches it. The whitelist is now shared by scalarValue and elementValue rather than copied. - "x,y" and "has]bracket" pass readsBackAs -- they are fine as a mapping value -- and corrupt a flow sequence, splitting into two elements and ending the sequence respectively. readsBackInSeqAs verifies in the style being written; verifying in block while writing flow is the direction that corrupts, and the hazard corpus now runs through both styles to pin it. Also corrects two comments the probes proved wrong: a continued plain scalar does not come back as a "|-" block (it folds onto one line; the block scalar is the shape that breaks), and dropBlankLines' safety no longer rests on "nothing spans more than one line", since a passed-through block list does. It rests on no emittable value carrying a meaningful blank line. --- internal/frontmatter/frontmatter.go | 326 +++++++++++++++++++---- internal/frontmatter/frontmatter_test.go | 258 +++++++++++++++++- 2 files changed, 529 insertions(+), 55 deletions(-) diff --git a/internal/frontmatter/frontmatter.go b/internal/frontmatter/frontmatter.go index 8f9d90b..3655553 100644 --- a/internal/frontmatter/frontmatter.go +++ b/internal/frontmatter/frontmatter.go @@ -2,17 +2,22 @@ // markfluence markdown files carry, and models a parsed file as a MarkdownFile. // // The block is real YAML, parsed and emitted by goccy/go-yaml. It is still -// restricted to flat key: value pairs -- no nesting, lists, or multiline -// values -- but that restriction is now enforced by scalarValue rather than -// assumed by a line-splitting parser that could not see a violation. +// flat -- no nesting -- but a value may be a scalar or a sequence of scalars, +// in either YAML spelling: the flow form "[a, b]" or the block form of "- a" +// lines. Every scalar, including a sequence's elements, must occupy a single +// line; that is enforced by scalarValue and elementValue rather than assumed by +// a line-splitting parser that could not see a violation. Scalars land in +// MarkdownFile.Frontmatter and sequences in MarkdownFile.Lists, so a key +// appears in exactly one map and nothing here knows which keys are lists. // -// Writes go through valueNodeFor, which verifies its own output: it emits with -// goccy's chosen style, re-reads the result, and falls back to a double-quoted -// scalar when the two disagree. goccy's default is wrong for a handful of -// shapes -- a tab is dropped, a value starting "? " produces a document goccy -// itself refuses to parse -- and a hand-written predicate listing them would be -// incomplete, since those cases turned up only by probing. Checking beats -// predicting. +// Writes go through valueNodeFor and elementNodeFor, which verify their own +// output: they emit with goccy's chosen style, re-read the result, and fall +// back to a double-quoted scalar when the two disagree. goccy's default is +// wrong for a handful of shapes -- a tab is dropped, a value starting "? " +// produces a document goccy itself refuses to parse, and a bare comma or "]" +// inside a flow sequence silently changes the list -- and a hand-written +// predicate listing them would be incomplete, since those cases turned up only +// by probing. Checking beats predicting. package frontmatter import ( @@ -160,10 +165,8 @@ func shiftLeadingPosition(msg string) string { return fmt.Sprintf("[%d:%d] %s", line+1, col, msg[len(m[0]):]) } -// scalarValue reads a mapping value as a string. It is a whitelist: every other -// node kind, including an anchor, an alias, a tag, and a "|" literal block, -// reports GetToken().Value as the indicator character rather than the content, -// so a blacklist of sequences and mappings would silently read "&" or "|". +// scalarValue reads a mapping value as a string, rejecting anything that spans +// more than one line. // // Every spelling of null -- an absent value, "null", "~", "Null" -- reads as // "", so a null is unset whatever the author wrote. The old parser mapped only @@ -173,6 +176,17 @@ func scalarValue(key string, n ast.Node) (string, error) { return "", fmt.Errorf("frontmatter %q must be a single-line scalar; "+ "a value split over several lines is not supported", key) } + return plainScalar(key, n) +} + +// plainScalar is the node-kind whitelist shared by scalarValue and +// elementValue. It is a whitelist because every other node kind -- an anchor, +// an alias, a tag, a "|" literal block -- reports GetToken().Value as its +// indicator character rather than its content, so a blacklist of sequences and +// mappings would silently read "&" or "|". A "- |-" sequence element is the +// case that makes this load-bearing twice over: its token does not span lines, +// so the whitelist is the only thing that catches it. +func plainScalar(key string, n ast.Node) (string, error) { switch v := n.(type) { case *ast.NullNode: return "", nil @@ -185,16 +199,59 @@ func scalarValue(key string, n ast.Node) (string, error) { } } +// elementValue reads one sequence element. It shares scalarValue's whitelist +// but applies the line rule to the origin trimmed at *both* ends, because a +// leading newline in an element's origin is structure rather than content: it +// means the element began on a new line, which is true of every block item and +// of a flow sequence wrapped across lines. Trimming only the right, as +// scalarValue does, would refuse "[a,\n b]" for no reason. +// +// What it still refuses is an element whose own value runs past its line -- a +// plain scalar continued on the next line, or a multi-line quoted one -- for +// the same reason scalarValue does. +func elementValue(key string, n ast.Node) (string, error) { + if strings.Contains(strings.TrimSpace(n.GetToken().Origin), "\n") { + return "", fmt.Errorf("frontmatter %q must be a single-line scalar; "+ + "a list element split over several lines is not supported", key) + } + return plainScalar(key, n) +} + +// sequenceValue reads a mapping value as a list of strings. Both YAML spellings +// are accepted -- the flow form "[a, b]" and the block form of "- a" lines -- +// since goccy parses both correctly and a block list survives Normalize intact, +// so refusing one would mean rejecting a file that was understood perfectly. +// What the style does decide is how a rewrite is emitted; see setField. +// +// The element index is carried into the key so a message points at the item +// that is wrong rather than at the field. +func sequenceValue(key string, n *ast.SequenceNode) ([]string, error) { + out := make([]string, 0, len(n.Values)) + for i, e := range n.Values { + s, err := elementValue(fmt.Sprintf("%s[%d]", key, i), e) + if err != nil { + return nil, err + } + out = append(out, s) + } + return out, nil +} + // spansLines reports whether a token's source text runs past its own line. // -// This is what enforces the "no multiline values" half of the flat contract, -// and it has to be enforced at read time rather than trusted: an untouched key -// is re-emitted from the node the parser produced, and goccy's re-emission of a -// parsed node is not identity. A plain scalar continued on the next line comes -// back as a "|-" block, which Parse then refuses -- so UpdateField would write a -// file it cannot read, after create had already made the page. A multi-line -// single-quoted scalar is worse: it re-emits on one line, silently turning -// "sq\nline" into "sq line". +// This is what enforces the single-line half of the contract, and it has to be +// enforced at read time rather than trusted: an untouched key is re-emitted +// from the node the parser produced, and goccy's re-emission of a parsed node +// is not identity. +// +// What that costs differs by shape, measured against the pinned goccy rather +// than assumed. A "|" or ">" block is the one that breaks outright -- it +// re-emits as a block, which plainScalar's whitelist then refuses, so a write +// would produce a file markfluence cannot read, in create only after the page +// had been made. A plain scalar continued on the next line and a multi-line +// quoted one both re-emit folded onto one line, which parses but silently +// rewrites the author's file. Neither is something to do on the way past while +// setting some unrelated field, so both are refused up front. // // Trailing newlines and spaces are stripped first because a token's origin runs // up to the next one, so even `title: T` carries the line break that follows it. @@ -205,18 +262,34 @@ func spansLines(origin string) bool { return strings.Contains(strings.TrimRight(origin, "\n\t "), "\n") } -// toMap reads a mapping into the flat key->value map every caller uses. -func toMap(m *ast.MappingNode) (map[string]string, error) { +// toMaps reads a mapping into the two maps every caller uses: scalars by key, +// and sequences by key. +// +// A sequence-valued key is absent from the scalar map rather than present in +// some flattened spelling. Leaving it there would be worse than absent -- +// MarkdownFile.field would hand update a title of "[a, b]" -- and re-typing the +// scalar map to hold both would touch every caller for no gain. The parser +// learns a kind, not a name: nothing here knows which keys are lists. +func toMaps(m *ast.MappingNode) (map[string]string, map[string][]string, error) { fm := make(map[string]string, len(m.Values)) + lists := map[string][]string{} for _, v := range m.Values { key := v.Key.GetToken().Value + if seq, ok := v.Value.(*ast.SequenceNode); ok { + l, err := sequenceValue(key, seq) + if err != nil { + return nil, nil, err + } + lists[key] = l + continue + } s, err := scalarValue(key, v.Value) if err != nil { - return nil, err + return nil, nil, err } fm[key] = s } - return fm, nil + return fm, lists, nil } // --- writing ------------------------------------------------------------------ @@ -299,33 +372,144 @@ func readsBackAs(n ast.Node, want string) bool { return err == nil && got == want } +// seqIndentColumn is the column block sequence items are emitted at, which +// renders them as " - item". A parsed node's own indentation is not +// reproduced: valid YAML in the right style is the contract, matching an +// author's byte-for-byte indent is not. +const seqIndentColumn = 3 + +// sequenceNodeFor builds the node to write for a list-valued key, in the given +// style. Block items are emitted at seqIndentColumn; with a default position +// they would render flush against the margin, which is valid YAML but not the +// convention anyone writes. +func sequenceNodeFor(values []string, flow bool) ast.Node { + at := pos() + if !flow { + at = &token.Position{Line: 1, Column: seqIndentColumn} + } + seq := ast.Sequence(token.New("", "", at), flow) + for _, v := range values { + seq.Values = append(seq.Values, elementNodeFor(v, flow)) + } + return seq +} + +// elementNodeFor builds one sequence element: goccy's chosen style when it +// reads back, else a double-quoted scalar. valueNodeFor's sibling, minus the +// typed-field rule, which is confined to page_id and parent and neither is a +// list. +func elementNodeFor(value string, flow bool) ast.Node { + n, err := yaml.ValueToNode(value) + if err != nil || !readsBackInSeqAs(n, value, flow) { + return doubleQuoted(value) + } + return n +} + +// readsBackInSeqAs emits n as the only element of a sequence in the style about +// to be written, re-parses it, and reports whether it survived as a string +// holding want. +// +// A sequence needs its own check rather than readsBackAs': a value can read +// back perfectly as a *mapping* value and still be wrong in a list. Measured +// against the pinned goccy, "x,y" and "has]bracket" both pass readsBackAs, but +// emitted bare into a flow sequence the first becomes two elements and the +// second ends the sequence outright. For labels that means publishing two +// labels where the author wrote one -- the exact defect #138's validation +// exists to prevent, arriving from the writer instead of the server. +// +// The style parameter is about minimal quoting, not correctness. Flow is the +// stricter of the two contexts -- a comma and a bracket are significant there +// and inert in block form -- so verifying in flow would be safe for both and +// merely over-quote a block list. Verifying in *block* while writing flow is +// the direction that corrupts, and is what TestSequenceWriteThenReadRoundTrips +// pins by running the hazard corpus through both styles. +// +// Requiring a *string* back is the same point readsBackAs makes: comparing text +// alone says ".inf" round-trips, and block style hands it back as an Infinity +// node that every conforming reader sees as a float. +func readsBackInSeqAs(n ast.Node, want string, flow bool) bool { + m := emptyMapping() + seq := ast.Sequence(token.New("", "", pos()), flow) + seq.Values = append(seq.Values, n) + m.Values = append(m.Values, ast.MappingValue(token.New("", "", pos()), + ast.String(token.New("v", "v", pos())), seq)) + f, err := parser.ParseBytes([]byte(m.String()+"\n"), 0) + if err != nil || len(f.Docs) == 0 { + return false + } + pair := soleMappingPair(f.Docs[0].Body) + if pair == nil { + return false + } + parsed, ok := pair.Value.(*ast.SequenceNode) + if !ok || len(parsed.Values) != 1 { + return false + } + if _, ok := parsed.Values[0].(*ast.StringNode); !ok { + return false + } + got, err := elementValue("v[0]", parsed.Values[0]) + return err == nil && got == want +} + +// soleMappingPair returns the one key/value pair of a single-pair document, or +// nil. goccy renders a one-key mapping as a MappingValueNode in some shapes and +// a MappingNode in others, so a type assertion on either alone silently reports +// "did not read back" and demotes every value to double quotes. +func soleMappingPair(n ast.Node) *ast.MappingValueNode { + switch b := n.(type) { + case *ast.MappingValueNode: + return b + case *ast.MappingNode: + if len(b.Values) == 1 { + return b.Values[0] + } + } + return nil +} + // commentGroup builds a trailing "# text" comment. func commentGroup(text string) *ast.CommentGroupNode { return ast.CommentGroup([]*token.Token{token.New(" "+text, "# "+text, pos())}) } +// nodeFor builds f's value node: a sequence when f is a list, else a scalar. +func nodeFor(f Field, flow bool) ast.Node { + if f.List != nil { + return sequenceNodeFor(f.List, flow) + } + return valueNodeFor(f.Key, f.Value) +} + // valueWithComment builds a value node carrying an optional trailing comment. // The comment goes on the value node: set on the enclosing pair it renders as a // full-line comment above the key instead. -func valueWithComment(key, value, comment string) ast.Node { - v := valueNodeFor(key, value) - if comment != "" { - _ = v.SetComment(commentGroup(comment)) +func valueWithComment(f Field, flow bool) ast.Node { + v := nodeFor(f, flow) + if f.Comment != "" { + _ = v.SetComment(commentGroup(f.Comment)) } return v } // mappingValue builds one `key: value` pair. -func mappingValue(key, value, comment string) *ast.MappingValueNode { +func mappingValue(f Field, flow bool) *ast.MappingValueNode { return ast.MappingValue(token.New("", "", pos()), - ast.String(token.New(key, key, pos())), valueWithComment(key, value, comment)) + ast.String(token.New(f.Key, f.Key, pos())), valueWithComment(f, flow)) } // Field is one frontmatter entry for Render. +// +// List, when non-nil, makes the field a YAML sequence and Value is ignored. +// Non-nil rather than non-empty, so an empty list renders "key: []" -- which is +// a meaningful declaration for a field like labels, where it means "remove +// them all" -- while a nil list stays a scalar. type Field struct { Key string Value string Comment string + List []string } // Render builds a frontmatter block from scratch, in canonical order, @@ -355,7 +539,9 @@ func Render(fields []Field) string { m := emptyMapping() for _, f := range ordered { - m.Values = append(m.Values, mappingValue(f.Key, f.Value, f.Comment)) + // Flow style for a block built from scratch: there is no author choice + // to honour here, and a generated list is short. + m.Values = append(m.Values, mappingValue(f, true)) } if len(m.Values) == 0 { return "---\n---\n" @@ -377,32 +563,48 @@ func Render(fields []Field) string { // token's value re-emits it unquoted whatever it now contains, which is exactly // the bug this package was rewritten to fix. func UpdateField(content, key, value, comment string) (string, error) { + return updateField(content, Field{Key: key, Value: value, Comment: comment}) +} + +// UpdateListField adds or updates key in content's frontmatter as a YAML +// sequence, returning the new content. +// +// A key that already holds a sequence keeps its style: rewriting a block list +// emits a block list. That is a deliberate contract -- a set large enough to be +// written as a block list is exactly the set whose flow spelling is an +// unreadable single line -- but it is style only, not formatting: the items are +// re-emitted at the package's own indent rather than the author's. +func UpdateListField(content, key string, values []string) (string, error) { + return updateField(content, Field{Key: key, List: values}) +} + +func updateField(content string, f Field) (string, error) { loc := frontmatterRE.FindStringSubmatchIndex(content) if loc == nil { - return Render([]Field{{Key: key, Value: value, Comment: comment}}) + content, nil + return Render([]Field{f}) + content, nil } b, err := parseBlock(content[loc[2]:loc[3]]) if err != nil { return "", err } - setField(b, key, value, comment) + setField(b, f) return "---\n" + b.mapping.String() + "\n---\n" + content[loc[1]:], nil } -// setField replaces or inserts key in b's mapping. -func setField(b *block, key, value, comment string) { +// setField replaces or inserts f's key in b's mapping. +func setField(b *block, f Field) { // An existing key keeps its own key node, not just its position: a blank // line before it lives in that node's token origin, so swapping the whole // pair would silently delete it. Only the value is replaced -- and replaced, // never mutated, since mutating a plain token re-emits it unquoted whatever // it now holds. for _, v := range b.mapping.Values { - if v.Key.GetToken().Value == key { - v.Value = valueWithComment(key, value, comment) + if v.Key.GetToken().Value == f.Key { + v.Value = valueWithComment(f, existingSeqIsFlow(v.Value)) return } } - mv := mappingValue(key, value, comment) + mv := mappingValue(f, true) // A comment that had no key to attach to rides along with the first key // added, rather than being dropped on the first write. if b.orphan != nil && len(b.mapping.Values) == 0 { @@ -411,7 +613,7 @@ func setField(b *block, key, value, comment string) { } at := len(b.mapping.Values) for i, v := range b.mapping.Values { - if keyLess(key, v.Key.GetToken().Value) { + if keyLess(f.Key, v.Key.GetToken().Value) { at = i break } @@ -421,6 +623,16 @@ func setField(b *block, key, value, comment string) { b.mapping.Values[at] = mv } +// existingSeqIsFlow reports the style to write a replacement sequence in: the +// style the value being replaced already had, defaulting to flow for anything +// that was not a sequence (a new list, or one replacing a scalar). +func existingSeqIsFlow(current ast.Node) bool { + if seq, ok := current.(*ast.SequenceNode); ok { + return seq.IsFlowStyle + } + return true +} + // Normalize rewrites content's frontmatter in canonical field order, reporting // whether anything moved. // @@ -462,9 +674,15 @@ func isCanonical(m *ast.MappingNode) bool { // // Textual rather than structural because a blank line is not a node: it lives in // the preceding value's token origin, so reordering carries it to a position -// that means nothing. Safe as a text filter because nothing this package emits -// spans more than one line -- a value containing a newline is written as a -// double-quoted scalar with an escape, never as a "|" block. +// that means nothing. +// +// Safe as a text filter because no value this package can emit carries a +// *meaningful* blank line. A scalar holding a newline is written as a +// double-quoted scalar with an escape, on one physical line. A block sequence +// does span lines, and a blank line between two of its items is inert in YAML, +// so dropping it changes nothing but the diff. The shape this would corrupt is +// a "|" block, where a blank line is content -- and that is refused on read, +// which is what keeps this filter honest. func dropBlankLines(s string) string { lines := strings.Split(s, "\n") kept := lines[:0] @@ -479,16 +697,19 @@ func dropBlankLines(s string) string { // --- MarkdownFile --------------------------------------------------------------- // MarkdownFile is a markdown source file parsed once: its path, raw text, -// frontmatter map, and body (content with the frontmatter block stripped). +// frontmatter maps, and body (content with the frontmatter block stripped). // -// Frontmatter is exported so callers that must distinguish absent from -// present-but-blank (e.g. the fix command) can read it directly. The accessor -// methods provide normalized reads: every null spelling and a blank value alike -// read as "". +// Frontmatter holds the scalar fields and Lists the sequence-valued ones; a key +// appears in exactly one of them. Both are exported, and both are non-nil even +// for a file with no frontmatter at all, so callers that must distinguish +// absent from present-but-blank (e.g. the fix command) can read them directly. +// The accessor methods provide normalized reads: every null spelling and a +// blank value alike read as "". type MarkdownFile struct { Filename string Content string Frontmatter map[string]string + Lists map[string][]string Body string } @@ -502,19 +723,20 @@ func Parse(filename, content string) (*MarkdownFile, error) { } return &MarkdownFile{ Filename: filename, Content: content, - Frontmatter: map[string]string{}, Body: content, + Frontmatter: map[string]string{}, Lists: map[string][]string{}, Body: content, }, nil } b, err := parseBlock(content[loc[2]:loc[3]]) if err != nil { return nil, fmt.Errorf("%s: %w", filename, err) } - fm, err := toMap(b.mapping) + fm, lists, err := toMaps(b.mapping) if err != nil { return nil, fmt.Errorf("%s: %w", filename, err) } return &MarkdownFile{ - Filename: filename, Content: content, Frontmatter: fm, Body: content[loc[1]:], + Filename: filename, Content: content, Frontmatter: fm, Lists: lists, + Body: content[loc[1]:], }, nil } diff --git a/internal/frontmatter/frontmatter_test.go b/internal/frontmatter/frontmatter_test.go index 948a7f0..8172903 100644 --- a/internal/frontmatter/frontmatter_test.go +++ b/internal/frontmatter/frontmatter_test.go @@ -97,16 +97,26 @@ func TestUnterminatedFrontmatter(t *testing.T) { } } -// TestRejectedShapes covers everything the flat-scalar contract refuses. Four of +// TestRejectedShapes covers everything the frontmatter contract refuses. Four of // these were accepted-and-mangled by the hand-rolled parser rather than // reported; the anchor, alias, tag and literal cases matter because each reports // its indicator character as its token value, so a whitelist is the only safe -// way to read a scalar. +// way to read a scalar or a list element. +// +// A sequence value is not here: both YAML spellings of a list are accepted now, +// and land in Lists rather than Frontmatter. What is still refused is an +// element that is not a single-line scalar. func TestRejectedShapes(t *testing.T) { tests := []struct{ name, content, wantSubstr string }{ {"colon unquoted", "---\ntitle: a: b\n---\nx\n", "mapping value"}, - {"nested list", "---\ntitle:\n - a\n - b\n---\nx\n", "scalar"}, {"nested map", "---\ntitle:\n a: b\n---\nx\n", "scalar"}, + {"list element literal", "---\nlabels:\n - |\n lit\n---\nx\n", "labels[0]"}, + {"list element anchor", "---\nlabels: [&a foo]\n---\nx\n", "labels[0]"}, + {"list element tag", "---\nlabels: [!!str 12]\n---\nx\n", "labels[0]"}, + {"list element continued", "---\nlabels:\n - a plain\n continued\n---\nx\n", + "single-line scalar"}, + {"list element multiline quoted", "---\nlabels:\n - 'sq\n folded'\n---\nx\n", + "single-line scalar"}, {"literal block", "---\ntitle: |\n lit\n---\nx\n", "scalar"}, {"anchor", "---\ntitle: &a foo\n---\nx\n", "scalar"}, {"tag", "---\ntitle: !!str 12\n---\nx\n", "scalar"}, @@ -505,3 +515,245 @@ func TestNullPageWidthIsUnset(t *testing.T) { } } } + +// --- sequences ---------------------------------------------------------------- + +// list parses content and returns one sequence field. +func list(t *testing.T, content, key string) []string { + t.Helper() + mf, err := frontmatter.Parse("doc.md", content) + if err != nil { + t.Fatalf("Parse(%q) = %v", content, err) + } + return mf.Lists[key] +} + +func updateList(t *testing.T, content, key string, values []string) string { + t.Helper() + got, err := frontmatter.UpdateListField(content, key, values) + if err != nil { + t.Fatalf("UpdateListField(%q, %q, %q) = %v", content, key, values, err) + } + return got +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// TestBothSequenceStylesReadTheSame pins the decision to accept either YAML +// spelling. A block list is valid YAML that goccy parses correctly, so refusing +// it would mean rejecting a file that was understood perfectly -- and a long +// list genuinely reads better as a block list. +// +// The wrapped flow case is the one that distinguishes the element line check +// from scalarValue's: element "b" carries a *leading* newline in its origin, +// which spansLines reports as multi-line even though the element itself is one +// line. +func TestBothSequenceStylesReadTheSame(t *testing.T) { + want := []string{"runbook", "howto", "ci/cd"} + tests := []struct{ name, content string }{ + {"flow", "---\nlabels: [runbook, howto, ci/cd]\n---\nx\n"}, + {"block", "---\nlabels:\n - runbook\n - howto\n - ci/cd\n---\nx\n"}, + {"block unindented", "---\nlabels:\n- runbook\n- howto\n- ci/cd\n---\nx\n"}, + {"flow wrapped", "---\nlabels: [runbook,\n howto,\n ci/cd]\n---\nx\n"}, + {"block with a comment", "---\nlabels:\n # why\n - runbook\n - howto\n - ci/cd\n---\nx\n"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := list(t, tt.content, "labels"); !equalStrings(got, want) { + t.Errorf("labels = %q, want %q", got, want) + } + }) + } +} + +// TestEmptySequenceIsPresentAndEmpty pins the distinction a destructive field +// depends on: "labels: []" is a declaration meaning "remove them all", not an +// absent key. A non-nil empty slice is how Lists says so. +func TestEmptySequenceIsPresentAndEmpty(t *testing.T) { + mf, err := frontmatter.Parse("doc.md", "---\nlabels: []\n---\nx\n") + if err != nil { + t.Fatalf("Parse = %v", err) + } + got, present := mf.Lists["labels"] + if !present { + t.Fatal("labels absent from Lists, want present") + } + if len(got) != 0 { + t.Errorf("labels = %q, want empty", got) + } +} + +// TestSequenceKeyIsAbsentFromScalars pins that a key lands in exactly one map. +// Leaving it in both, with the sequence flattened to some spelling, is how +// MarkdownFile.field would hand update a title of "[a, b]". +func TestSequenceKeyIsAbsentFromScalars(t *testing.T) { + mf, err := frontmatter.Parse("doc.md", "---\ntitle: T\nlabels: [a, b]\n---\nx\n") + if err != nil { + t.Fatalf("Parse = %v", err) + } + if raw, ok := mf.Frontmatter["labels"]; ok { + t.Errorf("Frontmatter[labels] = %q, want absent", raw) + } + if mf.Title() != "T" { + t.Errorf("Title() = %q, want T", mf.Title()) + } +} + +// TestSequenceWriteThenReadRoundTrips is the sequence half of C2, and it runs +// the hazard corpus in *both* styles because the two contexts quote +// differently. Neither covers the other: "a,b" and a "]" read back fine as a +// mapping value but a flow sequence splits the first and the second ends the +// sequence, while block style takes "? q" as YAML's explicit-key indicator. +// +// A single fixed verification context would be a check that passes while the +// write is wrong, which for labels means publishing two labels where the author +// wrote one. +func TestSequenceWriteThenReadRoundTrips(t *testing.T) { + seqHazards := append([]string{"a]b", "[a", "a, b", "x,y", "has]bracket", "a: b"}, hazards...) + styles := []struct { + name string + start string + }{ + {"into flow", "---\nlabels: [old]\n---\nbody\n"}, + {"into block", "---\nlabels:\n - old\n---\nbody\n"}, + {"new key", "---\nk: v\n---\nbody\n"}, + } + for _, st := range styles { + for _, v := range seqHazards { + t.Run(st.name+"/"+v, func(t *testing.T) { + out := updateList(t, st.start, "labels", []string{v, "after"}) + got := list(t, out, "labels") + if !equalStrings(got, []string{v, "after"}) { + t.Errorf("round-trip of %q in %s = %q\nwrote:\n%s", v, st.name, got, out) + } + }) + } + } +} + +// TestUpdateListFieldKeepsTheStyle pins the contract that a block list stays a +// block list through a rewrite. A set large enough to be written as a block +// list is exactly the set whose flow spelling is an unreadable single line, so +// converting it on the first fix that changes a label would defeat the reason +// block form is accepted at all. +// +// Indentation is deliberately not asserted beyond "it is a block list that +// parses": re-emitting an author's 4-space list at 2 spaces is accepted. +func TestUpdateListFieldKeepsTheStyle(t *testing.T) { + tests := []struct { + name, start string + wantFlow bool + }{ + {"flow stays flow", "---\nlabels: [a, b]\n---\nbody\n", true}, + {"block stays block", "---\nlabels:\n - a\n - b\n---\nbody\n", false}, + {"block at four spaces stays block", "---\nlabels:\n - a\n---\nbody\n", false}, + {"new key is flow", "---\ntitle: T\n---\nbody\n", true}, + {"replacing a scalar is flow", "---\nlabels: single\n---\nbody\n", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := updateList(t, tt.start, "labels", []string{"runbook", "howto", "ci/cd"}) + gotFlow := strings.Contains(out, "labels: [") + if gotFlow != tt.wantFlow { + t.Errorf("wrote flow=%v, want %v:\n%s", gotFlow, tt.wantFlow, out) + } + if got := list(t, out, "labels"); !equalStrings(got, []string{"runbook", "howto", "ci/cd"}) { + t.Errorf("labels = %q after write:\n%s", got, out) + } + }) + } +} + +// TestBlockListSurvivesNormalize is what dropBlankLines' safety comment now +// claims. It is a textual filter over the emitted block, and a passed-through +// block list is the first multi-line value it has ever run over: the blank line +// it removes between two items means nothing in YAML, unlike one inside a "|" +// block, which is content and is refused on read. +func TestBlockListSurvivesNormalize(t *testing.T) { + src := "---\nlabels:\n - runbook\n\n - howto\ntitle: T\npage_id: 5\n---\nbody\n" + got, reordered, err := frontmatter.Normalize(src) + if err != nil { + t.Fatalf("Normalize = %v", err) + } + if !reordered { + t.Fatal("reordered = false, want true") + } + if l := list(t, got, "labels"); !equalStrings(l, []string{"runbook", "howto"}) { + t.Errorf("labels = %q after Normalize, want [runbook howto]\n%s", l, got) + } + if strings.Contains(got, "labels: [") { + t.Errorf("Normalize converted a block list to flow:\n%s", got) + } +} + +// TestUnknownListKeySurvivesAWrite pins the generality of sequence support. +// markfluence knows nothing about "reviewers", and nothing may make it care: +// the parser learns a kind, not a name. Without this, an implementation that +// special-cased "labels" in toMaps would satisfy every other test here and +// still break the moment #21 or #100 adds a second list field. +func TestUnknownListKeySurvivesAWrite(t *testing.T) { + src := "---\ntitle: T\nreviewers: [ana, bo]\n---\nbody\n" + out := update(t, src, "page_id", "12345", "") + if got := list(t, out, "reviewers"); !equalStrings(got, []string{"ana", "bo"}) { + t.Errorf("reviewers = %q, want [ana bo]\n%s", got, out) + } + if !strings.Contains(out, "reviewers: [ana, bo]") { + t.Errorf("reviewers was re-emitted rather than passed through:\n%s", out) + } +} + +// TestRenderListField pins Render's list form against UpdateListField's, the +// same way TestRenderAndUpdateFieldAgree does for scalars: two writers that can +// disagree are two writers that will. +func TestRenderListField(t *testing.T) { + rendered := frontmatter.Render([]frontmatter.Field{ + {Key: "title", Value: "T"}, + {Key: "labels", List: []string{"runbook", "howto"}}, + {Key: "page_id", Value: "5"}, + }) + updated := updateList(t, "---\ntitle: T\npage_id: 5\n---\n", "labels", + []string{"runbook", "howto"}) + if !strings.Contains(rendered, "labels: [runbook, howto]") { + t.Errorf("Render wrote:\n%s", rendered) + } + if !strings.Contains(updated, "labels: [runbook, howto]") { + t.Errorf("UpdateListField wrote:\n%s", updated) + } + // Both order labels after page_id: it is not in fieldOrder, so it sorts + // alphabetically among the trailing keys. + if strings.Index(rendered, "labels:") < strings.Index(rendered, "page_id:") { + t.Errorf("Render put labels before page_id:\n%s", rendered) + } +} + +// TestEmptyListIsWrittenAsFlowEmpty pins that an empty list writes "[]" rather +// than a bare key, which would read back as a null scalar and so as an absent +// field -- turning "remove every label" into "do not touch the labels". +func TestEmptyListIsWrittenAsFlowEmpty(t *testing.T) { + for _, start := range []string{ + "---\nlabels: [a, b]\n---\nbody\n", + "---\nlabels:\n - a\n---\nbody\n", + } { + out := updateList(t, start, "labels", []string{}) + if !strings.Contains(out, "labels: []") { + t.Errorf("wrote:\n%s\nwant a \"labels: []\" line", out) + } + mf, err := frontmatter.Parse("doc.md", out) + if err != nil { + t.Fatalf("Parse = %v", err) + } + if got, present := mf.Lists["labels"]; !present || len(got) != 0 { + t.Errorf("labels = %q present=%v, want present and empty", got, present) + } + } +} From 5050057322690942d24efe720ed93c903e550494 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:48:43 -0400 Subject: [PATCH 03/16] docs(confluence): record how page labels behave The verified findings behind #138's design, so the client's comments have something to cite. Probed 2026-09-08 against a scratch page: names POSTed one at a time via v1, read back through v1 and v2, both delete forms exercised. The one to read before touching any of this: Confluence splits a label name on spaces and commas, silently and with a 200. `Runbook Two` becomes `runbook` + `two`, which under assert-exactly reads back as neither and so is re-added on every run with no frontmatter spelling that can remove it. Two labels in the SRE space look like this having already happened to somebody. Also recorded: the server's own reject set (a colon is in it, so there is no prefix syntax to offer an author), the 255-**UTF-16-code-unit** cap that a byte or rune check would get wrong in opposite directions, v2 being read-only for labels, and the `?name=` removal form -- the path form 400s with a Tomcat HTML body once a name holds a `/`, and `ci/cd` is a real label in the SRE space, so that is a real failure on a real page rather than a hypothetical. The scope for the two v1 write routes goes in the table as unverified; api.md already records that v1 scopes cannot be looked up. --- docs/confluence/README.md | 1 + docs/confluence/api.md | 3 + docs/confluence/labels.md | 148 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 152 insertions(+) create mode 100644 docs/confluence/labels.md diff --git a/docs/confluence/README.md b/docs/confluence/README.md index 083078f..224b949 100644 --- a/docs/confluence/README.md +++ b/docs/confluence/README.md @@ -10,6 +10,7 @@ person who ran the experiment. - [storage-format.md](storage-format.md) — tables, macros, what Confluence rewrites - [links-and-anchors.md](links-and-anchors.md) — heading anchors and page links - [page-width.md](page-width.md) — the content properties behind `page_width` +- [labels.md](labels.md) — what Confluence does to a label name, and why removal is `?name=` - [folders.md](folders.md) — the Cloud folder type, and why child listing is v1 - [spaces.md](spaces.md) — what sits at the top of a space, and how to enumerate it - [search.md](search.md) — finding content by title and by full text, and `/search`'s paging traps diff --git a/docs/confluence/api.md b/docs/confluence/api.md index 0ffd20e..228e46d 100644 --- a/docs/confluence/api.md +++ b/docs/confluence/api.md @@ -275,6 +275,9 @@ below. | `ListAttachments` | v1 | `GET /content/{id}/child/attachment` | **undocumented, see below** | | `ListChildPages` | v1 | `GET /content/{id}/child/page` | **undocumented, see below** | | `ListChildFolders` | v1 | `GET /content/{id}/child/folder` | **undocumented, see below** | +| `ListLabels` | v2 | `GET /pages/{id}/labels` | `read:page:confluence` | +| `AddLabels` | v1 | `POST /content/{id}/child/label` | **unverified**, presumably `write:confluence-content` | +| `RemoveLabel` | v1 | `DELETE /content/{id}/child/label?name=…` | **unverified**, presumably `write:confluence-content` | Union, which is what a token needs: diff --git a/docs/confluence/labels.md b/docs/confluence/labels.md new file mode 100644 index 0000000..46eebf3 --- /dev/null +++ b/docs/confluence/labels.md @@ -0,0 +1,148 @@ +# Labels + +A **label** is a short tag on a page. Labels are how Confluence content is +actually organized and searched — the SRE space carries 94 distinct labels +across 1123 pages — and `search --cql 'label = "runbook"'` is only useful for +pages someone labeled. + +Reads are v2; **writes are v1**, exactly like attachments. + +## Verified 2026-09-08 + +Probed against `mozilla-hub.atlassian.net` with a scratch page in a personal +space: names POSTed one at a time via v1, read back through both v1 and v2, +both delete forms exercised, then the page purged. + +### A space or a comma is a separator, not a character + +This is the finding everything else here defends against. + +| POSTed name | labels on the page afterward | +|---|---| +| `Runbook Two` | `runbook`, `two` | +| `a,b` | `a`, `b` | +| `trail ` | `trail` | + +HTTP 200 every time, with no warning of any kind. The name is split first, then +each piece is validated. + +Under an assert-exactly rule this does not merely mis-tag a page, it never +converges: `labels: [Runbook Two]` publishes as two labels, reads back as +neither, and so is re-added on **every** run, forever, with no spelling of the +frontmatter that can remove it. Two rows in the SRE space look like this having +already happened to a person — `continuous` + `delivery` and `url` + +`shortener`, each appearing exactly once, on pages where a human plainly typed a +two-word label. + +### A colon is refused, so there is no prefix syntax + +| POSTed name | result | +|---|---| +| `my:foo` | 400 `label.contains.invalid.chars` | +| `global:x` | 400, same | +| `team:eng` | 400, same | + +The 400 body names the reject set: + +``` +space ! # & ( ) * , . : ; < > ? @ [ ] ^ +``` + +No silent prefix-splitting, and no way for an author to write a prefix at all. +A tab is refused too, and is not in that list. + +Note what `.` being invalid costs: a version-shaped label (`v1.2`) is +impossible. That is Confluence's rule, not markfluence's, which is why the +error message quotes the set rather than making an author guess. + +### Names are lowercased, and the cap is UTF-16 code units + +| input | result | +|---|---| +| `UPPER` | `upper` | +| `HÉLLO` | `héllo` — Unicode-aware, not ASCII-only | +| 255 × `é` (510 bytes) | 200 | +| 256 × `é` | 400 `label.name.is.too.long` | +| 128 emoji (128 code points, 256 UTF-16 units) | 400 `label.name.is.too.long` | +| empty | 400, a parse error rather than a validation one | + +So the cap is **255 UTF-16 code units** — not bytes and not runes, and the Go +check is `len(utf16.Encode([]rune(s))) <= 255`. A byte-based check would refuse +a legal 255-character accented label; a rune-based one would accept an emoji +label the server rejects. + +Accepted verbatim: `/`, `"`, `'`, `+`, `_`, `-`, digits, non-ASCII +(`héllo-wörld`), emoji. The character inventory in real use across the SRE +space is lowercase alphanumerics, `-`, `_`, and a single `/`. + +### Adding is additive and idempotent; there is no bulk-set route + +`POST /wiki/rest/api/content/{id}/child/label` takes an array and returns the +page's whole label list. Re-POSTing a name the page already carries is a clean +200. Nothing sets a page's labels to a given set in one call, so "assert exactly +this set" is add-the-missing plus remove-the-extra, computed client-side. + +### Removal is one DELETE per label, and must use the `?name=` form + +| request | result | +|---|---| +| `DELETE …/child/label/runbook` | 204 | +| `DELETE …/child/label/a%2Fb` | **400**, and the body is Tomcat's HTML error page, not JSON | +| `DELETE …/child/label?name=a/b` | 204 | +| `DELETE …/child/label?name=absent` | 404 | + +The path form works right up until a name contains a `/`, which no amount of +percent-encoding fixes. This is not hypothetical: **`ci/cd` is a real label in +the SRE space**, so the path form would fail on a real page on a real run. Use +the query form only. + +A 404 means the label is already gone, which for a removal is the desired state +— but only when it is a genuine 404. A rejected credential answers every v2 +route with a 404 too, so that check goes through `notFound` rather than +comparing the status directly (see [api.md](api.md)). + +### v2 is read-only for labels + +| request | result | +|---|---| +| `GET /wiki/api/v2/pages/{id}/labels` | 200 | +| `POST /wiki/api/v2/pages/{id}/labels` | 405 `METHOD_NOT_ALLOWED` | +| `DELETE /wiki/api/v2/pages/{id}/labels` | 405, same | + +Hence the split: read v2, write v1. + +### Both pagination shapes are ones the client already has + +| route | pages by | helper | +|---|---|---| +| v2 `GET /pages/{id}/labels` | the cursor in `_links.next`, a `/wiki`-prefixed absolute path | `listV2`, `resolveNext` unchanged | +| v1 `GET /content/{id}/label` | `start`/`limit` offset | `listV1` | + +Neither is the `/wiki/rest/api/search` case, which is neither of these — see +[search.md](search.md). + +### Neither GET returns a useful order + +v2 sorts by label id; v1 differs again. Neither matches what the UI shows. So +any output has to be sorted locally or it is unstable across runs for no reason +a reader could explain. + +### Every label in the SRE space is `global:` + +A survey of all 1123 pages found 94 distinct labels and **not one** carrying a +`my:`, `team:`, or `system:` prefix. That is the evidence for markfluence +managing `global:` and nothing else: the other prefixes are read and displayed, +never written and never removed. + +`my:` labels are personal (visible only to the account that set them) and +`team:` ones belong to a team space's own vocabulary. Removing either on an +author's behalf, in the course of asserting a frontmatter field that has no +syntax for them, would be deleting data the file could not have expressed. + +## What is not verified + +- **The OAuth scope for the v1 label routes.** Presumably + `write:confluence-content`, which [api.md](api.md#scopes) already records as + unlookupable for v1 routes. A scoped-token run will settle it. +- **Whether the 255-unit cap is enforced on the v2 read path.** Irrelevant + unless a label was created by some other client that bypassed it. From 23027eb61d67a545e0fa8e5e928176e545005ba9 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:50:28 -0400 Subject: [PATCH 04/16] feat(client): list, add, and remove page labels Reads are v2, writes are v1 -- the same split attachments have, and not a choice: v2 answers POST and DELETE on a page's label collection with a 405. ListLabels returns the list **unfiltered**. Callers asserting a frontmatter field filter to the "global" prefix themselves, because info shows every label a page carries including the my:/team: ones no frontmatter could express, and a client that dropped them would make that impossible. RemoveLabel puts the name in the query string, never the path. The path form works right up until a name holds a "/", which answers 400 with a Tomcat HTML body however the slash is encoded, while ?name= answers 204 for the same label -- and "ci/cd" is a real label in the SRE space, so this is a real failure on a real page. The test asserts the path and the query separately, since a name appended to the path would leave the query empty and still 204 against a lenient fake. Its 404 check goes through notFound rather than comparing the status, because a rejected credential is also a 404 with a body that names nothing. Read as "already gone", that would report a whole batch of removals as a success against a page nobody can read. Pinned by a test that fails if the gate is weakened to a status comparison. AddLabels batches into one request and makes none at all for an empty list, so publishing a file with no labels costs nothing. Every name must already be valid when it gets here: Confluence splits a name on spaces and commas with no warning and a 200, so the validation that prevents it lives in internal/labels and runs before any write. --- internal/client/client.go | 76 ++++++++++++++++ internal/client/client_test.go | 161 +++++++++++++++++++++++++++++++++ 2 files changed, 237 insertions(+) diff --git a/internal/client/client.go b/internal/client/client.go index 613e1e1..fd4736c 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -1409,3 +1409,79 @@ func (c *ConfluenceClient) trySetContentProperty(pageID, key, value string) (str } return "set", nil } + +// --- labels ------------------------------------------------------------------ + +// Label is one label on a page: its name and its prefix. The prefix is a +// namespace Confluence keeps separately from the name and that no name can +// contain, since a colon is in the set of characters a label name refuses +// outright (see docs/confluence/labels.md). +// +// markfluence manages "global" labels only. The prefix is reported rather than +// filtered here because info shows every label a page carries, including the +// ones no frontmatter field could express. +type Label struct { + ID string `json:"id"` + Name string `json:"name"` + Prefix string `json:"prefix"` +} + +// ListLabels returns every label on a page, unfiltered, following pagination. +// +// v2 for the read, v1 for the writes below -- the same split attachments have, +// and not a choice: v2 answers POST and DELETE on this collection with a 405. +// +// Unfiltered deliberately. Callers that assert a frontmatter field filter to +// the "global" prefix themselves; info needs the whole list to show what it +// cannot manage. +func (c *ConfluenceClient) ListLabels(pageID string) ([]Label, error) { + return listV2[Label](c, "/wiki/api/v2/pages/"+pageID+"/labels", nil) +} + +// AddLabels adds labels to a page in one request, returning nil for an empty +// list without calling anything. +// +// The v1 route takes an array and is additive and idempotent: re-adding a name +// the page already carries is a clean 200, and the response is the page's whole +// label list. There is no bulk *set* route, so asserting an exact set is this +// call plus RemoveLabel for the surplus, diffed client-side. +// +// Every name must already be valid. Confluence splits a name on spaces and +// commas with no warning and a 200, so posting "Runbook Two" silently creates +// two labels that no later read can attribute back -- see +// docs/confluence/labels.md. Validation lives in internal/labels and runs +// before any write. +func (c *ConfluenceClient) AddLabels(pageID string, names []string) error { + if len(names) == 0 { + return nil + } + payload := make([]map[string]string, 0, len(names)) + for _, name := range names { + payload = append(payload, map[string]string{"prefix": "global", "name": name}) + } + path := c.baseURL + "/wiki/rest/api/content/" + pageID + "/child/label" + return c.doJSON(http.MethodPost, path, nil, payload, nil, timeoutWrite) +} + +// RemoveLabel removes one label from a page. A label that is not there is +// success: the desired state is "absent", and it already is. +// +// The name goes in the **query string**, never the path. The path form +// (DELETE .../child/label/{name}) works until a name contains a "/", which +// answers 400 with a Tomcat HTML error page no matter how the slash is encoded, +// while ?name= answers 204 for the same label. That is not a hypothetical +// shape: "ci/cd" is a real label in the SRE space. Measured both ways in +// docs/confluence/labels.md. +// +// The 404 check goes through notFound rather than comparing the status, because +// a rejected credential is also a 404 -- on a whole batch of removals that +// would otherwise report every label as "already gone" and the run as a +// success. +func (c *ConfluenceClient) RemoveLabel(pageID, name string) error { + path := c.baseURL + "/wiki/rest/api/content/" + pageID + "/child/label" + err := c.doJSON(http.MethodDelete, path, url.Values{"name": {name}}, nil, nil, timeoutWrite) + if notFound(err) { + return nil + } + return err +} diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 7a1004a..b104052 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -1845,3 +1845,164 @@ func TestRequestErrorIsTransparent(t *testing.T) { t.Error("wrapRequest(nil) != nil") } } + +// --- labels ------------------------------------------------------------------ + +// recorder captures every request a call makes: method, path, and raw query. +// The label tests assert on the *shape* of the request rather than only its +// result, because the two things most likely to be got wrong here -- the +// removal form and the v1/v2 split -- are invisible in a 204. +type recorder struct { + methods []string + paths []string + queries []string + bodies []string +} + +func newRecordingServer(t *testing.T, responses ...resp) (*ConfluenceClient, *recorder) { + t.Helper() + rec := &recorder{} + idx := 0 + c := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + rec.methods = append(rec.methods, r.Method) + rec.paths = append(rec.paths, r.URL.Path) + rec.queries = append(rec.queries, r.URL.RawQuery) + rec.bodies = append(rec.bodies, string(body)) + if idx >= len(responses) { + t.Errorf("unexpected extra request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(500) + return + } + out := responses[idx] + idx++ + w.WriteHeader(out.status) + _, _ = w.Write([]byte(out.body)) + }) + return c, rec +} + +func TestListLabelsReadsPrefixes(t *testing.T) { + c, rec := newRecordingServer(t, resp{200, `{"results":[ + {"id":"1","name":"runbook","prefix":"global"}, + {"id":"2","name":"mine","prefix":"my"} + ]}`}) + got, err := c.ListLabels("123") + if err != nil { + t.Fatal(err) + } + if len(got) != 2 { + t.Fatalf("len = %d, want 2", len(got)) + } + if got[0].Name != "runbook" || got[0].Prefix != "global" { + t.Errorf("got[0] = %+v", got[0]) + } + // Unfiltered: info needs the my: label, so the client must not drop it. + if got[1].Prefix != "my" { + t.Errorf("got[1] = %+v, want the my: label kept", got[1]) + } + if want := "/wiki/api/v2/pages/123/labels"; rec.paths[0] != want { + t.Errorf("path = %q, want %q", rec.paths[0], want) + } +} + +// TestListLabelsFollowsTheCursor pins the v2 pagination shape. A label +// collection reports _links.next whenever more remains, so termination is its +// absence -- the opposite of the v1 short-page rule, and picking the wrong one +// truncates silently. +func TestListLabelsFollowsTheCursor(t *testing.T) { + c, rec := newRecordingServer(t, + resp{200, `{"results":[{"id":"1","name":"a","prefix":"global"}], + "_links":{"next":"/wiki/api/v2/pages/123/labels?cursor=X"}}`}, + resp{200, `{"results":[{"id":"2","name":"b","prefix":"global"}]}`}, + ) + got, err := c.ListLabels("123") + if err != nil { + t.Fatal(err) + } + if len(got) != 2 { + t.Fatalf("len = %d, want 2 across both pages", len(got)) + } + if len(rec.paths) != 2 { + t.Fatalf("paths = %v, want two requests", rec.paths) + } + if !strings.Contains(rec.queries[1], "cursor=X") { + t.Errorf("second query = %q, want the cursor", rec.queries[1]) + } +} + +// TestAddLabelsPostsOneV1Request pins the batch and the prefix. Writes are v1: +// v2 answers POST on this collection with a 405. +func TestAddLabelsPostsOneV1Request(t *testing.T) { + c, rec := newRecordingServer(t, resp{200, `{"results":[]}`}) + if err := c.AddLabels("123", []string{"runbook", "ci/cd"}); err != nil { + t.Fatal(err) + } + if len(rec.methods) != 1 || rec.methods[0] != http.MethodPost { + t.Fatalf("methods = %v, want one POST", rec.methods) + } + if want := "/wiki/rest/api/content/123/child/label"; rec.paths[0] != want { + t.Errorf("path = %q, want %q", rec.paths[0], want) + } + for _, want := range []string{`"name":"runbook"`, `"name":"ci/cd"`, `"prefix":"global"`} { + if !strings.Contains(rec.bodies[0], want) { + t.Errorf("body = %s, want it to contain %s", rec.bodies[0], want) + } + } +} + +// TestAddLabelsSkipsAnEmptyBatch: nothing to add must not be a request, or +// every publish of a file with no labels costs one. +func TestAddLabelsSkipsAnEmptyBatch(t *testing.T) { + c, rec := newRecordingServer(t) + if err := c.AddLabels("123", nil); err != nil { + t.Fatal(err) + } + if len(rec.methods) != 0 { + t.Errorf("methods = %v, want no request", rec.methods) + } +} + +// TestRemoveLabelUsesTheQueryForm is the one that matters most in this file. +// The path form works right up until a name holds a "/", which 400s with a +// Tomcat HTML body however it is encoded -- and "ci/cd" is a real label in the +// SRE space, so the path form fails on a real page. Asserting on the path and +// query separately is what catches a regression to it: a name appended to the +// path would leave the query empty and still 204 against a lenient fake. +func TestRemoveLabelUsesTheQueryForm(t *testing.T) { + c, rec := newRecordingServer(t, resp{204, ""}) + if err := c.RemoveLabel("123", "ci/cd"); err != nil { + t.Fatal(err) + } + if want := "/wiki/rest/api/content/123/child/label"; rec.paths[0] != want { + t.Errorf("path = %q, want exactly %q with the name in the query", rec.paths[0], want) + } + if want := "name=ci%2Fcd"; rec.queries[0] != want { + t.Errorf("query = %q, want %q", rec.queries[0], want) + } + if rec.methods[0] != http.MethodDelete { + t.Errorf("method = %q, want DELETE", rec.methods[0]) + } +} + +// TestRemoveLabelTreatsAbsentAsDone: the desired state of a removal is +// "absent", and a 404 says it already is. +func TestRemoveLabelTreatsAbsentAsDone(t *testing.T) { + c, _ := newRecordingServer(t, resp{404, `{"message":"No label with name [gone]"}`}) + if err := c.RemoveLabel("123", "gone"); err != nil { + t.Errorf("RemoveLabel = %v, want nil for an absent label", err) + } +} + +// TestRemoveLabelDoesNotSwallowARejectedCredential is why the 404 check goes +// through notFound rather than comparing the status. A revoked token answers +// with a 404 whose body names nothing, and reading that as "already gone" would +// report a whole batch of removals as a success against a page nobody can even +// read. +func TestRemoveLabelDoesNotSwallowARejectedCredential(t *testing.T) { + c, _ := newRecordingServer(t, resp{404, `{"errors":[{"status":404,"title":"Not Found"}]}`}) + err := c.RemoveLabel("123", "runbook") + if err == nil { + t.Fatal("RemoveLabel = nil, want the credential failure reported") + } +} From 818c99f08e15c7c1ab611623d0531a180b35be6c Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:53:11 -0400 Subject: [PATCH 05/16] feat(labels): validate and reconcile a page's label set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The vocabulary package for the labels frontmatter field, modeled on internal/pagewidth: Declared reads and validates, Diff answers what to add and remove, Apply/Plan run it against the client. Set arithmetic lives here because three commands need the same answer and three copies of a set difference is three chances to leave a label behind. Validation mirrors Confluence's own reject set rather than inventing an allowlist, so ci/cd, dataops_reports and héllo-wörld publish unchanged. It refuses rather than repairs, because the input it exists to catch is silently destructive: Confluence splits a name on a space or comma with a 200 and no warning, so `labels: [Runbook Two]` publishes two labels that read back as neither and are re-added on every run, forever, with no frontmatter spelling that removes them. The error quotes the reject set, since half of it is surprising -- a "." means a version-shaped label is impossible. Case is the one repair, with a warning naming the label as written, because Confluence lowercases server-side and a file that disagreed would never converge. A leading or trailing space is *not* trimmed: a space is a separator there, so trimming would quietly repair the exact class of input that produced the "continuous" + "delivery" pair in the SRE space. Length is counted in UTF-16 code units, pinned at the three points measured against the server (255 accented runes pass, 256 fail, 128 emoji fail). A byte check fails the first and a rune check passes the last, so the test catches a drift in either direction. Three shapes kept deliberately distinct: - absent: the page's labels are untouched, so a hand-labeled page survives a run that never mentioned labels; - `[]`: remove every managed label; - a scalar, including every null spelling: an error. The field is destructive, so `labels:` with nothing after it must not be read as "strip this page" on a file where an author typed a key and got distracted. Those cases run through frontmatter.Parse rather than a hand-built map, since the parser is what flattens "~" and "null" to "" and a test that passed "" three times would prove one case and claim three. Only the global prefix is managed. A my: label is personal and a team: one belongs to a space's vocabulary; neither has a frontmatter spelling, so removing one would delete data the file could not have expressed. --- internal/labels/labels.go | 351 +++++++++++++++++++++++++++++++++ internal/labels/labels_test.go | 294 +++++++++++++++++++++++++++ 2 files changed, 645 insertions(+) create mode 100644 internal/labels/labels.go create mode 100644 internal/labels/labels_test.go diff --git a/internal/labels/labels.go b/internal/labels/labels.go new file mode 100644 index 0000000..c05be2e --- /dev/null +++ b/internal/labels/labels.go @@ -0,0 +1,351 @@ +// Package labels models the labels frontmatter field and its mapping to +// Confluence's page labels. +// +// The field is a YAML sequence of names: +// +// labels: [ci/cd, howto, runbook] +// +// Its semantics mirror page_width's asymmetry, for the same reason: a declared +// field is asserted, an absent one is left alone. Declared means the set is +// made exact -- a label on the page that is not in the file is removed -- and +// "labels: []" means remove every managed label. An absent key means +// markfluence does not touch the page's labels at all, so a page labeled by +// hand is not silently stripped by a run that never mentioned labels. +// +// markfluence manages the "global" prefix and nothing else. A my:, team: or +// system: label is read and displayed, never written and never removed: a +// colon cannot appear in a label name at all, so there is no frontmatter +// spelling for those and removing one would delete data the file could not +// have expressed. Every label in the SRE space is global (see +// docs/confluence/labels.md). +// +// Validation mirrors the server's own reject set rather than inventing an +// allowlist, so real labels like ci/cd, dataops_reports and héllo-wörld publish +// unchanged. It refuses rather than repairs, with one exception: case, which is +// lowercased locally with a warning, because Confluence lowercases server-side +// and a file that disagreed would never converge. +// +// The vocabulary logic is pure; Apply and Read orchestrate the client calls. +package labels + +import ( + "fmt" + "sort" + "strings" + "unicode" + "unicode/utf16" + + "github.com/mozilla/markfluence/internal/client" +) + +// Field is the frontmatter key this package owns. +const Field = "labels" + +// ManagedPrefix is the only label namespace markfluence writes or removes. +const ManagedPrefix = "global" + +// RejectChars is the set of characters Confluence refuses in a label name, +// taken from its own 400 body rather than guessed: +// +// label.contains.invalid.chars … space ! # & ( ) * , . : ; < > ? @ [ ] ^ +// +// The space and the comma are the dangerous two. Confluence treats them as +// *separators*, splitting a name and validating the pieces, so "Runbook Two" +// posts as two labels with a 200 and no warning -- which under the +// assert-exactly rule reads back as neither and is re-added on every run +// forever. Refusing them is the whole point of validating at all. +// +// A colon being in the set is why there is no prefix syntax, and a "." being +// in it is why a version-shaped label (v1.2) is impossible. That is +// Confluence's rule, not markfluence's, which is why the error quotes the set +// instead of leaving an author to guess. +const RejectChars = ` !#&()*,.:;<>?@[]^` + +// MaxNameUnits is the longest label name Confluence accepts, in **UTF-16 code +// units** -- not bytes and not runes. Measured: 255 × "é" (510 bytes) is +// accepted, 256 × "é" is not, and 128 emoji (128 runes, 256 units) is not. A +// byte-based check would refuse a legal 255-character accented label and a +// rune-based one would accept an emoji label the server rejects. +const MaxNameUnits = 255 + +// Set is the label set a file asserts: the normalized names, whether the key +// was present at all, and any warning raised while normalizing. +// +// One struct rather than four return values because Declared is the call every +// command makes, and a signature of (names, declared, warnings, err) is four +// opportunities to drop the warnings on the floor. +type Set struct { + // Names is sorted and deduplicated, and every entry is valid. + Names []string + + // Declared reports whether the file had a labels key at all. False means + // the live page's labels are not touched; it is not the same as an empty + // Names, which means remove them all. + Declared bool + + // Warnings are author-facing notes about what normalization changed. + Warnings []string +} + +// Action is the outcome for one label in an asserted set. +type Action struct { + Name string + Action string // "added", "removed", or "unchanged" +} + +// Action values. +const ( + ActionAdded = "added" + ActionRemoved = "removed" + ActionUnchanged = "unchanged" +) + +// Declared reads and validates the labels field from a file's frontmatter. +// +// It takes both maps because a labels key can arrive in either, and the two +// mean different things. A sequence is the field; a *scalar* is an error rather +// than a one-element list, because this field is destructive -- declaring it +// removes every label not listed -- so "labels:" with nothing after it would +// otherwise mean "strip every label off this page" on a file where an author +// most likely typed a key and got distracted. "labels: []" stays the one way to +// say that, and it has to be written on purpose. +func Declared(lists map[string][]string, frontmatter map[string]string) (Set, error) { + if raw, scalar := frontmatter[Field]; scalar { + // An empty or null value gets its own message. Pointing an author at + // "labels: []" when that is nearly what they already wrote would read + // as a formatting nit, when the actual point is that the two mean + // opposite things: one is unfinished, the other strips the page. + if strings.TrimSpace(raw) == "" { + return Set{}, fmt.Errorf( + "frontmatter %q has no value; remove the key to leave the page's "+ + "labels alone, or write %s [] to remove every label", Field, Field+":") + } + return Set{}, fmt.Errorf( + "frontmatter %q must be a list, not a single value: write %s [%s]", + Field, Field+":", strings.TrimSpace(raw)) + } + declared, ok := lists[Field] + if !ok { + return Set{}, nil + } + + seen := make(map[string]bool, len(declared)) + set := Set{Declared: true, Names: make([]string, 0, len(declared))} + for _, raw := range declared { + name, changed := Normalize(raw) + if err := Validate(name); err != nil { + return Set{}, err + } + if changed { + set.Warnings = append(set.Warnings, fmt.Sprintf( + "label %q is not lowercase; publishing it as %q, which is what "+ + "Confluence stores. Update the file to match.", raw, name)) + } + if seen[name] { + continue + } + seen[name] = true + set.Names = append(set.Names, name) + } + sort.Strings(set.Names) + return set, nil +} + +// Normalize lowercases a label name, reporting whether that changed it. +// +// Lowercasing is the only repair this package makes, and it is a repair rather +// than a refusal because Confluence lowercases server-side: a file declaring +// "Runbook" would read back "runbook", differ from itself, and be rewritten on +// every run. Lowercasing locally is what makes the comparison converge. The +// caller warns, so the author can make the file say what will happen. +// +// Note what is *not* trimmed. A leading or trailing space is refused by +// Validate like an inner one, because a space is a separator server-side and +// silently trimming it would repair exactly the class of input that produced +// the "continuous" + "delivery" pair in the SRE space. +func Normalize(name string) (string, bool) { + lowered := strings.ToLower(name) + return lowered, lowered != name +} + +// Validate reports why a label name is unusable, or nil. +// +// Offline and exact: every rule here is one the server enforces, so a name that +// passes is a name that publishes as itself. The reject set is quoted in the +// message because half of it is surprising -- a "." means v1.2 is impossible -- +// and an author should not have to bisect their own label to find out. +func Validate(name string) error { + if strings.TrimSpace(name) == "" { + if name == "" { + return fmt.Errorf("label is empty") + } + return fmt.Errorf("label %q is only whitespace", name) + } + if i := strings.IndexAny(name, RejectChars); i >= 0 { + return fmt.Errorf( + "label %q contains %q, which Confluence refuses; a space or comma is "+ + "a separator there, not a character, so %q would publish as "+ + "several labels. Invalid: %s", + name, string(name[i]), name, describeRejectChars()) + } + // A tab is refused server-side too and is not in the reject set, so the + // whitespace check is separate rather than folded into it. + for _, r := range name { + if unicode.IsSpace(r) { + return fmt.Errorf("label %q contains whitespace (%q), which Confluence refuses", + name, string(r)) + } + } + if n := len(utf16.Encode([]rune(name))); n > MaxNameUnits { + return fmt.Errorf("label %q is %d UTF-16 code units; Confluence allows %d", + name, n, MaxNameUnits) + } + return nil +} + +// describeRejectChars renders the reject set for an error message, naming the +// space rather than printing one where it would be invisible. +func describeRejectChars() string { + parts := []string{"space"} + for _, r := range RejectChars { + if r != ' ' { + parts = append(parts, string(r)) + } + } + return strings.Join(parts, " ") +} + +// Global returns the names of the managed labels in a page's label list, +// sorted. The unmanaged ones are dropped here rather than by the client, +// because info needs the full list. +func Global(live []client.Label) []string { + out := make([]string, 0, len(live)) + for _, l := range live { + if l.Prefix == ManagedPrefix { + out = append(out, l.Name) + } + } + sort.Strings(out) + return out +} + +// Unmanaged returns the labels markfluence will not touch, as "prefix:name", +// sorted. Only info shows these. +func Unmanaged(live []client.Label) []string { + out := make([]string, 0, len(live)) + for _, l := range live { + if l.Prefix != ManagedPrefix { + out = append(out, l.Prefix+":"+l.Name) + } + } + sort.Strings(out) + return out +} + +// Diff reports what asserting declared over live requires: what to add, what to +// remove, and what is already right. +// +// Sets, not sequences: order and duplicates carry no meaning to Confluence, so +// a file whose list is merely differently ordered needs no write at all. Every +// returned slice is sorted, because neither label GET returns a useful order +// and unsorted output would be unstable across runs for no reason a reader +// could explain. +// +// This lives here so no command reimplements it. Three commands need the same +// answer, and three copies of a set difference is three chances to leave a +// label behind. +func Diff(declared, live []string) (add, remove, unchanged []string) { + inLive := make(map[string]bool, len(live)) + for _, l := range live { + inLive[l] = true + } + inDeclared := make(map[string]bool, len(declared)) + for _, d := range declared { + inDeclared[d] = true + } + for d := range inDeclared { + if inLive[d] { + unchanged = append(unchanged, d) + } else { + add = append(add, d) + } + } + for l := range inLive { + if !inDeclared[l] { + remove = append(remove, l) + } + } + sort.Strings(add) + sort.Strings(remove) + sort.Strings(unchanged) + return add, remove, unchanged +} + +// Actions renders a diff as the per-label report both the human and --json +// paths print: the full declared set plus whatever was removed, sorted by name. +// +// The full set rather than only the changes, so a consumer can read a page's +// labels off a publish without a second call. +func Actions(add, remove, unchanged []string) []Action { + out := make([]Action, 0, len(add)+len(remove)+len(unchanged)) + for _, n := range add { + out = append(out, Action{Name: n, Action: ActionAdded}) + } + for _, n := range remove { + out = append(out, Action{Name: n, Action: ActionRemoved}) + } + for _, n := range unchanged { + out = append(out, Action{Name: n, Action: ActionUnchanged}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out +} + +// Read returns a page's labels, unfiltered. +func Read(c *client.ConfluenceClient, pageID string) ([]client.Label, error) { + return c.ListLabels(pageID) +} + +// Apply asserts s over a page's managed labels, returning one Action per label +// in the declared set plus one per label removed. +// +// A set that is already right makes no write: additions go out as one batched +// POST and each removal is its own DELETE, both skipped when there is nothing +// to do. Adding before removing is deliberate -- if the run dies between the +// two, the page is left over-labeled rather than under-labeled, and the next +// run converges either way. +// +// Calling this with an undeclared Set is a caller bug rather than a no-op with +// a plausible reading: it would mean "remove every label" for a file that said +// nothing about labels. +func Apply(c *client.ConfluenceClient, pageID string, s Set) ([]Action, error) { + if !s.Declared { + return nil, fmt.Errorf("internal: labels.Apply called for a file that declares none") + } + live, err := c.ListLabels(pageID) + if err != nil { + return nil, err + } + add, remove, unchanged := Diff(s.Names, Global(live)) + if err := c.AddLabels(pageID, add); err != nil { + return nil, err + } + for _, name := range remove { + if err := c.RemoveLabel(pageID, name); err != nil { + return nil, err + } + } + return Actions(add, remove, unchanged), nil +} + +// Plan is Apply's dry run: the same Actions, with nothing written. +func Plan(c *client.ConfluenceClient, pageID string, s Set) ([]Action, error) { + if !s.Declared { + return nil, fmt.Errorf("internal: labels.Plan called for a file that declares none") + } + live, err := c.ListLabels(pageID) + if err != nil { + return nil, err + } + return Actions(Diff(s.Names, Global(live))), nil +} diff --git a/internal/labels/labels_test.go b/internal/labels/labels_test.go new file mode 100644 index 0000000..b2481a5 --- /dev/null +++ b/internal/labels/labels_test.go @@ -0,0 +1,294 @@ +package labels_test + +import ( + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/labels" +) + +func eq(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// TestValidateRefusesTheSeparatorClass is the test this package exists for. +// Confluence splits a label name on a space or a comma, with a 200 and no +// warning, so an unvalidated "Runbook Two" publishes as two labels that read +// back as neither and are re-added on every run forever. Two labels in the SRE +// space are that bug having already happened to a person. +func TestValidateRefusesTheSeparatorClass(t *testing.T) { + for _, name := range []string{"Runbook Two", "a,b", "trail ", " lead", "a\tb"} { + t.Run(name, func(t *testing.T) { + lowered, _ := labels.Normalize(name) + if err := labels.Validate(lowered); err == nil { + t.Errorf("Validate(%q) = nil, want a refusal", lowered) + } + }) + } +} + +// TestValidateMirrorsTheServersRejectSet walks every character Confluence's own +// 400 body names, so the set cannot drift from what was measured. +func TestValidateMirrorsTheServersRejectSet(t *testing.T) { + for _, r := range labels.RejectChars { + name := "a" + string(r) + "b" + if err := labels.Validate(name); err == nil { + t.Errorf("Validate(%q) = nil, want a refusal for %q", name, string(r)) + } + } +} + +// TestValidateAcceptsRealLabels pins the decision to mirror the reject set +// rather than invent an allowlist: every one of these is a real shape, and +// ci/cd is a real label in the SRE space. +func TestValidateAcceptsRealLabels(t *testing.T) { + for _, name := range []string{ + "runbook", "ci/cd", "dataops_reports", "héllo-wörld", "v1", "2026", + "a+b", `a"b`, "a'b", "🎉", + } { + if err := labels.Validate(name); err != nil { + t.Errorf("Validate(%q) = %v, want nil", name, err) + } + } +} + +// TestValidateCountsUTF16Units pins the three points measured against the +// server. A byte-based check fails the first case and a rune-based one passes +// the third, so this is what keeps the units right in both directions. +func TestValidateCountsUTF16Units(t *testing.T) { + tests := []struct { + name string + label string + wantErr bool + }{ + {"255 accented runes is 255 units", strings.Repeat("é", 255), false}, + {"256 accented runes is 256 units", strings.Repeat("é", 256), true}, + {"128 emoji is 256 units", strings.Repeat("🎉", 128), true}, + {"127 emoji is 254 units", strings.Repeat("🎉", 127), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := labels.Validate(tt.label) + if (err != nil) != tt.wantErr { + t.Errorf("Validate(%d runes) = %v, wantErr %v", + len([]rune(tt.label)), err, tt.wantErr) + } + }) + } +} + +// TestValidateQuotesTheRejectSet: half the set is surprising -- a "." means a +// version-shaped label is impossible -- and an author should not have to bisect +// their own label to discover it. +func TestValidateQuotesTheRejectSet(t *testing.T) { + err := labels.Validate("v1.2") + if err == nil { + t.Fatal("Validate(v1.2) = nil, want a refusal") + } + for _, want := range []string{"space", "."} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + +func TestDeclaredNormalizesAndWarns(t *testing.T) { + set, err := labels.Declared(map[string][]string{"labels": {"Runbook", "HOWTO"}}, nil) + if err != nil { + t.Fatal(err) + } + if !eq(set.Names, []string{"howto", "runbook"}) { + t.Errorf("Names = %q, want [howto runbook]", set.Names) + } + if len(set.Warnings) != 2 { + t.Errorf("Warnings = %q, want one per label changed", set.Warnings) + } + if !strings.Contains(strings.Join(set.Warnings, " "), "Runbook") { + t.Errorf("warnings do not name the label as written: %q", set.Warnings) + } +} + +// TestDeclaredSortsAndDedupes: the set is what matters to Confluence, and +// neither label GET returns a useful order, so a generated list is sorted or it +// is unstable across runs. +func TestDeclaredSortsAndDedupes(t *testing.T) { + set, err := labels.Declared(map[string][]string{ + "labels": {"runbook", "ci/cd", "runbook", "Ci/CD"}, + }, nil) + if err != nil { + t.Fatal(err) + } + if !eq(set.Names, []string{"ci/cd", "runbook"}) { + t.Errorf("Names = %q, want [ci/cd runbook]", set.Names) + } +} + +// TestDeclaredSeparatesAbsentFromEmpty pins the asymmetry the whole field rests +// on. Absent means the live labels are untouched; empty means remove them all. +// Collapsing the two either strips a hand-labeled page on a run that never +// mentioned labels, or makes "remove them all" impossible to say. +func TestDeclaredSeparatesAbsentFromEmpty(t *testing.T) { + absent, err := labels.Declared(map[string][]string{}, map[string]string{"title": "T"}) + if err != nil { + t.Fatal(err) + } + if absent.Declared { + t.Error("Declared = true for a file with no labels key") + } + + empty, err := labels.Declared(map[string][]string{"labels": {}}, nil) + if err != nil { + t.Fatal(err) + } + if !empty.Declared { + t.Error("Declared = false for labels: []") + } + if len(empty.Names) != 0 { + t.Errorf("Names = %q, want empty", empty.Names) + } +} + +// TestDeclaredRefusesAScalar: the field is destructive, so "labels:" with +// nothing after it must not be read as "remove every label" on a file where an +// author typed a key and stopped. +// +// Driven through frontmatter.Parse rather than a hand-built map, because the +// null spellings are the cases worth proving and the parser is what flattens +// them: "~" and "null" both arrive here as "", so a test that passed "" three +// times would prove one case and claim three. +func TestDeclaredRefusesAScalar(t *testing.T) { + tests := []struct{ name, block, wantSubstr string }{ + {"a single name", "labels: runbook", "must be a list"}, + {"no value", "labels:", "no value"}, + {"tilde null", "labels: ~", "no value"}, + {"literal null", "labels: null", "no value"}, + {"blank string", `labels: ""`, "no value"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mf, err := frontmatter.Parse("doc.md", "---\n"+tt.block+"\n---\nbody\n") + if err != nil { + t.Fatalf("Parse(%q) = %v", tt.block, err) + } + _, err = labels.Declared(mf.Lists, mf.Frontmatter) + if err == nil { + t.Fatalf("Declared(%q) = nil error, want one", tt.block) + } + if !strings.Contains(err.Error(), tt.wantSubstr) { + t.Errorf("err = %q, want it to mention %q", err, tt.wantSubstr) + } + }) + } +} + +// TestDeclaredReadsBothYAMLStyles closes the loop with internal/frontmatter: +// this package never sees the spelling, so a block list and a flow list must +// reach it identically. +func TestDeclaredReadsBothYAMLStyles(t *testing.T) { + for _, block := range []string{ + "labels: [runbook, ci/cd]", + "labels:\n - runbook\n - ci/cd", + } { + mf, err := frontmatter.Parse("doc.md", "---\n"+block+"\n---\nbody\n") + if err != nil { + t.Fatalf("Parse(%q) = %v", block, err) + } + set, err := labels.Declared(mf.Lists, mf.Frontmatter) + if err != nil { + t.Fatalf("Declared(%q) = %v", block, err) + } + if !set.Declared || !eq(set.Names, []string{"ci/cd", "runbook"}) { + t.Errorf("%q gave Declared=%v Names=%q", block, set.Declared, set.Names) + } + } +} + +func TestDeclaredRefusesAnInvalidLabel(t *testing.T) { + _, err := labels.Declared(map[string][]string{"labels": {"ok", "Runbook Two"}}, nil) + if err == nil { + t.Fatal("Declared = nil error, want the invalid label reported") + } + if !strings.Contains(err.Error(), "runbook two") { + t.Errorf("err = %q, want it to name the label", err) + } +} + +// TestGlobalAndUnmanagedSplitByPrefix pins that markfluence manages one +// namespace. A my: label is personal and a team: one belongs to a space's own +// vocabulary; neither has a frontmatter spelling, so removing one would delete +// data the file could not have expressed. +func TestGlobalAndUnmanagedSplitByPrefix(t *testing.T) { + live := []client.Label{ + {Name: "runbook", Prefix: "global"}, + {Name: "mine", Prefix: "my"}, + {Name: "eng", Prefix: "team"}, + {Name: "ci/cd", Prefix: "global"}, + } + if got := labels.Global(live); !eq(got, []string{"ci/cd", "runbook"}) { + t.Errorf("Global = %q, want [ci/cd runbook]", got) + } + if got := labels.Unmanaged(live); !eq(got, []string{"my:mine", "team:eng"}) { + t.Errorf("Unmanaged = %q, want [my:mine team:eng]", got) + } +} + +func TestDiffIsSetArithmetic(t *testing.T) { + add, remove, unchanged := labels.Diff( + []string{"howto", "runbook"}, + []string{"runbook", "stale"}, + ) + if !eq(add, []string{"howto"}) { + t.Errorf("add = %q, want [howto]", add) + } + if !eq(remove, []string{"stale"}) { + t.Errorf("remove = %q, want [stale]", remove) + } + if !eq(unchanged, []string{"runbook"}) { + t.Errorf("unchanged = %q, want [runbook]", unchanged) + } +} + +// TestDiffIgnoresOrderAndDuplicates: a file whose list is merely reordered +// needs no write at all, which is what lets fix leave an author's ordering and +// any duplicate alone. +func TestDiffIgnoresOrderAndDuplicates(t *testing.T) { + add, remove, unchanged := labels.Diff( + []string{"runbook", "howto", "runbook"}, + []string{"howto", "runbook"}, + ) + if len(add) != 0 || len(remove) != 0 { + t.Errorf("add = %q remove = %q, want both empty", add, remove) + } + if !eq(unchanged, []string{"howto", "runbook"}) { + t.Errorf("unchanged = %q", unchanged) + } +} + +// TestActionsReportTheWholeSet: the full declared set, not just the changes, so +// a consumer can read a page's labels off a publish without a second call. +func TestActionsReportTheWholeSet(t *testing.T) { + got := labels.Actions([]string{"howto"}, []string{"stale"}, []string{"runbook"}) + if len(got) != 3 { + t.Fatalf("len = %d, want 3", len(got)) + } + want := map[string]string{"howto": "added", "stale": "removed", "runbook": "unchanged"} + for _, a := range got { + if want[a.Name] != a.Action { + t.Errorf("%s = %s, want %s", a.Name, a.Action, want[a.Name]) + } + } + // Sorted by name, since nothing upstream supplies a meaningful order. + if got[0].Name != "howto" || got[1].Name != "runbook" || got[2].Name != "stale" { + t.Errorf("order = %v, want sorted by name", got) + } +} From 9b405f45d694f6d31059cef1a8fb9dff7b55b7e1 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:54:52 -0400 Subject: [PATCH 06/16] feat(check): validate labels offline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An invalid label is the same class as an invalid page_width -- a guaranteed publish defect visible with no network -- and worse in one respect, which is why check is the command that should catch it. A page_width Confluence refuses fails loudly. A label name holding a space *succeeds*: Confluence splits it, stores the pieces, and returns 200, so the file now declares a label that reads back as neither piece and is re-added on every run with no spelling that removes it. There is no later opportunity to catch that one. So an invalid label is status: failed with code: VALIDATION, beside the page_width check, and the case repair is a warning carrying both spellings so an author can make the file say what will actually happen. Also fixes the message to quote the label as written. Validation ran after lowercasing, so a file saying "Runbook Two" was told about `label "runbook two"` -- a string that is not in their file. It now validates the raw name first for the message, and re-checks the normalized one because case folding can change UTF-16 length ("İ" lowercases to two code points). Label warnings lead the warnings list, since they are a property of the frontmatter and hold whatever the converter went on to find in the body. --- cmd/check/check.go | 15 ++++- cmd/check/check_test.go | 102 +++++++++++++++++++++++++++++++++ internal/labels/labels.go | 11 ++++ internal/labels/labels_test.go | 8 ++- 4 files changed, 133 insertions(+), 3 deletions(-) diff --git a/cmd/check/check.go b/cmd/check/check.go index d8cd355..ed297ab 100644 --- a/cmd/check/check.go +++ b/cmd/check/check.go @@ -15,6 +15,7 @@ import ( "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" @@ -123,6 +124,16 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache if _, err := pagewidth.Declared(mf.Frontmatter); err != nil { return r.fail(err, jsonout.CodeValidation) } + // An invalid label is a guaranteed publish defect that needs no network to + // see, the same class as an invalid page_width -- and worse in one way: a + // name Confluence splits on a space publishes successfully, as the wrong + // labels, and then cannot be removed by any spelling of the file (see + // docs/confluence/labels.md). Catching it offline is the cheapest place it + // can be caught. + labelSet, err := labels.Declared(mf.Lists, mf.Frontmatter) + if err != nil { + return r.fail(err, jsonout.CodeValidation) + } if pageID := mf.PageID(); pageID != "" && !pageref.IsDigits(pageID) { return r.fail(errors.New(pageref.NotNumericMessage(pageID)), jsonout.CodeValidation) } @@ -179,7 +190,9 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache return r.fail(err, jsonout.CodeConvert) } r.broken = append(frontmatterBroken, page.Broken...) - r.warnings = page.Warnings + // Label warnings lead: they are a property of the frontmatter, so they hold + // whatever the converter went on to find in the body. + r.warnings = append(labelSet.Warnings, page.Warnings...) if showHTML { r.debugHTML = page.HTML r.debugAttachments = page.Attachments diff --git a/cmd/check/check_test.go b/cmd/check/check_test.go index c7abb8d..be039b2 100644 --- a/cmd/check/check_test.go +++ b/cmd/check/check_test.go @@ -392,3 +392,105 @@ func TestRunEmptyTitleReportedEvenWhenConversionFails(t *testing.T) { t.Errorf("output = %q, want the empty-title message alongside the collision", out) } } + +// --- labels ------------------------------------------------------------------- + +// TestRunInvalidLabelIsFailed pins the worst label defect as a failure rather +// than a warning. "Runbook Two" publishes *successfully* as two labels that +// read back as neither, so no later run can remove them -- catching it offline +// is the only cheap place to catch it at all. +func TestRunInvalidLabelIsFailed(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "bad.md"), + "---\ntitle: T\nlabels: [Runbook Two]\n---\n# T\n\nBody.\n") + + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "bad.md")}) + }) + if err == nil { + t.Fatal("run = nil error, want a failure for an invalid label") + } + if !strings.Contains(out, "separator") { + t.Errorf("output = %q, want it to explain that a space is a separator", out) + } +} + +// TestRunLabelCaseIsAWarning: lowercasing is the one repair, so the file still +// publishes -- but silently rewriting an author's label without telling them is +// how a file stays permanently out of step with its page. +func TestRunLabelCaseIsAWarning(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "case.md"), + "---\ntitle: T\nlabels: [Runbook]\n---\n# T\n\nBody.\n") + + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "case.md")}) + }) + if err != nil { + t.Fatalf("run = %v, want a warning rather than a failure", err) + } + if !strings.Contains(out, "Runbook") || !strings.Contains(out, "runbook") { + t.Errorf("output = %q, want both spellings named", out) + } +} + +// TestRunValidLabelsAreClean covers the shapes that must *not* be refused: a +// slash (ci/cd is a real label in the SRE space), an underscore, and non-ASCII. +// Mirroring the server's reject set rather than an allowlist is what makes +// these pass. +func TestRunValidLabelsAreClean(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "ok.md"), + "---\ntitle: T\nlabels: [ci/cd, dataops_reports, héllo-wörld]\n---\n# T\n\nBody.\n") + + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "ok.md")}) + }) + if err != nil { + t.Fatalf("run = %v, want clean", err) + } + if !strings.Contains(out, "clean") { + t.Errorf("output = %q, want a clean line", out) + } +} + +// TestRunBothLabelStylesCheckTheSame closes the loop through the real file +// reader: check is the command an author runs before publishing, so it must +// accept the spelling they chose. +func TestRunBothLabelStylesCheckTheSame(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "flow.md"), + "---\ntitle: T\nlabels: [Runbook]\n---\n# T\n\nBody.\n") + write(t, filepath.Join(dir, "block.md"), + "---\ntitle: T\nlabels:\n - Runbook\n---\n# T\n\nBody.\n") + + for _, name := range []string{"flow.md", "block.md"} { + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, name)}) + }) + if err != nil { + t.Fatalf("%s: run = %v", name, err) + } + if !strings.Contains(out, "not lowercase") { + t.Errorf("%s: output = %q, want the case warning", name, out) + } + } +} + +// TestRunScalarLabelsIsFailed: the field removes every label not listed, so +// "labels:" with nothing after it must not be read as "strip this page". +func TestRunScalarLabelsIsFailed(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "scalar.md"), + "---\ntitle: T\nlabels: runbook\n---\n# T\n\nBody.\n") + + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "scalar.md")}) + }) + if err == nil { + t.Fatal("run = nil error, want a failure for a scalar labels field") + } + if !strings.Contains(out, "must be a list") { + t.Errorf("output = %q, want it to name the list form", out) + } +} diff --git a/internal/labels/labels.go b/internal/labels/labels.go index c05be2e..fe5cea8 100644 --- a/internal/labels/labels.go +++ b/internal/labels/labels.go @@ -132,7 +132,18 @@ func Declared(lists map[string][]string, frontmatter map[string]string) (Set, er seen := make(map[string]bool, len(declared)) set := Set{Declared: true, Names: make([]string, 0, len(declared))} for _, raw := range declared { + // Validated as written *first*, so the message quotes the label the + // author can actually find in their file. Lowercasing before validating + // reported `label "runbook two"` for a file that says "Runbook Two", + // which sends them searching for a string that is not there. + if err := Validate(raw); err != nil { + return Set{}, err + } name, changed := Normalize(raw) + // Re-checked after normalizing because case folding can change length + // in UTF-16 units: "İ" (U+0130) lowercases to two code points. Nothing + // realistic reaches this, and a label that passed as written and fails + // lowercased would otherwise be refused by the server instead. if err := Validate(name); err != nil { return Set{}, err } diff --git a/internal/labels/labels_test.go b/internal/labels/labels_test.go index b2481a5..ea412bd 100644 --- a/internal/labels/labels_test.go +++ b/internal/labels/labels_test.go @@ -213,13 +213,17 @@ func TestDeclaredReadsBothYAMLStyles(t *testing.T) { } } +// TestDeclaredRefusesAnInvalidLabel also pins *which spelling* the message +// quotes: the one in the file. Validating the lowercased name first reported +// `label "runbook two"` for a file that says "Runbook Two", sending an author +// to search for a string their file does not contain. func TestDeclaredRefusesAnInvalidLabel(t *testing.T) { _, err := labels.Declared(map[string][]string{"labels": {"ok", "Runbook Two"}}, nil) if err == nil { t.Fatal("Declared = nil error, want the invalid label reported") } - if !strings.Contains(err.Error(), "runbook two") { - t.Errorf("err = %q, want it to name the label", err) + if !strings.Contains(err.Error(), "Runbook Two") { + t.Errorf("err = %q, want it to name the label as written", err) } } From 839f08c60b8d3158265dab0213691483a3341324 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 20:57:41 -0400 Subject: [PATCH 07/16] feat(update): assert the declared label set Declared means exact: a label on the page that the file does not list is removed. Absent means untouched, which mirrors page_width's existing asymmetry and is what keeps a hand-labeled page from being stripped by a run that never mentioned labels. That last part is pinned as "makes no label request at all", not "makes no write". No request means there is no path by which a run that said nothing about labels can decide to change them, which makes it a property rather than an implementation detail. Ordering, both halves deliberate. Validation runs before any request and is fatal, because the defect it catches is not recoverable afterwards: a name holding a space publishes *successfully* as several labels that read back as none of what the file says. Application runs last and is non-fatal, matching pagewidth.Apply -- the body is published by then, so failing the result would report that the publish did not happen. A failed application leaves labels null rather than claiming a set that is not there. --json gets `labels`: the full declared set every run, not just the changes, so a consumer can read a page's labels off a publish without a second call. It is `*[]Label` rather than a slice because an empty declared set and an absent key are different answers -- "[]" means the labels were removed, null means they were never touched -- and a nil slice cannot say which. In a dry run the same field carries the actions that would be taken, read-only. Unmanaged labels survive, tested: a my: or team: label has no frontmatter spelling, so a set that cannot mention it must not remove it. --- cmd/update/json.go | 25 ++++ cmd/update/json_test.go | 1 + cmd/update/update.go | 65 +++++++++++ cmd/update/update_test.go | 227 +++++++++++++++++++++++++++++++++++++ internal/jsonout/types.go | 22 ++++ schema/json-output/v1.json | 38 ++++++- 6 files changed, 377 insertions(+), 1 deletion(-) diff --git a/cmd/update/json.go b/cmd/update/json.go index 7fc1006..9d4787a 100644 --- a/cmd/update/json.go +++ b/cmd/update/json.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/ui" ) @@ -30,6 +31,11 @@ type updateResult struct { versionNew int width *jsonout.PageWidth // set only when a width was asserted this run widthSet bool // a "page width:" line should show (human) + // labels is nil when the file declares no labels key, which is the + // precedent page_width already sets for "not asserted this run" -- and it + // is load-bearing rather than cosmetic here, since an empty set is a + // meaningful declaration that means "remove them all". + labels []jsonout.Label attachments []jsonout.Attachment broken []string warnings []string @@ -73,6 +79,11 @@ func (r *updateResult) renderHuman() { if r.widthSet && r.width != nil { ui.Info(prefix + " page width: " + r.width.Value) } + for _, l := range r.labels { + if l.Action != labels.ActionUnchanged { + ui.Info(fmt.Sprintf("%s label %s: %s", prefix, l.Action, l.Name)) + } + } ui.Success(fmt.Sprintf("%s Published v%d: %s", prefix, r.versionNew, r.url)) } @@ -88,6 +99,7 @@ type jsonUpdateResult struct { URL *string `json:"url"` Version *jsonUpdateVersion `json:"version"` PageWidth *jsonout.PageWidth `json:"page_width"` + Labels *[]jsonout.Label `json:"labels"` Attachments []jsonout.Attachment `json:"attachments"` Warnings []string `json:"warnings"` Broken []string `json:"broken"` @@ -111,6 +123,7 @@ func (r *updateResult) jsonResult() jsonUpdateResult { Space: strOrNil(r.space), URL: strOrNil(r.url), PageWidth: r.width, + Labels: labelsOrNil(r.labels), Attachments: nonNilAttachments(r.attachments), Warnings: nonNilStrings(r.warnings), Broken: nonNilStrings(r.broken), @@ -127,6 +140,18 @@ func (r *updateResult) jsonResult() jsonUpdateResult { return res } +// labelsOrNil renders the labels field: an array when the file declared the +// key, null when it did not. A pointer to a slice rather than a slice, because +// an empty declared set and an absent key are different answers -- "[]" means +// the page's labels were removed, null means they were never touched -- and a +// nil slice cannot say which. +func labelsOrNil(l []jsonout.Label) *[]jsonout.Label { + if l == nil { + return nil + } + return &l +} + // summarize builds update's batch summary. func summarize(results []*updateResult) map[string]int { s := map[string]int{"total": len(results), "succeeded": 0, "failed": 0, "skipped": 0} diff --git a/cmd/update/json_test.go b/cmd/update/json_test.go index 4b7d5b9..2907472 100644 --- a/cmd/update/json_test.go +++ b/cmd/update/json_test.go @@ -67,6 +67,7 @@ func TestJSONResultPublished(t *testing.T) { "value": "max", "default": false }, + "labels": null, "attachments": [ { "action": "updated", diff --git a/cmd/update/update.go b/cmd/update/update.go index 26a0a7f..9e22b67 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -16,6 +16,7 @@ import ( "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" @@ -167,6 +168,16 @@ func processFile( if err != nil { return r.fail(err, jsonout.CodeValidation) } + // Before any request, and fatal: an invalid label is a local defect, and + // the one it exists to catch is not recoverable afterwards. A name holding + // a space publishes *successfully* as several labels that read back as none + // of what the file says, so there is no later run that can clean it up -- + // see docs/confluence/labels.md. + labelSet, err := labels.Declared(mf.Lists, mf.Frontmatter) + if err != nil { + return r.fail(err, jsonout.CodeValidation) + } + r.warnings = append(r.warnings, labelSet.Warnings...) // GetPageOrNil, not GetPage: a 404 here means the page_id is wrong, which is // worth saying in words. Every other transport failure still reports itself. @@ -235,6 +246,7 @@ func processFile( } r.versionNew = next r.previewWidth(c, pageID, width, applyWidth) + r.previewLabels(c, pageID, labelSet) r.ok = true r.status = statusPublished return r @@ -275,6 +287,12 @@ func processFile( } } + // Labels last, and non-fatal for the same reason the width is: the page is + // published by the time this runs, so failing the result would report that + // the publish did not happen. A declared-but-unapplied set leaves labels + // null rather than claiming a set that is not there. + r.applyLabels(c, pageID, labelSet) + r.ok = true r.status = statusPublished return r @@ -343,3 +361,50 @@ func resolveWidth(cliPageWidth string, mf *frontmatter.MarkdownFile) (pagewidth. } return "", false, nil } + +// applyLabels asserts the declared label set, recording the per-label actions. +// +// A file that declares no labels key makes no request at all -- not merely no +// write. That is what makes "absent means untouched" a property rather than an +// implementation detail, and it is why the check is here rather than inside +// labels.Apply, which refuses an undeclared set outright. +// +// A failure is a warning on a successful result, matching pagewidth.Apply: the +// body is already published, and reporting the file as failed would say +// otherwise. The labels field stays nil so nothing claims a set that was not +// asserted. +func (r *updateResult) applyLabels(c *client.ConfluenceClient, pageID string, s labels.Set) { + if !s.Declared { + return + } + actions, err := labels.Apply(c, pageID, s) + if err != nil { + r.warnings = append(r.warnings, "could not set labels: "+err.Error()) + return + } + r.labels = toJSONLabels(actions) +} + +// previewLabels reports the label changes a dry run would make, read-only. A +// read failure is a warning, not fatal -- mirroring previewWidth. +func (r *updateResult) previewLabels(c *client.ConfluenceClient, pageID string, s labels.Set) { + if !s.Declared { + return + } + actions, err := labels.Plan(c, pageID, s) + if err != nil { + r.warnings = append(r.warnings, "could not read labels: "+err.Error()) + return + } + r.labels = toJSONLabels(actions) +} + +// toJSONLabels converts label actions to the reported shape, always non-nil so +// a declared-but-empty set renders as [] rather than null. +func toJSONLabels(actions []labels.Action) []jsonout.Label { + out := make([]jsonout.Label, 0, len(actions)) + for _, a := range actions { + out = append(out, jsonout.Label{Action: a.Action, Name: a.Name}) + } + return out +} diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index cb60f92..6137ae2 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -382,3 +382,230 @@ func TestProcessFileKeepsLiveTitleWhenAbsent(t *testing.T) { t.Errorf("title = %q, want the live page's title", r.title) } } + +// --- labels ------------------------------------------------------------------- + +// labelServer answers a publish plus whatever label traffic the run makes, +// recording every label path it sees so a test can assert on requests that +// were *not* made. +func labelServer(t *testing.T, live string, paths *[]string) *client.ConfluenceClient { + t.Helper() + return clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "label") { + *paths = append(*paths, r.Method+" "+r.URL.Path+"?"+r.URL.RawQuery) + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(live)) + return + } + w.WriteHeader(http.StatusNoContent) + return + } + switch r.Method { + case http.MethodGet: + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2020-01-01T00:00:00Z"))) + case http.MethodPut: + _, _ = w.Write([]byte(pageWithVersion("1", 4, "2026-01-01T00:00:00Z"))) + default: + t.Errorf("unexpected method: %s %s", r.Method, r.URL.Path) + } + }) +} + +// TestProcessFileAbsentLabelsMakesNoRequest is the test that makes "absent +// means untouched" a property rather than an implementation detail. Not merely +// "no write": no *request*, so there is no path by which a run that never +// mentioned labels can decide to change them. +func TestProcessFileAbsentLabelsMakesNoRequest(t *testing.T) { + var paths []string + c := labelServer(t, `{"results":[]}`, &paths) + path := writeUpdateFixture(t, "---\npage_id: 1\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok { + t.Fatalf("result = %+v, want ok", r) + } + if len(paths) != 0 { + t.Errorf("label requests = %v, want none for a file with no labels key", paths) + } + if r.labels != nil { + t.Errorf("r.labels = %v, want nil so --json reports null", r.labels) + } +} + +// TestProcessFileAssertsTheDeclaredSet: declared means exact, so a label on the +// page that the file does not list is removed. +func TestProcessFileAssertsTheDeclaredSet(t *testing.T) { + var paths []string + live := `{"results":[ + {"id":"1","name":"runbook","prefix":"global"}, + {"id":"2","name":"stale","prefix":"global"} + ]}` + c := labelServer(t, live, &paths) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: [runbook, howto]\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok || r.status != statusPublished { + t.Fatalf("result = %+v, want ok/published", r) + } + got := map[string]string{} + for _, l := range r.labels { + got[l.Name] = l.Action + } + want := map[string]string{"howto": "added", "stale": "removed", "runbook": "unchanged"} + for name, action := range want { + if got[name] != action { + t.Errorf("labels[%q] = %q, want %q (all: %v)", name, got[name], action, r.labels) + } + } + // The removal must go out as ?name=, never a path segment. + var sawRemoval bool + for _, p := range paths { + if strings.HasPrefix(p, http.MethodDelete) { + sawRemoval = true + if !strings.Contains(p, "?name=stale") { + t.Errorf("removal request = %q, want the ?name= form", p) + } + } + } + if !sawRemoval { + t.Error("want the surplus label removed") + } +} + +// TestProcessFileUnmanagedLabelsSurvive: a my: or team: label has no +// frontmatter spelling, so asserting a set that cannot mention it must not +// remove it. +func TestProcessFileUnmanagedLabelsSurvive(t *testing.T) { + var paths []string + live := `{"results":[ + {"id":"1","name":"mine","prefix":"my"}, + {"id":"2","name":"eng","prefix":"team"} + ]}` + c := labelServer(t, live, &paths) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: [runbook]\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok { + t.Fatalf("result = %+v, want ok", r) + } + for _, p := range paths { + if strings.HasPrefix(p, http.MethodDelete) { + t.Errorf("removal request %q, want no unmanaged label removed", p) + } + } +} + +// TestProcessFileEmptyLabelsRemovesThemAll pins the other half of the +// absent/empty distinction: "labels: []" is a declaration, not a no-op. +func TestProcessFileEmptyLabelsRemovesThemAll(t *testing.T) { + var paths []string + c := labelServer(t, `{"results":[{"id":"1","name":"stale","prefix":"global"}]}`, &paths) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: []\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok { + t.Fatalf("result = %+v, want ok", r) + } + if r.labels == nil { + t.Fatal("r.labels = nil, want [] so --json distinguishes it from an absent key") + } + if len(r.labels) != 1 || r.labels[0].Action != "removed" { + t.Errorf("labels = %v, want stale removed", r.labels) + } +} + +// TestProcessFileInvalidLabelFailsBeforeAnyWrite is the ordering that matters: +// a name Confluence would split publishes successfully and cannot then be +// cleaned up, so the run must stop before the body goes out. +func TestProcessFileInvalidLabelFailsBeforeAnyWrite(t *testing.T) { + var sawWrite bool + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + sawWrite = true + } + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2020-01-01T00:00:00Z"))) + }) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: [Runbook Two]\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if r.ok { + t.Fatal("result ok, want a validation failure") + } + if r.code != jsonout.CodeValidation { + t.Errorf("code = %q, want VALIDATION", r.code) + } + if sawWrite { + t.Error("a write went out for a file with an invalid label") + } +} + +// TestProcessFileLabelFailureIsAWarning: the body is published by the time +// labels are applied, so a label failure must not report the publish as failed. +func TestProcessFileLabelFailureIsAWarning(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "label") { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + return + } + switch r.Method { + case http.MethodGet: + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2020-01-01T00:00:00Z"))) + default: + _, _ = w.Write([]byte(pageWithVersion("1", 4, "2026-01-01T00:00:00Z"))) + } + }) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: [runbook]\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok || r.status != statusPublished { + t.Fatalf("result = %+v, want the publish still reported as ok", r) + } + if r.labels != nil { + t.Errorf("r.labels = %v, want nil rather than a set that was not asserted", r.labels) + } + if !strings.Contains(strings.Join(r.warnings, " "), "could not set labels") { + t.Errorf("warnings = %q, want the label failure reported", r.warnings) + } +} + +// TestProcessFileLabelCaseWarns: lowercasing is a repair, so the run succeeds, +// but silently rewriting an author's label is how a file stays out of step with +// its page forever. +func TestProcessFileLabelCaseWarns(t *testing.T) { + var paths []string + c := labelServer(t, `{"results":[]}`, &paths) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: [Runbook]\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok { + t.Fatalf("result = %+v, want ok", r) + } + if !strings.Contains(strings.Join(r.warnings, " "), "not lowercase") { + t.Errorf("warnings = %q, want the case warning", r.warnings) + } + if len(r.labels) != 1 || r.labels[0].Name != "runbook" { + t.Errorf("labels = %v, want the lowercased name", r.labels) + } +} + +// TestProcessFileSkippedFileSkipsLabels: a file the mtime check skipped is +// skipped entirely, labels included. +func TestProcessFileSkippedFileSkipsLabels(t *testing.T) { + var paths []string + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "label") { + paths = append(paths, r.URL.Path) + } + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2999-01-01T00:00:00Z"))) + }) + path := writeUpdateFixture(t, "---\npage_id: 1\nlabels: [runbook]\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if r.status != statusSkipped { + t.Fatalf("status = %q, want skipped", r.status) + } + if len(paths) != 0 { + t.Errorf("label requests = %v, want none for a skipped file", paths) + } +} diff --git a/internal/jsonout/types.go b/internal/jsonout/types.go index ca55963..50964f0 100644 --- a/internal/jsonout/types.go +++ b/internal/jsonout/types.go @@ -31,6 +31,28 @@ type Attachment struct { Filename string `json:"filename"` } +// Label is one label's outcome in an asserted set: what happened to it and its +// name, mirroring Attachment's {action, filename}. +// +// A result carries the *whole* declared set every run, not just the changes, so +// a consumer can read a page's labels off a publish without a second call. +type Label struct { + Action string `json:"action"` + Name string `json:"name"` +} + +// LabelInfo describes one label a page carries, for info: its name, its +// namespace, and whether markfluence manages it. +// +// Managed is reported rather than left to be derived, so a consumer does not +// have to know the prefix rule to reproduce the split -- and so the rule can +// only be wrong in one place. +type LabelInfo struct { + Name string `json:"name"` + Prefix string `json:"prefix"` + Managed bool `json:"managed"` +} + // AttachmentActionResult is one file's outcome from attachment-upload or // attachment-download: what happened, whether it succeeded, and (download only) // where it landed on disk. DestPath is always nil for attachment-upload, which diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index 937d011..becec6b 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -271,6 +271,41 @@ "filename": { "type": "string" } } }, + "labelAction": { + "description": "What happened to one label in an asserted set.", + "type": "object", + "additionalProperties": false, + "required": ["action", "name"], + "properties": { + "action": { "enum": ["added", "removed", "unchanged"] }, + "name": { "type": "string" } + } + }, + "labelActionsOrNull": { + "description": "The full label set asserted this run, or null when the file declares no labels key and the page's labels were left alone.", + "oneOf": [ + { "type": "array", "items": { "$ref": "#/$defs/labelAction" } }, + { "type": "null" } + ] + }, + "labelInfo": { + "description": "One label a page carries. managed is true for the global prefix, the only one markfluence writes or removes.", + "type": "object", + "additionalProperties": false, + "required": ["name", "prefix", "managed"], + "properties": { + "name": { "type": "string" }, + "prefix": { "type": "string" }, + "managed": { "type": "boolean" } + } + }, + "labelInfoListOrNull": { + "description": "Every label on a page, or null when the fetch failed. An empty array means the page genuinely has none.", + "oneOf": [ + { "type": "array", "items": { "$ref": "#/$defs/labelInfo" } }, + { "type": "null" } + ] + }, "singleOpFailure": { "description": "An operational failure for a single-target command (info/read): page not found, fetch error, etc.", "type": "object", @@ -346,7 +381,7 @@ "additionalProperties": false, "required": [ "ok", "status", "dry_run", "file", "page_id", "title", "space", "url", - "version", "page_width", "attachments", "warnings", "broken", "error", "code" + "version", "page_width", "labels", "attachments", "warnings", "broken", "error", "code" ], "properties": { "ok": { "type": "boolean" }, @@ -369,6 +404,7 @@ ] }, "page_width": { "$ref": "#/$defs/pageWidthOrNull" }, + "labels": { "$ref": "#/$defs/labelActionsOrNull" }, "attachments": { "type": "array", "items": { "$ref": "#/$defs/attachment" } }, "warnings": { "type": "array", "items": { "type": "string" } }, "broken": { "type": "array", "items": { "type": "string" } }, From a5562172ac06903e412cec6f5a762efd4bbd3108 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 21:00:45 -0400 Subject: [PATCH 08/16] feat(create): validate labels in preflight and apply them on publish #127's guarantee applied to labels: a defect in the file must not leave a created page behind. It matters more here than for most phase-1 checks, because the defect is not recoverable afterwards -- a label Confluence splits on a space publishes *successfully*, so a page created before the check would carry labels no later run can remove, and the author would have a page_id written into their file to undo by hand. Pinned by a test asserting no page is created, no label request is made, and no page_id is written back. Validated beside resolveWidth rather than beside the preflight conversion, which is where the plan put it. The width check is the closer precedent: both are offline reads of one frontmatter field, so neither spends a request to find out, and the page_id precedence the conversion had to be sequenced around doesn't apply to a local check. A bad label takes newFailure's fallback and so reports VALIDATION -- not CONVERT, since the converter never saw it, and not a request code, since no request was made. The validated set is carried on the record rather than re-read at publish, so the two phases cannot disagree about what was checked. Application is last and non-fatal, matching the width: the page exists and carries its content by then, so failing the result would report that it does not. The dry run previews labels without asking for the live set, unlike update's. There is no page yet, so every declared label is an add and there is nothing to remove -- looking it up would be a request against an id that does not exist. --- cmd/create/create.go | 43 ++++++++- cmd/create/json.go | 58 +++++++++--- cmd/create/json_test.go | 1 + cmd/create/run_test.go | 176 +++++++++++++++++++++++++++++++++++++ schema/json-output/v1.json | 3 +- 5 files changed, 266 insertions(+), 15 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 4b48603..ee6b661 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -31,6 +31,7 @@ import ( "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" @@ -122,6 +123,11 @@ type record struct { spaceID string parent parentInfo width pagewidth.Width + // labels is the validated label set the file declares. Validated in + // preflight so a name Confluence would mangle never reaches a created page, + // and carried rather than re-read so publish cannot disagree with what was + // checked. + labels labels.Set // root bounds this file's image/parent reads and is what its attachments' // names and recorded Source are relative to. Discovered from the file's own // directory, cached across the batch by internal/project.Cache. @@ -553,6 +559,12 @@ func publishOne(r record, res *createResult, pageID string, version int, c *clie } res.width = &jsonout.PageWidth{Value: string(r.width), Default: false} res.widthSet = true + // No request for the live set, unlike update's dry run: the page does + // not exist, so every declared label is an add and there is nothing to + // remove. Asking would be a request against an id that is not there. + if r.labels.Declared { + res.labels = toJSONLabels(labels.Actions(r.labels.Names, nil, nil)) + } res.ok = true res.status = statusCreated return res @@ -589,6 +601,18 @@ func publishOne(r record, res *createResult, pageID string, version int, c *clie } } + // Non-fatal, like the width and for the same reason: the page exists and + // carries its content by now, so failing the result would report that it + // does not. The names were validated in preflight, so anything that fails + // here is the server or the network rather than the file. + if r.labels.Declared { + if acts, err := labels.Apply(c, pageID, r.labels); err != nil { + res.warnings = append(res.warnings, "could not set labels: "+err.Error()) + } else { + res.labels = toJSONLabels(acts) + } + } + res.ok = true res.status = statusCreated return res @@ -624,6 +648,20 @@ func resolveFile( if err != nil { return record{}, err } + // Beside the width rather than beside the conversion at the end of this + // function, though both are there for #127's reason -- a defect in the file + // must not leave a created page behind. The width check is the closer + // precedent: both are offline reads of one frontmatter field, so neither + // spends a request to find out. The conversion is last only because it is + // expensive and because the page_id precedence below had to be preserved. + // + // Fatal here, and it has to be: a label Confluence splits on a space + // publishes *successfully*, so unlike almost anything else phase 1 rejects, + // there is no later run that can repair it. + labelSet, err := labels.Declared(mf.Lists, mf.Frontmatter) + if err != nil { + return record{}, err + } // Before the space, parent, and duplicate-title lookups: a page_id that is // already taken or already broken is the most specific thing wrong with the @@ -695,7 +733,10 @@ func resolveFile( return record{}, &convertFailure{err: err} } - return record{filename, abs, mf, title, spaceKey, spaceID, parent, width, root, index}, nil + return record{ + filename: filename, absPath: abs, mdfile: mf, title: title, spaceKey: spaceKey, + spaceID: spaceID, parent: parent, width: width, labels: labelSet, root: root, index: index, + }, nil } // resolveParent resolves a file's parent: reference. A ".md" reference is read diff --git a/cmd/create/json.go b/cmd/create/json.go index 6db93a7..c25d6d6 100644 --- a/cmd/create/json.go +++ b/cmd/create/json.go @@ -5,6 +5,7 @@ import ( "os" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/ui" ) @@ -19,19 +20,23 @@ const ( // createResult captures the outcome of creating one page. type createResult struct { - file string - ok bool - status string - dryRun bool - pageID string - title string - space string - parent *string - parentType *string - parentFile *string - url string - width *jsonout.PageWidth - widthSet bool + file string + ok bool + status string + dryRun bool + pageID string + title string + space string + parent *string + parentType *string + parentFile *string + url string + width *jsonout.PageWidth + widthSet bool + // labels is nil when the file declares no labels key -- the same "not + // asserted this run" convention page_width uses, and load-bearing here + // because an empty declared set is itself a declaration. + labels []jsonout.Label persisted bool attachments []jsonout.Attachment broken []string @@ -90,6 +95,11 @@ func (r *createResult) renderHuman() { if r.widthSet && r.width != nil { ui.Info(prefix + " page width: " + r.width.Value) } + for _, l := range r.labels { + if l.Action != labels.ActionUnchanged { + ui.Info(fmt.Sprintf("%s label %s: %s", prefix, l.Action, l.Name)) + } + } // A dry-run has created no page, so there is no id or URL to print; name the // title and space instead. Every other line above is identical to a real run. if r.dryRun { @@ -113,6 +123,7 @@ type jsonCreateResult struct { ParentFile *string `json:"parent_file"` URL *string `json:"url"` PageWidth *jsonout.PageWidth `json:"page_width"` + Labels *[]jsonout.Label `json:"labels"` Persisted bool `json:"persisted"` Attachments []jsonout.Attachment `json:"attachments"` Warnings []string `json:"warnings"` @@ -135,6 +146,7 @@ func (r *createResult) jsonResult() jsonCreateResult { ParentFile: r.parentFile, URL: nullableStr(r.url), PageWidth: r.width, + Labels: labelsOrNil(r.labels), Persisted: r.persisted, Attachments: nonNilAttachments(r.attachments), Warnings: nonNilStrings(r.warnings), @@ -280,3 +292,23 @@ func fatalFail(msg string, code jsonout.Code) error { } return ui.SilentExit(2) } + +// labelsOrNil renders the labels field: an array when the file declared the +// key, null when it did not. A pointer to a slice, because an empty declared +// set and an absent key are different answers and a nil slice cannot say which. +func labelsOrNil(l []jsonout.Label) *[]jsonout.Label { + if l == nil { + return nil + } + return &l +} + +// toJSONLabels converts label actions to the reported shape, always non-nil so +// a declared-but-empty set renders as [] rather than null. +func toJSONLabels(actions []labels.Action) []jsonout.Label { + out := make([]jsonout.Label, 0, len(actions)) + for _, a := range actions { + out = append(out, jsonout.Label{Action: a.Action, Name: a.Name}) + } + return out +} diff --git a/cmd/create/json_test.go b/cmd/create/json_test.go index 065f4b0..228c2cd 100644 --- a/cmd/create/json_test.go +++ b/cmd/create/json_test.go @@ -130,6 +130,7 @@ func TestJSONResultCreated(t *testing.T) { "value": "max", "default": false }, + "labels": null, "persisted": true, "attachments": [], "warnings": [], diff --git a/cmd/create/run_test.go b/cmd/create/run_test.go index 7a1f89e..731a74e 100644 --- a/cmd/create/run_test.go +++ b/cmd/create/run_test.go @@ -41,6 +41,10 @@ type fakeConfluence struct { // created under that title -- used to test a publish-phase failure after a // successful reserve. failUpdateForTitle string + // labelCalls records every label request, so a test can assert on one that + // was not made; labelsAdded records the names POSTed. + labelCalls []string + labelsAdded []string // rejectCredential makes every route answer the way the API answers a // revoked token: 404 with a title that names nothing. This is the shape // #133 is about -- it is not a missing page, and reporting it as one (or as @@ -101,6 +105,22 @@ func (f *fakeConfluence) handle(w http.ResponseWriter, r *http.Request) { case r.Method == http.MethodPut && strings.HasPrefix(r.URL.Path, "/wiki/api/v2/pages/"): f.updatePage(w, r) + case strings.Contains(r.URL.Path, "label"): + f.labelCalls = append(f.labelCalls, r.Method+" "+r.URL.Path+"?"+r.URL.RawQuery) + if r.Method == http.MethodGet { + // A freshly created page carries no labels. + _, _ = fmt.Fprint(w, `{"results":[]}`) + return + } + if r.Method == http.MethodPost { + var body []map[string]string + _ = json.NewDecoder(r.Body).Decode(&body) + for _, l := range body { + f.labelsAdded = append(f.labelsAdded, l["name"]) + } + } + w.WriteHeader(http.StatusOK) + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/properties"): _, _ = fmt.Fprint(w, `{"results":[]}`) @@ -801,3 +821,159 @@ func captureStdout(t *testing.T, fn func() error) (string, error) { } return string(out), runErr } + +// --- labels ------------------------------------------------------------------- + +// TestRunInvalidLabelCreatesNothing is #127's guarantee applied to labels, and +// the reason validating them in preflight rather than at publish matters. A +// label Confluence splits on a space publishes *successfully*, so a page +// created before the check would carry labels no later run can remove -- and +// the author would have a page_id written into their file to undo by hand. +func TestRunInvalidLabelCreatesNothing(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + bad := write(t, dir, "bad.md", "---\ntitle: Bad\nlabels: [Runbook Two]\n---\nbody\n") + + c, f := newFakeConfluence(t) + _, runErr := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{bad}) + }) + if runErr == nil { + t.Fatal("run should have failed for an invalid label") + } + if len(f.pages) != 0 { + t.Errorf("pages = %v, want none created", f.pages) + } + if len(f.labelCalls) != 0 { + t.Errorf("label requests = %v, want none", f.labelCalls) + } + raw, err := os.ReadFile(bad) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "page_id") { + t.Error("a page_id was written back for a file that never published") + } +} + +// TestRunInvalidLabelReportsVALIDATION: a bad label is a defect in the file, so +// it takes newFailure's fallback code rather than CONVERT (the converter never +// saw it) or a request code (no request was made). +func TestRunInvalidLabelReportsVALIDATION(t *testing.T) { + resetOpts(t) + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + dir := t.TempDir() + spaceOpt = "ENG" + bad := write(t, dir, "bad.md", "---\ntitle: Bad\nlabels: [a,b c]\n---\nbody\n") + + c, _ := newFakeConfluence(t) + out, runErr := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{bad}) + }) + if runErr == nil { + t.Fatal("run should have failed") + } + schematest.ValidateEnvelope(t, []byte(out)) + + var env struct { + Results []struct { + Code *string `json:"code"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("unmarshal %q: %v", out, err) + } + if len(env.Results) != 1 || env.Results[0].Code == nil { + t.Fatalf("results = %+v, want one coded failure", env.Results) + } + if got := *env.Results[0].Code; got != string(jsonout.CodeValidation) { + t.Errorf("code = %q, want VALIDATION", got) + } +} + +// TestRunAppliesDeclaredLabels: the happy path, plus the shape of the reported +// actions -- every declared label is an add on a page that did not exist. +func TestRunAppliesDeclaredLabels(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + path := write(t, dir, "ok.md", "---\ntitle: Ok\nlabels: [runbook, ci/cd]\n---\nbody\n") + + c, f := newFakeConfluence(t) + if _, err := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{path}) + }); err != nil { + t.Fatalf("run: %v", err) + } + want := map[string]bool{"runbook": true, "ci/cd": true} + for _, name := range f.labelsAdded { + delete(want, name) + } + if len(want) != 0 { + t.Errorf("labels added = %v, missing %v", f.labelsAdded, want) + } +} + +// TestRunAbsentLabelsMakesNoRequest: absent means untouched here too, and on a +// freshly created page that means no label traffic at all. +func TestRunAbsentLabelsMakesNoRequest(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + path := write(t, dir, "ok.md", "---\ntitle: Ok\n---\nbody\n") + + c, f := newFakeConfluence(t) + if _, err := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{path}) + }); err != nil { + t.Fatalf("run: %v", err) + } + if len(f.labelCalls) != 0 { + t.Errorf("label requests = %v, want none", f.labelCalls) + } +} + +// TestRunDryRunPreviewsLabelsWithoutAsking: the page does not exist in a dry +// run, so every declared label is an add and there is nothing to look up -- +// asking would be a request against an id that is not there. +func TestRunDryRunPreviewsLabelsWithoutAsking(t *testing.T) { + resetOpts(t) + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + dir := t.TempDir() + spaceOpt = "ENG" + dryRunOpt = true + path := write(t, dir, "ok.md", "---\ntitle: Ok\nlabels: [runbook]\n---\nbody\n") + + c, f := newFakeConfluence(t) + out, err := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{path}) + }) + if err != nil { + t.Fatalf("run: %v", err) + } + schematest.ValidateEnvelope(t, []byte(out)) + if len(f.labelCalls) != 0 { + t.Errorf("label requests = %v, want none in a dry run", f.labelCalls) + } + + var env struct { + Results []struct { + Labels []struct { + Action string `json:"action"` + Name string `json:"name"` + } `json:"labels"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("unmarshal %q: %v", out, err) + } + if len(env.Results) != 1 || len(env.Results[0].Labels) != 1 { + t.Fatalf("labels = %+v, want one previewed action", env.Results) + } + if got := env.Results[0].Labels[0]; got.Action != "added" || got.Name != "runbook" { + t.Errorf("previewed label = %+v, want runbook added", got) + } +} diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index becec6b..e1206f5 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -418,7 +418,7 @@ "additionalProperties": false, "required": [ "ok", "status", "dry_run", "file", "page_id", "title", "space", "parent", "parent_type", "parent_file", "url", - "page_width", "persisted", "attachments", "warnings", "broken", "error", "code" + "page_width", "labels", "persisted", "attachments", "warnings", "broken", "error", "code" ], "properties": { "ok": { "type": "boolean" }, @@ -433,6 +433,7 @@ "parent_file": { "$ref": "#/$defs/stringOrNull" }, "url": { "$ref": "#/$defs/stringOrNull" }, "page_width": { "$ref": "#/$defs/pageWidthOrNull" }, + "labels": { "$ref": "#/$defs/labelActionsOrNull" }, "persisted": { "type": "boolean" }, "attachments": { "type": "array", "items": { "$ref": "#/$defs/attachment" } }, "warnings": { "type": "array", "items": { "type": "string" } }, From 5427a00ef889c5e2c23c6ca746f1184158e3b49c Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 21:03:17 -0400 Subject: [PATCH 09/16] feat(fix): reconcile a page's labels into the file This is the only way to adopt a page somebody labeled in the UI, which is why it reconciles labels even for a file with no labels key -- the opposite of update and create, where an absent key means "leave the page alone". The directions are not symmetric and should not be: fix reconciles the file to the page, so the page is the authority here in exactly the way the file is there. Compared as sets, so a file that merely orders its labels differently, repeats one, or spells one in the wrong case is left alone and keeps the author's own ordering. When a write is needed the list is emitted sorted and deduplicated, since neither label GET returns a useful order. The live read is best-effort like the width read, and nil carries a meaning an empty slice cannot: "not known" rather than "the page has no labels". A failed read plans nothing, because the alternative is a transient failure proposing that every label be stripped from the file. Tested both ways. A block list stays a block list through the write, end to end, via UpdateListField. The change struct carries newList alongside the display string, so --json's `new` stays a string ("[a, b]") and the reported shape of a change does not vary by field -- no new schema case for one list field. Also: an invalid label in the file is reconciled rather than refused, the one place fix repairs something check would have failed. The live set is what gets written and it came from the server, so it is valid by construction -- which is what lets an author recover a file that already suffered the space-splitting bug. A convergence test pins the property that pair in the SRE space is the absence of: reconcile once, and the second run has nothing to do. --- cmd/fix/fix.go | 107 +++++++++++++++++++-- cmd/fix/fix_test.go | 228 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 317 insertions(+), 18 deletions(-) diff --git a/cmd/fix/fix.go b/cmd/fix/fix.go index 7ca79d9..e3109ad 100644 --- a/cmd/fix/fix.go +++ b/cmd/fix/fix.go @@ -13,6 +13,7 @@ import ( "github.com/mozilla/markfluence/internal/completion" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" "github.com/mozilla/markfluence/internal/ui" @@ -101,8 +102,15 @@ func run(cmd *cobra.Command, args []string) error { } // change is a planned frontmatter edit. +// +// newList, when non-nil, makes this a list-valued field: newValue is then the +// display rendering ("[a, b]") that both the human line and --json's `new` +// string carry, and newList is what actually gets written. Keeping the display +// a string means the reported shape of a change does not vary by field, so the +// schema needs no new case for one list field. type change struct { field, oldDisplay, newValue string + newList []string } // processFile reconciles one file and returns a result. It performs no output; @@ -132,7 +140,21 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult { liveWidth = string(w) } - r.changes = plannedChanges(mf.Frontmatter, page, liveWidth) + // The live labels, likewise best-effort. nil means "not known", which is + // distinct from an empty slice: a page with no labels and a page whose + // labels could not be read must not plan the same change, or a failed read + // would write `labels: []` and silently propose stripping the page. + var liveLabels []string + if live, err := labels.Read(c, page.ID); err != nil { + r.warnings = append(r.warnings, "could not read labels: "+err.Error()) + } else { + liveLabels = labels.Global(live) + if liveLabels == nil { + liveLabels = []string{} + } + } + + r.changes = plannedChanges(mf, page, liveWidth, liveLabels) // Field order is reconciled too, and counts as a change: reporting a // jumbled file "consistent" would mean running fix, being told there is // nothing to do, and still having a jumbled file. Computed before any edit, @@ -158,7 +180,12 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult { content := mf.Content for _, ch := range r.changes { var err error - if content, err = frontmatter.UpdateField(content, ch.field, ch.newValue, ""); err != nil { + if ch.newList != nil { + content, err = frontmatter.UpdateListField(content, ch.field, ch.newList) + } else { + content, err = frontmatter.UpdateField(content, ch.field, ch.newValue, "") + } + if err != nil { return r.fail(err, jsonout.CodeValidation) } } @@ -216,9 +243,12 @@ func locatePage(fm map[string]string, c *client.ConfluenceClient) (*client.Page, } } -// plannedChanges computes the field edits needed to reconcile fm to page. Only +// plannedChanges computes the field edits needed to reconcile mf to page. Only // fields that actually differ are returned. -func plannedChanges(fm map[string]string, page *client.Page, liveWidth string) []change { +func plannedChanges( + mf *frontmatter.MarkdownFile, page *client.Page, liveWidth string, liveLabels []string, +) []change { + fm := mf.Frontmatter live := []struct{ field, value string }{ {"page_id", page.ID}, {"space", client.SpaceKeyFromWebUI(page.Links.WebUI)}, @@ -233,7 +263,7 @@ func plannedChanges(fm map[string]string, page *client.Page, liveWidth string) [ current, present := fm[lv.field] switch { case !present: - changes = append(changes, change{lv.field, "(none)", lv.value}) + changes = append(changes, change{field: lv.field, oldDisplay: "(none)", newValue: lv.value}) case norm(current) != norm(lv.value): // A present-but-blank value goes through norm, not straight to // "(none)": every null spelling now parses to "", so a top-level @@ -241,12 +271,12 @@ func plannedChanges(fm map[string]string, page *client.Page, liveWidth string) [ // orNull("null") the live side reports. Short-circuiting on blank // would plan `parent: (none) -> null` on every run, write it, read // "" again, and never converge. - changes = append(changes, change{lv.field, orNone(current), lv.value}) + changes = append(changes, change{field: lv.field, oldDisplay: orNone(current), newValue: lv.value}) } } if strings.TrimSpace(fm["title"]) == "" { - changes = append(changes, change{"title", "(none)", page.Title}) + changes = append(changes, change{field: "title", oldDisplay: "(none)", newValue: page.Title}) } if liveWidth != "" { @@ -260,12 +290,73 @@ func plannedChanges(fm map[string]string, page *client.Page, liveWidth string) [ if present && strings.TrimSpace(raw) != "" { old = raw } - changes = append(changes, change{"page_width", old, liveWidth}) + changes = append(changes, change{field: "page_width", oldDisplay: old, newValue: liveWidth}) } } + + if ch, ok := labelChange(mf, liveLabels); ok { + changes = append(changes, ch) + } return changes } +// labelChange plans the labels edit, if one is needed. +// +// This is the only way to adopt a page somebody labeled by hand, so it runs +// even for a file with no labels key at all -- unlike update and create, where +// an absent key means "leave the page alone". The directions are not symmetric +// and should not be: fix reconciles the *file* to the page, so the page is the +// authority here in exactly the way the file is there. +// +// Compared as sets, so a file whose list is merely reordered or holds a +// duplicate is left alone and keeps the author's own ordering. When a write is +// needed the list is emitted sorted and deduplicated, since neither label GET +// returns a useful order and anything else would be unstable across runs. +// +// A file whose labels are invalid is reconciled rather than refused: the live +// set is what is about to be written, and it came from the server, so it is +// valid by construction. That is the one place fix repairs a file check would +// have failed. +func labelChange(mf *frontmatter.MarkdownFile, liveLabels []string) (change, bool) { + // nil means the read failed. Planning nothing is right: a change here would + // propose the file's own labels be replaced by a set nobody could see. + if liveLabels == nil { + return change{}, false + } + declared, present := mf.Lists[labels.Field] + normalized := make([]string, 0, len(declared)) + for _, d := range declared { + n, _ := labels.Normalize(d) + normalized = append(normalized, n) + } + if present { + if add, remove, _ := labels.Diff(normalized, liveLabels); len(add) == 0 && len(remove) == 0 { + return change{}, false + } + } else if len(liveLabels) == 0 { + // No key and no labels: nothing to adopt, and writing "labels: []" + // would add a field that says nothing to every file fix touches. + return change{}, false + } + + old := noneDisplay + if present { + old = renderLabelList(declared) + } + return change{ + field: labels.Field, + oldDisplay: old, + newValue: renderLabelList(liveLabels), + newList: liveLabels, + }, true +} + +// renderLabelList renders a label list the way the frontmatter writes it, for +// the human line and --json's `new` string. +func renderLabelList(names []string) string { + return "[" + strings.Join(names, ", ") + "]" +} + // norm treats "", whitespace-only, and the literal "null" all as no value. func norm(value string) string { t := strings.TrimSpace(value) diff --git a/cmd/fix/fix_test.go b/cmd/fix/fix_test.go index 8354177..02487c2 100644 --- a/cmd/fix/fix_test.go +++ b/cmd/fix/fix_test.go @@ -10,6 +10,7 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/clienttest" + "github.com/mozilla/markfluence/internal/frontmatter" ) // --- locatePage -------------------------------------------------------------- @@ -113,7 +114,7 @@ func TestPlannedChangesNoneWhenConsistent(t *testing.T) { "page_id": "123", "space": "ENG", "parent": "null", "title": "Runbook", "page_width": "max", } page := &client.Page{ID: "123", Title: "Runbook", Links: client.Links{WebUI: "/spaces/ENG/pages/123/Runbook"}} - got := plannedChanges(fm, page, "max") + got := plannedChangesFM(fm, page, "max") if len(got) != 0 { t.Errorf("changes = %+v, want none", got) } @@ -121,7 +122,7 @@ func TestPlannedChangesNoneWhenConsistent(t *testing.T) { func TestPlannedChangesFillsMissingFields(t *testing.T) { page := &client.Page{ID: "123", Title: "Runbook", Links: client.Links{WebUI: "/spaces/ENG/pages/123/Runbook"}} - got := plannedChanges(map[string]string{}, page, "") + got := plannedChangesFM(map[string]string{}, page, "") want := map[string]string{"page_id": "123", "space": "ENG", "parent": "null", "title": "Runbook"} if len(got) != len(want) { t.Fatalf("changes = %+v, want %d entries", got, len(want)) @@ -139,7 +140,7 @@ func TestPlannedChangesFillsMissingFields(t *testing.T) { func TestPlannedChangesUpdatesFieldsThatDiffer(t *testing.T) { fm := map[string]string{"page_id": "999", "space": "OLD", "parent": "1"} page := &client.Page{ID: "123", ParentID: "2", Links: client.Links{WebUI: "/spaces/ENG/pages/123/Runbook"}} - got := plannedChanges(fm, page, "") + got := plannedChangesFM(fm, page, "") byField := map[string]change{} for _, ch := range got { byField[ch.field] = ch @@ -162,7 +163,7 @@ func TestPlannedChangesParentNullNormalizes(t *testing.T) { // parser can no longer produce and would pass while fix looped forever. fm := map[string]string{"parent": ""} page := &client.Page{ID: "1", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} - got := plannedChanges(fm, page, "") + got := plannedChangesFM(fm, page, "") for _, ch := range got { if ch.field == "parent" { t.Errorf("parent change = %+v, want none (both sides are null)", ch) @@ -173,7 +174,7 @@ func TestPlannedChangesParentNullNormalizes(t *testing.T) { func TestPlannedChangesTitlePresentIsUntouched(t *testing.T) { fm := map[string]string{"page_id": "1", "space": "ENG", "parent": "null", "title": "Kept"} page := &client.Page{ID: "1", Title: "Live Title", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} - got := plannedChanges(fm, page, "") + got := plannedChangesFM(fm, page, "") for _, ch := range got { if ch.field == "title" { t.Errorf("title change = %+v, want none: an existing title is never overwritten", ch) @@ -184,7 +185,7 @@ func TestPlannedChangesTitlePresentIsUntouched(t *testing.T) { func TestPlannedChangesSkipsWidthWhenLiveWidthUnknown(t *testing.T) { fm := map[string]string{"page_id": "1", "space": "ENG", "parent": "null", "title": "X"} page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} - got := plannedChanges(fm, page, "") + got := plannedChangesFM(fm, page, "") for _, ch := range got { if ch.field == "page_width" { t.Errorf("page_width change = %+v, want none when liveWidth is unknown", ch) @@ -197,7 +198,7 @@ func TestPlannedChangesWidthDefaultsToMaxWhenUnset(t *testing.T) { page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} t.Run("live width already max: no change", func(t *testing.T) { - got := plannedChanges(fm, page, "max") + got := plannedChangesFM(fm, page, "max") for _, ch := range got { if ch.field == "page_width" { t.Errorf("page_width change = %+v, want none: unset frontmatter defaults to max", ch) @@ -205,7 +206,7 @@ func TestPlannedChangesWidthDefaultsToMaxWhenUnset(t *testing.T) { } }) t.Run("live width differs: filled from (none)", func(t *testing.T) { - got := plannedChanges(fm, page, "narrow") + got := plannedChangesFM(fm, page, "narrow") var found *change for i, ch := range got { if ch.field == "page_width" { @@ -223,7 +224,7 @@ func TestPlannedChangesWidthCaseInsensitive(t *testing.T) { "page_id": "1", "space": "ENG", "parent": "null", "title": "X", "page_width": " Wide ", } page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} - got := plannedChanges(fm, page, "wide") + got := plannedChangesFM(fm, page, "wide") for _, ch := range got { if ch.field == "page_width" { t.Errorf("page_width change = %+v, want none: %q normalizes to wide", ch, fm["page_width"]) @@ -236,7 +237,7 @@ func TestPlannedChangesWidthDiffers(t *testing.T) { "page_id": "1", "space": "ENG", "parent": "null", "title": "X", "page_width": "narrow", } page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} - got := plannedChanges(fm, page, "max") + got := plannedChangesFM(fm, page, "max") var found *change for i, ch := range got { if ch.field == "page_width" { @@ -427,3 +428,210 @@ func TestProcessFileTopLevelPageConverges(t *testing.T) { t.Fatalf("status = %q with changes %+v, want consistent", r.status, r.changes) } } + +// plannedChangesFM adapts plannedChanges for the tests that predate labels: +// no list fields, and a nil liveLabels meaning the label read failed, which +// plans no label change at all. That is exactly what a width or title test +// wants -- one field under test and nothing else moving. +func plannedChangesFM(fm map[string]string, page *client.Page, liveWidth string) []change { + mf := &frontmatter.MarkdownFile{Frontmatter: fm, Lists: map[string][]string{}} + return plannedChanges(mf, page, liveWidth, nil) +} + +// --- labels ------------------------------------------------------------------- + +// mdFile parses a frontmatter block into the MarkdownFile plannedChanges takes, +// so a label test exercises the real reader rather than a hand-built Lists map. +func mdFile(t *testing.T, block string) *frontmatter.MarkdownFile { + t.Helper() + mf, err := frontmatter.Parse("doc.md", "---\n"+block+"\n---\nbody\n") + if err != nil { + t.Fatalf("Parse(%q) = %v", block, err) + } + return mf +} + +func labelChangeIn(changes []change) (change, bool) { + for _, ch := range changes { + if ch.field == "labels" { + return ch, true + } + } + return change{}, false +} + +// TestFixAdoptsHandLabels is the reason fix reconciles labels at all: it is the +// only way to take over a page somebody labeled in the UI. Note the direction +// -- update leaves an absent key alone, fix fills it in, because fix reconciles +// the file to the page and update the page to the file. +func TestFixAdoptsHandLabels(t *testing.T) { + page := &client.Page{ID: "1", Title: "T"} + got := plannedChanges(mdFile(t, "page_id: 1"), page, "", []string{"ci/cd", "runbook"}) + + ch, ok := labelChangeIn(got) + if !ok { + t.Fatalf("changes = %+v, want a labels change", got) + } + if ch.oldDisplay != noneDisplay { + t.Errorf("old = %q, want %q", ch.oldDisplay, noneDisplay) + } + if ch.newValue != "[ci/cd, runbook]" { + t.Errorf("new = %q, want [ci/cd, runbook]", ch.newValue) + } + if len(ch.newList) != 2 { + t.Errorf("newList = %v, want the two names to write", ch.newList) + } +} + +// TestFixLeavesAMatchingSetAlone: compared as sets, so a file that merely +// orders its labels differently or repeats one is not rewritten, and the +// author's own ordering survives. +func TestFixLeavesAMatchingSetAlone(t *testing.T) { + page := &client.Page{ID: "1", Title: "T"} + for _, block := range []string{ + "page_id: 1\nlabels: [ci/cd, runbook]", + "page_id: 1\nlabels: [runbook, ci/cd]", + "page_id: 1\nlabels: [runbook, ci/cd, runbook]", + "page_id: 1\nlabels: [Runbook, CI/CD]", + } { + got := plannedChanges(mdFile(t, block), page, "", []string{"ci/cd", "runbook"}) + if ch, ok := labelChangeIn(got); ok { + t.Errorf("%q planned %+v, want no labels change", block, ch) + } + } +} + +// TestFixPlansNothingWhenNeitherHasLabels: a file with no key and a page with +// no labels must not gain a "labels: []" that says nothing. +func TestFixPlansNothingWhenNeitherHasLabels(t *testing.T) { + page := &client.Page{ID: "1", Title: "T"} + got := plannedChanges(mdFile(t, "page_id: 1"), page, "", []string{}) + if ch, ok := labelChangeIn(got); ok { + t.Errorf("planned %+v, want no labels change", ch) + } +} + +// TestFixPlansNothingWhenTheReadFailed is the distinction nil carries. A failed +// read must not look like "the page has no labels", or a transient failure +// would propose stripping every label from the file. +func TestFixPlansNothingWhenTheReadFailed(t *testing.T) { + page := &client.Page{ID: "1", Title: "T"} + got := plannedChanges(mdFile(t, "page_id: 1\nlabels: [runbook]"), page, "", nil) + if ch, ok := labelChangeIn(got); ok { + t.Errorf("planned %+v, want no labels change when the read failed", ch) + } +} + +// TestFixRemovesLabelsThePageNoLongerHas: reconciling downward too, including +// to the empty set, which is a real state a page can be in. +func TestFixRemovesLabelsThePageNoLongerHas(t *testing.T) { + page := &client.Page{ID: "1", Title: "T"} + got := plannedChanges(mdFile(t, "page_id: 1\nlabels: [runbook, gone]"), page, "", []string{}) + + ch, ok := labelChangeIn(got) + if !ok { + t.Fatalf("changes = %+v, want a labels change", got) + } + if ch.newValue != "[]" { + t.Errorf("new = %q, want []", ch.newValue) + } + if ch.newList == nil || len(ch.newList) != 0 { + t.Errorf("newList = %v, want a non-nil empty list", ch.newList) + } +} + +// TestFixReconcilesAnInvalidLabel: the live set is what gets written and it +// came from the server, so it is valid by construction. This is the one place +// fix repairs a file check would have refused. +func TestFixReconcilesAnInvalidLabel(t *testing.T) { + page := &client.Page{ID: "1", Title: "T"} + got := plannedChanges(mdFile(t, `page_id: 1 +labels: ["Runbook Two"]`), page, "", []string{"runbook", "two"}) + + ch, ok := labelChangeIn(got) + if !ok { + t.Fatalf("changes = %+v, want the invalid label reconciled", got) + } + if ch.newValue != "[runbook, two]" { + t.Errorf("new = %q, want [runbook, two]", ch.newValue) + } +} + +// labelFixServer answers the page, the width property, and a label list. +func labelFixServer(t *testing.T, page string, liveLabels ...string) *client.ConfluenceClient { + t.Helper() + rows := make([]string, 0, len(liveLabels)) + for i, n := range liveLabels { + rows = append(rows, fmt.Sprintf(`{"id":"%d","name":%q,"prefix":"global"}`, i+1, n)) + } + body := `{"results":[` + strings.Join(rows, ",") + `]}` + return clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "label"): + _, _ = w.Write([]byte(body)) + case strings.HasSuffix(r.URL.Path, "/properties"): + _, _ = w.Write([]byte(`{"results":[{"value":"max"}]}`)) + default: + _, _ = w.Write([]byte(page)) + } + }) +} + +// TestProcessFileKeepsBlockLabelStyle is the end-to-end form-preservation +// contract. A set large enough to be written as a block list is exactly the set +// whose flow spelling is an unreadable single line, so converting it on the +// first fix that changes one label would defeat the reason block form is +// accepted at all. +func TestProcessFileKeepsBlockLabelStyle(t *testing.T) { + content := "---\ntitle: X\nspace: ENG\nparent: null\npage_id: 1\n" + + "labels:\n - runbook\n - stale\npage_width: max\n---\nbody\n" + path := writeFixture(t, content) + c := labelFixServer(t, pageJSON("1", "X", "", "/spaces/ENG/pages/1/X"), "runbook", "howto") + + r := processFile(path, c) + if !r.ok || r.status != statusChanged { + t.Fatalf("result = %+v, want ok/changed", r) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(got), "labels: [") { + t.Errorf("a block list was converted to flow:\n%s", got) + } + for _, want := range []string{"- howto", "- runbook"} { + if !strings.Contains(string(got), want) { + t.Errorf("file = %q, want a %q item", got, want) + } + } + if strings.Contains(string(got), "stale") { + t.Errorf("file = %q, want the dropped label gone", got) + } +} + +// TestProcessFileLabelFixConverges is the property the "continuous"/"delivery" +// pair in the SRE space is the absence of: reconcile once, and the second run +// has nothing to do. A file that never converges means running fix, being told +// it changed something, and getting the same change forever. +func TestProcessFileLabelFixConverges(t *testing.T) { + content := "---\ntitle: X\nspace: ENG\nparent: null\npage_id: 1\npage_width: max\n---\nbody\n" + path := writeFixture(t, content) + c := labelFixServer(t, pageJSON("1", "X", "", "/spaces/ENG/pages/1/X"), "ci/cd", "runbook") + + first := processFile(path, c) + if !first.ok || first.status != statusChanged { + t.Fatalf("first run = %+v, want ok/changed", first) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(got), "labels: [ci/cd, runbook]") { + t.Errorf("file = %q, want a sorted flow list for a newly added key", got) + } + + second := processFile(path, c) + if !second.ok || second.status != statusConsistent { + t.Fatalf("second run = %+v (changes %+v), want ok/consistent", second, second.changes) + } +} From 30867e59716835297f8fc529891056dc5d212176 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 21:04:48 -0400 Subject: [PATCH 10/16] feat(info): show a page's labels, managed and not Two rows, the second appearing only when there is something in it per info's existing "empty fields omitted" rule: page_width: max (Confluence default) labels: ci/cd, howto, runbook labels/unmanaged: my:mine, team:eng info is the one command that shows what markfluence will not touch, which is why client.ListLabels returns the list unfiltered: a my: label is personal and a team: one belongs to a space's own vocabulary, neither has a frontmatter spelling, and a reader looking at a page's labels should see all of them rather than the subset markfluence happens to manage. The fetch is best-effort like the width's -- a page nobody can label is still worth describing. In --json, [] means the page genuinely has none and null means the fetch failed, the distinction the report keeps labelsKnown for; collapsing them would make a transient failure indistinguishable from a fact about the page. `managed` travels with each label rather than being left to a consumer to derive, so the prefix rule can only be wrong in one place. --- cmd/info/info.go | 17 ++++++++ cmd/info/json.go | 50 +++++++++++++++++------- cmd/info/json_test.go | 79 ++++++++++++++++++++++++++++++++++++++ schema/json-output/v1.json | 3 +- 4 files changed, 134 insertions(+), 15 deletions(-) diff --git a/cmd/info/info.go b/cmd/info/info.go index e2fcbb1..1589b14 100644 --- a/cmd/info/info.go +++ b/cmd/info/info.go @@ -12,6 +12,7 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/completion" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" "github.com/mozilla/markfluence/internal/ui" @@ -126,6 +127,12 @@ type report struct { withProps bool properties []client.Property propsErr error + // labelsKnown separates "the page has no labels" from "the fetch failed", + // which --json reports as [] and null respectively. info is the one command + // that shows unmanaged labels, so it keeps the client's unfiltered list + // rather than a split it would then have to re-derive. + labelsKnown bool + labels []client.Label } // buildReport resolves a page (and, when withProps is set, its content @@ -177,6 +184,14 @@ func buildReport(page *client.Page, c *client.ConfluenceClient, withProps bool) r.widthKnown = true r.width = jsonout.PageWidth{Value: string(width), Default: !explicit} } + + // Best-effort, like the width: a page nobody can label is still worth + // describing, so a failed fetch leaves the rows out rather than failing the + // command. + if live, err := labels.Read(c, page.ID); err == nil { + r.labelsKnown = true + r.labels = live + } return r } @@ -202,6 +217,8 @@ func (r report) human() string { {"parent", parent}, {"version", versionNumber(r.versionNum)}, {"page_width", widthDisplay}, + {"labels", strings.Join(labels.Global(r.labels), ", ")}, + {"labels/unmanaged", strings.Join(labels.Unmanaged(r.labels), ", ")}, {"created", withAuthor(r.createdAt, r.creator)}, {"updated", withAuthor(r.updatedAt, r.editor)}, {"message", r.message}, diff --git a/cmd/info/json.go b/cmd/info/json.go index 727618e..0008646 100644 --- a/cmd/info/json.go +++ b/cmd/info/json.go @@ -5,6 +5,7 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" ) // jsonInfoResult is info's --json result shape. Keys are always present (per the @@ -12,20 +13,21 @@ import ( // as null. page_status is Confluence's page status, renamed to avoid colliding // with the action commands' result-status concept. type jsonInfoResult struct { - OK bool `json:"ok"` - PageID string `json:"page_id"` - Title string `json:"title"` - PageStatus string `json:"page_status"` - Space string `json:"space"` - Parent *string `json:"parent"` - ParentType *string `json:"parent_type"` - Version jsonVersion `json:"version"` - PageWidth *jsonout.PageWidth `json:"page_width"` - Created *jsonout.Stamp `json:"created"` - Updated *jsonout.Stamp `json:"updated"` - Message string `json:"message"` - URL string `json:"url"` - Properties []jsonProperty `json:"properties"` + OK bool `json:"ok"` + PageID string `json:"page_id"` + Title string `json:"title"` + PageStatus string `json:"page_status"` + Space string `json:"space"` + Parent *string `json:"parent"` + ParentType *string `json:"parent_type"` + Version jsonVersion `json:"version"` + PageWidth *jsonout.PageWidth `json:"page_width"` + Labels *[]jsonout.LabelInfo `json:"labels"` + Created *jsonout.Stamp `json:"created"` + Updated *jsonout.Stamp `json:"updated"` + Message string `json:"message"` + URL string `json:"url"` + Properties []jsonProperty `json:"properties"` } type jsonVersion struct { @@ -57,6 +59,26 @@ func (r report) jsonResult() jsonInfoResult { w := r.width res.PageWidth = &w } + // null when the fetch failed, [] when the page genuinely has none. A + // consumer should not have to know the prefix rule to reproduce the + // managed/unmanaged split, so managed is reported per label rather than + // left to be derived. + if r.labelsKnown { + res.Labels = &[]jsonout.LabelInfo{} + list := make([]jsonout.LabelInfo, 0, len(r.labels)) + for _, l := range r.labels { + list = append(list, jsonout.LabelInfo{ + Name: l.Name, Prefix: l.Prefix, Managed: l.Prefix == labels.ManagedPrefix, + }) + } + sort.Slice(list, func(i, j int) bool { + if list[i].Prefix != list[j].Prefix { + return list[i].Prefix < list[j].Prefix + } + return list[i].Name < list[j].Name + }) + res.Labels = &list + } // properties stays null unless --properties was given and the fetch succeeded; // then it is a (possibly empty) sorted array. if r.withProps && r.propsErr == nil { diff --git a/cmd/info/json_test.go b/cmd/info/json_test.go index 1e41dfe..2887e81 100644 --- a/cmd/info/json_test.go +++ b/cmd/info/json_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "strings" "testing" "github.com/mozilla/markfluence/internal/client" @@ -59,6 +60,7 @@ func TestJSONResultFull(t *testing.T) { "value": "max", "default": true }, + "labels": null, "created": { "at": "2026-07-01T00:00:00Z", "by": { @@ -144,3 +146,80 @@ func TestSchemaConformance(t *testing.T) { } schematest.ValidateEnvelope(t, buf.Bytes()) } + +// --- labels ------------------------------------------------------------------- + +// TestHumanShowsLabelRows pins the two-row layout, and that the unmanaged row +// appears only when there is something in it -- info's existing "empty fields +// omitted" rule. +func TestHumanShowsLabelRows(t *testing.T) { + r := report{ + id: "1", title: "T", status: "current", space: "ENG", versionNum: 1, + labelsKnown: true, + labels: []client.Label{ + {Name: "runbook", Prefix: "global"}, + {Name: "ci/cd", Prefix: "global"}, + {Name: "mine", Prefix: "my"}, + }, + } + out := r.human() + if !strings.Contains(out, "labels:") { + t.Errorf("output = %q, want a labels row", out) + } + if !strings.Contains(out, "ci/cd, runbook") { + t.Errorf("output = %q, want the managed labels sorted", out) + } + if !strings.Contains(out, "labels/unmanaged:") || !strings.Contains(out, "my:mine") { + t.Errorf("output = %q, want the unmanaged row naming my:mine", out) + } +} + +func TestHumanOmitsUnmanagedRowWhenEmpty(t *testing.T) { + r := report{ + id: "1", title: "T", status: "current", space: "ENG", versionNum: 1, + labelsKnown: true, + labels: []client.Label{{Name: "runbook", Prefix: "global"}}, + } + if out := r.human(); strings.Contains(out, "unmanaged") { + t.Errorf("output = %q, want no unmanaged row when there are none", out) + } +} + +// TestJSONLabelsSeparateNoneFromUnknown: [] means the page has none, null means +// the fetch failed. Collapsing them would make a transient failure +// indistinguishable from a fact about the page. +func TestJSONLabelsSeparateNoneFromUnknown(t *testing.T) { + none := report{labelsKnown: true}.jsonResult() + if none.Labels == nil { + t.Error("labels = null for a page with no labels, want []") + } else if len(*none.Labels) != 0 { + t.Errorf("labels = %v, want []", *none.Labels) + } + + unknown := report{labelsKnown: false}.jsonResult() + if unknown.Labels != nil { + t.Errorf("labels = %v for a failed fetch, want null", *unknown.Labels) + } +} + +// TestJSONLabelsReportManaged: a consumer should not have to know the prefix +// rule to reproduce the split, so managed travels with each label. +func TestJSONLabelsReportManaged(t *testing.T) { + r := report{ + labelsKnown: true, + labels: []client.Label{ + {Name: "mine", Prefix: "my"}, + {Name: "runbook", Prefix: "global"}, + }, + } + got := r.jsonResult().Labels + if got == nil || len(*got) != 2 { + t.Fatalf("labels = %v, want two", got) + } + for _, l := range *got { + wantManaged := l.Prefix == "global" + if l.Managed != wantManaged { + t.Errorf("%s:%s managed = %v, want %v", l.Prefix, l.Name, l.Managed, wantManaged) + } + } +} diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index e1206f5..64cddea 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -323,7 +323,7 @@ "additionalProperties": false, "required": [ "ok", "page_id", "title", "page_status", "space", "parent", "parent_type", - "version", "page_width", "created", "updated", "message", "url", "properties" + "version", "page_width", "labels", "created", "updated", "message", "url", "properties" ], "properties": { "ok": { "const": true }, @@ -340,6 +340,7 @@ "properties": { "number": { "type": "integer" } } }, "page_width": { "$ref": "#/$defs/pageWidthOrNull" }, + "labels": { "$ref": "#/$defs/labelInfoListOrNull" }, "created": { "$ref": "#/$defs/stampOrNull" }, "updated": { "$ref": "#/$defs/stampOrNull" }, "message": { "type": "string" }, From adb281e461ce66776cc92e364e53551d1030e861 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 21:06:50 -0400 Subject: [PATCH 11/16] feat(read): emit labels in rendered frontmatter Through pagedoc, so read and export gain it together and cannot drift -- the same reason Render/Options exist. A round-tripped tree now republishes its labels instead of dropping them, which is what made an export lossy for the one piece of metadata people actually search on. Global labels only: a my: or team: label has no frontmatter spelling, so emitting one would produce a file that cannot publish what it says. Sorted, because neither label GET returns a useful order and a tree that reshuffles its own frontmatter between runs is noise in every later diff. An empty list and a failed fetch are written the same way -- no labels: key at all -- and that is the one place this deliberately does *not* mirror how update reads the field. An emitted "labels: []" would tell the next publish to remove every label, so a read of a page with no labels would quietly assert that on the author's behalf and strip anything added in the UI in between. Pinned by a test covering both cases. read --json gets `labels` as a plain sorted string array, a different shape from info's on purpose: info describes the page, read describes the document it produced. null when the fetch failed, so "none" and "unknown" stay distinguishable. Costs one extra request per page on an export walk, which is the price of the round trip being complete. --- cmd/read/json.go | 14 +++++++++ cmd/read/json_test.go | 1 + internal/pagedoc/pagedoc.go | 27 +++++++++++++++-- internal/pagedoc/pagedoc_test.go | 50 +++++++++++++++++++++++++++++--- schema/json-output/v1.json | 9 +++++- 5 files changed, 94 insertions(+), 7 deletions(-) diff --git a/cmd/read/json.go b/cmd/read/json.go index 0b4c635..7c5f110 100644 --- a/cmd/read/json.go +++ b/cmd/read/json.go @@ -3,6 +3,7 @@ package read import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/pagewidth" ) @@ -18,6 +19,7 @@ type jsonReadResult struct { Parent *string `json:"parent"` ParentType *string `json:"parent_type"` PageWidth *jsonout.PageWidth `json:"page_width"` + Labels *[]string `json:"labels"` Format string `json:"format"` Body string `json:"body"` } @@ -46,5 +48,17 @@ func buildResult(c *client.ConfluenceClient, page *client.Page, format, body str if w, explicit, err := pagewidth.Read(c, page.ID); err == nil { res.PageWidth = &jsonout.PageWidth{Value: string(w), Default: !explicit} } + // A plain string array, deliberately a different shape from info's: info + // describes the page, read describes the document it produced. So this is + // exactly what went into the rendered frontmatter -- global-only and + // sorted -- and null when the fetch failed, so "none" and "unknown" stay + // distinguishable. + if live, err := labels.Read(c, page.ID); err == nil { + names := labels.Global(live) + if names == nil { + names = []string{} + } + res.Labels = &names + } return res } diff --git a/cmd/read/json_test.go b/cmd/read/json_test.go index e69c9e7..9af01e8 100644 --- a/cmd/read/json_test.go +++ b/cmd/read/json_test.go @@ -65,6 +65,7 @@ func TestJSONReadResultMarshal(t *testing.T) { "value": "max", "default": true }, + "labels": null, "format": "markdown", "body": "# X\n\nhello" }` diff --git a/internal/pagedoc/pagedoc.go b/internal/pagedoc/pagedoc.go index 5bf5b8d..faa093b 100644 --- a/internal/pagedoc/pagedoc.go +++ b/internal/pagedoc/pagedoc.go @@ -20,6 +20,7 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/labels" "github.com/mozilla/markfluence/internal/pageslug" "github.com/mozilla/markfluence/internal/pagewidth" "github.com/mozilla/markfluence/internal/ui" @@ -275,13 +276,32 @@ func Frontmatter(c *client.ConfluenceClient, page *client.Page, parentOverride s if w, _, err := pagewidth.Read(c, page.ID); err == nil { width = string(w) } - return RenderFrontmatter(page.Title, client.SpaceKeyFromWebUI(page.Links.WebUI), parent, page.ID, width) + // Best-effort, in the same shape as every other lookup here: a failed + // fetch omits the field rather than failing the render. Only the managed + // labels are emitted -- a my: or team: label has no frontmatter spelling, + // so writing one would produce a file that cannot publish what it says. + // + // Sorted, because neither label GET returns a useful order and an exported + // tree that reshuffles its own frontmatter between runs is noise in every + // later diff. + var names []string + if live, err := labels.Read(c, page.ID); err == nil { + names = labels.Global(live) + } + return RenderFrontmatter( + page.Title, client.SpaceKeyFromWebUI(page.Links.WebUI), parent, page.ID, width, names) } // RenderFrontmatter assembles the frontmatter block from resolved field values, // omitting space/parent/page_width when empty. frontmatter.Render emits them in // the canonical order and quotes values as YAML needs. -func RenderFrontmatter(title, space, parent, pageID, width string) string { +// +// labels is nil when the fetch failed and empty when the page has none, and the +// two are written the same way -- no labels: key at all. That differs from +// update's reading of the field, deliberately: an emitted "labels: []" would +// tell a later publish to remove every label, which is not something a read of +// a page with no labels should assert on the author's behalf. +func RenderFrontmatter(title, space, parent, pageID, width string, labels []string) string { fields := []frontmatter.Field{{Key: "title", Value: title}} if space != "" { fields = append(fields, frontmatter.Field{Key: "space", Value: space}) @@ -293,5 +313,8 @@ func RenderFrontmatter(title, space, parent, pageID, width string) string { if width != "" { fields = append(fields, frontmatter.Field{Key: "page_width", Value: width}) } + if len(labels) > 0 { + fields = append(fields, frontmatter.Field{Key: "labels", List: labels}) + } return frontmatter.Render(fields) } diff --git a/internal/pagedoc/pagedoc_test.go b/internal/pagedoc/pagedoc_test.go index 4d177dc..d11e749 100644 --- a/internal/pagedoc/pagedoc_test.go +++ b/internal/pagedoc/pagedoc_test.go @@ -1,6 +1,7 @@ package pagedoc import ( + "strings" "testing" "github.com/mozilla/markfluence/internal/client" @@ -9,7 +10,7 @@ import ( func TestRenderFrontmatter(t *testing.T) { // Fields come out in the canonical order (title, space, parent, page_id, then // the rest) regardless of the order renderFrontmatter writes them. - got := RenderFrontmatter("My Page", "ENG", "456", "123456", "max") + got := RenderFrontmatter("My Page", "ENG", "456", "123456", "max", nil) want := "---\ntitle: My Page\nspace: ENG\nparent: 456\npage_id: 123456\npage_width: max\n---\n" if got != want { t.Errorf("RenderFrontmatter =\n%q\nwant\n%q", got, want) @@ -18,7 +19,7 @@ func TestRenderFrontmatter(t *testing.T) { func TestRenderFrontmatterTopLevelParent(t *testing.T) { // A top-level page carries parent: null. - got := RenderFrontmatter("T", "ENG", "null", "1", "max") + got := RenderFrontmatter("T", "ENG", "null", "1", "max", nil) want := "---\ntitle: T\nspace: ENG\nparent: null\npage_id: 1\npage_width: max\n---\n" if got != want { t.Errorf("RenderFrontmatter =\n%q\nwant\n%q", got, want) @@ -26,7 +27,7 @@ func TestRenderFrontmatterTopLevelParent(t *testing.T) { } func TestRenderFrontmatterOmitsEmptyFields(t *testing.T) { - got := RenderFrontmatter("T", "", "", "1", "") + got := RenderFrontmatter("T", "", "", "1", "", nil) want := "---\ntitle: T\npage_id: 1\n---\n" if got != want { t.Errorf("RenderFrontmatter =\n%q\nwant\n%q", got, want) @@ -35,7 +36,7 @@ func TestRenderFrontmatterOmitsEmptyFields(t *testing.T) { func TestRenderFrontmatterQuotesWhenNeeded(t *testing.T) { // A title with a leading '#' would be read as a comment unless quoted. - got := RenderFrontmatter("# Sharp", "", "", "1", "") + got := RenderFrontmatter("# Sharp", "", "", "1", "", nil) want := "---\ntitle: \"# Sharp\"\npage_id: 1\n---\n" if got != want { t.Errorf("RenderFrontmatter =\n%q\nwant\n%q", got, want) @@ -84,3 +85,44 @@ func TestDocString(t *testing.T) { t.Errorf("String() = %q, want %q", d.String(), want) } } + +// TestRenderFrontmatterEmitsLabels pins the field's place in the block: labels +// is not in fieldOrder, so it sorts alphabetically among the trailing keys and +// lands after page_id but before page_width. +func TestRenderFrontmatterEmitsLabels(t *testing.T) { + got := RenderFrontmatter("T", "ENG", "null", "1", "max", []string{"ci/cd", "runbook"}) + want := "---\ntitle: T\nspace: ENG\nparent: null\npage_id: 1\nlabels: [ci/cd, runbook]\npage_width: max\n---\n" + if got != want { + t.Errorf("RenderFrontmatter =\n%q\nwant\n%q", got, want) + } +} + +// TestRenderFrontmatterOmitsEmptyLabels: a page with no labels, and a page +// whose labels could not be read, both get no labels: key. +// +// Not "labels: []", which is what update reads as "remove every label". A read +// of a page that has none must not assert that on the author's behalf -- the +// round trip would then strip any label added in the UI in between. +func TestRenderFrontmatterOmitsEmptyLabels(t *testing.T) { + for name, given := range map[string][]string{ + "fetch failed": nil, + "none on page": {}, + } { + got := RenderFrontmatter("T", "", "", "1", "", given) + if strings.Contains(got, "labels") { + t.Errorf("%s: RenderFrontmatter = %q, want no labels key", name, got) + } + } +} + +// TestRenderFrontmatterQuotesALabelThatNeedsIt: labels go through the same +// verified writer as every other value, so a name YAML would misread is +// quoted. No valid Confluence label needs this -- every character that would +// break a flow sequence is in the server's reject set -- which is exactly why +// it is worth pinning that the general path is still being used. +func TestRenderFrontmatterQuotesALabelThatNeedsIt(t *testing.T) { + got := RenderFrontmatter("T", "", "", "1", "", []string{"a,b"}) + if !strings.Contains(got, `"a,b"`) { + t.Errorf("RenderFrontmatter = %q, want the comma-bearing label quoted", got) + } +} diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index 64cddea..466f89a 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -364,7 +364,10 @@ "readResult": { "type": "object", "additionalProperties": false, - "required": ["ok", "page_id", "title", "space", "parent", "parent_type", "page_width", "format", "body"], + "required": [ + "ok", "page_id", "title", "space", "parent", "parent_type", "page_width", "labels", + "format", "body" + ], "properties": { "ok": { "const": true }, "page_id": { "type": "string" }, @@ -373,6 +376,10 @@ "parent": { "$ref": "#/$defs/stringOrNull" }, "parent_type": { "$ref": "#/$defs/parentTypeOrNull" }, "page_width": { "$ref": "#/$defs/pageWidthOrNull" }, + "labels": { + "description": "The managed labels that went into the rendered frontmatter, sorted; null when the fetch failed. A different shape from info's labels on purpose: info describes the page, read describes the document it produced.", + "oneOf": [{ "type": "array", "items": { "type": "string" } }, { "type": "null" }] + }, "format": { "enum": ["markdown", "storage"] }, "body": { "type": "string" } } From 88aa437a7623f7f386fd465439dcf48cfcdb4d5b Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Fri, 11 Sep 2026 21:09:27 -0400 Subject: [PATCH 12/16] docs: labels in the README, guarantees, and CLAUDE.md README: a `labels` row in the frontmatter table, the assert/absent asymmetry in the `update` and `fix` sections, `labels` in what `read`/`export` emit and what `check` validates, and a note on the existing `--cql 'label = "runbook"'` example -- that search only pays off for pages someone labeled, which is the gap this closes. The frontmatter description no longer says "no lists". guarantees.md gets **L9** (`declared-metadata-is-asserted`), status **Partial**, and the gap is `page_width` rather than `labels`: `update` honours it for both, but `create` asserts a default width of `max` for a file that declares none, so an omitted field is not left alone there. `labels` holds in both verbs. `fix` is outside the law rather than violating it -- it reconciles the file to the page, so an absent field is filled in, which is the only way to adopt a page labeled by hand. C2's wording now admits sequences, and its prose records what extending the writer's self-check cost, because that was the closest thing to a repeat of #130: the scalar check verifies a value as a *mapping* value, and `x,y` and `has]bracket` pass it and then split or end a flow sequence. Flow turns out to be the stricter context, so verifying there is always safe and the style parameter buys minimal quoting -- the corrupting direction is a block check standing in for a flow write. Also corrects two claims the probes disproved: a continued plain scalar folds onto one line rather than returning as a `|-` block, and `dropBlankLines`' safety no longer rests on "nothing spans more than one line", since a passed-through block list does. --- CLAUDE.md | 5 ++-- README.md | 39 ++++++++++++++++++-------- docs/guarantees.md | 70 +++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 93 insertions(+), 21 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 17862da..9332aec 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -66,7 +66,8 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `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`. **Two error types on the request path, and one predicate for them**: an `*HTTPError` once a response has a status, an unexported `requestError` when there is none (a transport failure, a request that would not build, a body that would not decode), and `FromRequest` answers whether an error is either. That is what lets a caller tell a server failure from a local one — `jsonout.CodeOr(err, fallback)` is the whole point of it, since `CodeFor` alone reports every non-`HTTPError` as `NETWORK` and so turns `no title given` into a network problem (#133). The rule is deliberately scoped to the request: `DownloadAttachment` writing to the caller's writer, `uploadAttachment` opening the caller's file, and `Resolve` reading the environment stay untyped, because tagging them would misreport an unreadable file as a network failure. The wrapper carries no message of its own, so `Error()` is the inner text verbatim and nothing a reader sees changed. 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, plus the **`.env` permission warning** (#136): a `.env` reachable by anyone but its owner (`mode.Perm()&0o077`) *and* containing `CONFLUENCE_TOKEN` earns a warning naming the file, its mode, and the `chmod`. Both halves matter — a `.env` holding only the URL and username leaks nothing, and a warning that fires on a file with no secret in it is how one becomes something people scroll past. It stats rather than lstats (a link's own `0777` would cry wolf over a `0600` target), lives in `loadDotenv` because that is the one function both the discovered `.env` and `--env-file` pass through, and reaches the reader through `SetSecurityWarner` for the same reason `SetRetryLogger` exists — wired to `cmd/root.go`'s `reportSecurityWarning`, which prints it (human mode) *and* records it via `jsonout.AddWarning`, since stderr under `--json` is a schema-validated document with no room for a stray line. A group/world-*writable* `.env` with no token in it is knowingly **not** covered, though `CONFLUENCE_URL` resolves from there too and rewriting it would redirect the token: see #136. 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, which is now the path's **base name** and nothing else (#59/`_plans/029`): the name is the attachment's identity, so an encoded path moved the name every time the file moved and orphaned the old attachment, and the path is recorded in the comment anyway. The mapping is therefore lossy, and what the bijection used to buy is an explicit refusal — two assets in one document whose base names agree return a typed `NameCollisionError` from `MdToConfluence`, which is a *failure* and not a `Broken` entry, since nothing blocks a publish on `Broken`. `check` catches that error and reports it as `Broken` anyway, because there it is a document defect like a dead link rather than a converter failure. A stored name is never interpreted in the other direction either: `sourceFor` reads the recorded path or uses the name verbatim. 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 `