From e1713b5e0e32b68c3369f68ed704762d2492f5ee Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:28:07 -0400 Subject: [PATCH 1/5] docs: plan for making frontmatter real YAML Replace the hand-rolled flat-key parser in internal/frontmatter with goccy/go-yaml, so the frontmatter markfluence writes is valid YAML. The bug (#130): renderValue decided whether to quote by asking "would our own ParseValue round-trip this?", not "is this valid YAML?". A colon round-trips through our first-colon split, so a title containing ": " was written bare and no other tool could read the file. The colon is one member of a class that also includes booleans, numbers, nulls, flow collections and the reserved indicators. The plan records what was probed rather than assumed: goccy's quoting, comment round-tripping, the token-vs-String() read gotcha, null identified by node type rather than token text, and four shapes goccy emits wrongly. One of them, a title starting "? ", makes goccy refuse to parse its own output -- so the writer verifies and retries rather than trusting the serializer, which is what lets C2 claim more than "goccy is correct". Field-order normalization was designed here and split out to _plans/033: it is a separate feature that merely needs a surgical UpdateField, it carries the only --json contract change, and keeping it here made a ~400-line change into a ~600-line one. The decisions it rests on stay recorded and marked, since 033 builds on them. Adds C2 (frontmatter-is-valid-yaml) to docs/guarantees.md. --- _plans/032_frontmatter-yaml.md | 573 +++++++++++++++++++++++++++++++++ 1 file changed, 573 insertions(+) create mode 100644 _plans/032_frontmatter-yaml.md diff --git a/_plans/032_frontmatter-yaml.md b/_plans/032_frontmatter-yaml.md new file mode 100644 index 0000000..f6b0090 --- /dev/null +++ b/_plans/032_frontmatter-yaml.md @@ -0,0 +1,573 @@ +# Plan: frontmatter is real YAML + +Replace the hand-rolled frontmatter parser with `goccy/go-yaml`, so the +frontmatter markfluence writes is valid YAML. Closes #130. Adds **C2** +(`frontmatter-is-valid-yaml`) to `docs/guarantees.md`. + +Field-order normalization (`Normalize`, `fix --json`'s `reordered`, and +`create --persist` normalizing) was designed here and then **split out** into +`_plans/033`: it is a separate feature that happens to need a surgical +`UpdateField`, it is the only `--json` contract change, and keeping it here made +a ~400-line change into a ~600-line one. The decisions below that concern it are +kept for the record and marked, since 033 builds on them. + +## Current state of the codebase + +`internal/frontmatter` is a 303-line hand-rolled parser for a flat `key: value` +block. It is self-consistent and wrong. + +**The bug.** A page title containing `: ` is written unquoted: + +``` +--- +title: Deploy Runbook: Part 2 +page_id: 123 +--- +``` + +`Extract` splits each line at the *first* `:` only, so that reads back as +`Deploy Runbook: Part 2` for us. Every real YAML parser rejects it — +`mapping value is not allowed in this context`. VSCode's YAML extension is what +surfaced it. + +**Why it happens.** `renderValue` (`frontmatter.go:135`) decides whether to +quote by asking *"would our own `ParseValue` round-trip this?"*, not *"is this +valid YAML?"*. A colon round-trips through our parser, so it is never quoted. +The current output is pinned by a test — `frontmatter_test.go:80`, +`{"colon value bare", "title", "a: b", "", "title: a: b"}` — so this is a +deliberate decision being reversed, not an oversight. + +**The colon is one member of a class.** The same predicate emits all of these +bare (measured, not guessed): + +| written | a real YAML parser sees | +|---|---| +| `title: a: b` | parse error | +| `title: true` / `no` | boolean | +| `title: 123` / `1.5` | number | +| `title: null` / `~` | null | +| `title: [draft] Foo` / `{a}` | flow collection | +| `title: @home` `*star` `&anchor` `%pct` `- dash` `\|pipe` `>gt` `!bang` | reserved or misparsed indicators | + +`parent` has the same exposure, since it can hold a relative `.md` path. + +**What already exists and constrains the change:** + +- `MarkdownFile.Frontmatter` is a `map[string]string`, read directly by + `pagewidth.Declared`, `fix.locatePage`, `fix.plannedChanges`, + `create.go:570`, `update.go:323`. Keeping that shape keeps the blast radius + inside the package. +- `coordinate()` maps a literal `"null"` to `""`, which is how `parent: null` + and `page_id: null` mean "unset". It does **not** map `~` or `Null`. +- `Title()` deliberately does *not* collapse `"null"` — "a literal `null` is a + legal title", pinned by `frontmatter_test.go:171`. +- `UpdateField(content, key, value, comment)` is the single write path: + `create.go:451-455`, `fix.go:144`, and `pagedoc.RenderFrontmatter` + (`pagedoc.go:284-298`), which chains five calls starting from `""`. +- The `comment` parameter is **write-only**. `create` emits + `parent: 1234 # original.md` as a human breadcrumb; nothing reads it back + (`frontmatter_test.go:107`). +- `fieldOrder` = `title, space, parent, page_id`, then the rest alphabetically. + `UpdateField` rewrites the whole block in that order on every write. +- `ErrUnterminatedFrontmatter` is a lexical check on the `---` delimiters + (`frontmatter.go:261-264`), before any value parsing. +- `Extract` and `ParseValue` are exported but called only by this package's own + tests. +- `check.go:120` routes any `ParseFile` error to `r.fail(err, CodeValidation)`. +- `linkindex.Build` skips a file whose frontmatter fails to parse. +- `create.go:554` **already errors** on an empty title. `update.go:174-175` + deliberately falls back to the live page title instead. +- `internal/convert`'s `lineOffset` already exists because goldmark parses the + body rather than the file, so reported positions need correcting. Frontmatter + positions have the same problem and should be reported the same way. + +## What was verified (2026-09-06) + +Probed `github.com/goccy/go-yaml v1.19.2` in a throwaway module. Everything +below is measured output. + +1. **Quoting is correct for the whole hazard set.** `title: "a: b"`, `"true"`, + `"123"`, `"null"`, `"~"`, `"[draft] Foo"`, `"@home"`, `"*star"`, `"- dash"`, + `"%pct"`, `"Detect # Verify"` — and `Plain Title` / `Bob's Runbook` stay + bare. Quotes only when YAML requires it; prefers **double**. +2. **No line wrapping.** A 309-character plain scalar emits as one line. +3. **Comments round-trip.** `parser.ParseBytes(src, parser.ParseComments)` then + `MappingNode.String()` reproduces a full-line comment and a trailing + `# foo.md`. One space before the `#`, where we currently write two. +4. **Reads must go through the token, not `String()`.** + `v.Value.GetToken().Value` gives `"4"` for `parent: 4 # foo.md`; + `v.Value.String()` gives `"4 # foo.md"`. +5. **Node type, not token text, is what identifies a null.** This corrects an + earlier draft of this plan, which claimed token values reproduce today's map + exactly. They do for `page_id: 123` → `"123"` and `space: ~abc` → `"~abc"`, + but not for nulls: + + ``` + title: -> NullNode tok="null" (today: "") + title: null -> NullNode tok="null" + title: ~ -> NullNode tok="~" (coordinate() never mapped this) + title: Null -> NullNode tok="Null" (nor this) + ``` + + So a blank `title:` would read as the string `"null"` and `create` would + publish a page named `null`. The `~`/`Null` gap exists in the current code + too: `parent: ~` reads today as if it were a page id. +6. **`GetToken().Value` returns the indicator for four node kinds**, so + `scalarValue` must be a whitelist, not a Sequence/Mapping blacklist: + + ``` + title: &a foo -> AnchorNode tok="&" + parent: *a -> AliasNode tok="*" + page_id: !!str 123 -> TagNode tok="!!str" + title: |\n lit -> LiteralNode tok="|" + ``` +7. **Stricter than us in four places we want.** A space before the colon + (`key :v`) fails with `non-map value is specified`; duplicate keys error + (`mapping key "title" already defined at [1:1]`) where we silently take the + last; tab indentation errors; a nested value parses as a + `SequenceNode`/`MappingNode` where we silently produced `""`. +8. **An empty block is not an error, but has no mapping node.** + + ``` + "" -> docs=1 body=nil + "\n" -> docs=1 body=nil + "# only a comment" -> docs=1 body=*ast.CommentGroupNode + ``` + + Today `---\n\n---` parses to an empty map, so `parseBlock` must treat both + as an empty mapping rather than failing. +9. **Editing the AST works, with two traps.** A hand-built + `MappingValueNode` whose `token.Position.Column` is 0 **panics** in + `String()` (`ast.go:1438`, `strings: negative Repeat count`); `Column >= 1` + is required. And a comment set on the `MappingValueNode` renders as a + full-line head comment; it must go on the *value* node to render as + `parent: "42" # foo.md`. +10. **Reordering carries comments *and* blank lines with their key**, producing + stray blanks in meaningless positions. Blank lines are not nodes — they + live in the preceding value token's `Origin` — so removing them is a + textual filter over the emitted block, not an AST operation. +11. **goccy's default emission is not round-trip-safe for four shapes.** Probed + ~80 strings; these are the ones that fail, and the failure modes differ: + + ``` + "a\tb" -> title: ab -> reads back "ab" (silent loss) + "? q" -> title: ? q -> goccy REFUSES its own output + ".inf" -> title: .inf -> InfinityNode, not a string + ".nan" -> title: .nan -> NanNode, not a string + ``` + + The last two are why the verify step compares the re-read **node kind** and + not just its text: `scalarValue` flattens every scalar to its token, so + comparing text says `.inf` round-trips. It does, *for us* -- and the file + still says "float" to every other reader. + + `? q` is the #130 class recurring: a file we write and then cannot read. + Everything else in the set quotes correctly, including `0x1f`, `12:30`, + `y`, `On`, `2026-09-06`, ` lead`, and `""`. +12. **Forcing `token.DoubleQuoteType` round-trips everything.** 15/15 — + `? q`, `.inf`, `.nan`, `-.inf`, tab, newline, `a: b`, `true`, embedded + `"` and `\`, `é`, the empty string, and leading whitespace all come back + byte-identical. This is what makes verify-and-retry a complete strategy + rather than a partial one. +13. **Error text is multi-line by default and off by one.** + + ``` + [1:8] mapping value is not allowed in this context + > 1 | title: a: b + ^ + 2 | page_id: 9 + ``` + + `yaml.FormatError(err, false, false)` gives the one-liner. The line is + relative to the block, so file line = block line + 1. +14. **Every `.md` in this repo already passes.** 15 files with frontmatter, 0 + fail a real YAML parse. Nothing in testdata depends on our leniency. + +## Decisions + +**Swap the parser, do not widen the predicate.** Patching `renderValue` fixes +only files we write, and the class is bigger than the colon. Hand-maintaining a +YAML plain-scalar rule set in a package with no YAML parser to check itself +against is how the version that forgets `%` ships. + +**`goccy/go-yaml`, not `gopkg.in/yaml.v3`.** Both clear every hurdle; goccy is +maintained and yaml.v3 is archived at v3.0.1 (2022). + +**A file that no longer parses is a hard error.** No lenient fallback to the +first-colon split — that means shipping both parsers forever and keeps +producing the invalid files this change exists to eliminate. No repair path in +`fix` either: `fix` reconciles *from the live page* and needs `page_id` to do +it, which it cannot read out of a file it cannot parse. markfluence is +unreleased, so the only affected files are already on a developer's disk. + +**A non-scalar value is a hard parse error**, naming the key. Not really a new +failure: `title:\n - a` yields `""` today, and `create` then rejects it as "no +title" — an error naming a symptom instead of the cause. A hand-authored `|` +block is a `LiteralNode` and gets the same error: the documented contract is +flat `key: value` with no multi-line values, and our writer never produces one. + +**Nulls are unset, uniformly, and identified by node type.** `NullNode` reads +as `""` for every field, whatever its spelling. This drops the "a literal +`null` is a legal title" rule, and loses nothing: goccy emits the *string* +`null` as `"null"` (quoted), so a page genuinely titled `null` still round-trips +through `pagedoc.RenderFrontmatter`. It also fixes the existing `parent: ~` / +`parent: Null` gap. + +**An empty block is an empty mapping, not an error.** A `nil` body and a +`CommentGroupNode` body both become an empty mapping. Nothing about +`---\n\n---` is invalid YAML. + +**Typing on the write path is ours; quoting is goccy's.** `yaml.ValueToNode("123")` +emits `"123"` and `ValueToNode("null")` emits `"null"` — both quoted, both +wrong. So `UpdateField` types by key: for `page_id` and `parent`, digits become +an `IntegerNode` and `"null"` becomes a `NullNode`; everything else is a string. +This *is* a hand-rolled predicate in a plan that argues against them, and the +distinction being drawn is that goccy still owns quoting — the thing #130 is +about — while we own typing for two fields whose value domains we already parse +elsewhere (`pageref.IsDigits`, `coordinate`). C2's justification is worded +accordingly. + +**Replace the value node; never mutate its token.** Mutating a plain token's +`Value` re-emits it unquoted, which reintroduces #130 through the back door. +Node replacement also drops a stale line comment, which is what `fix` wants +when it overwrites `parent: 4 # foo.md` with a live id. + +**A present-but-null coordinate equals a live null.** Once `NullNode` reads as +`""`, `fix.plannedChanges` (`fix.go:208-214`) takes its `!present || blank` +branch for `parent: null`, plans `parent: (none) -> null`, writes it, and reads +`""` again -- a correct top-level page reported `changed` forever. The first +branch narrows to `!present` so a present-but-blank value falls through to the +`norm` comparison, where `norm("null") == norm("") == ""`. The `"(none)"` +display is preserved by an explicit blank check on the old value. +`TestPlannedChangesParentNullNormalizes` (`fix_test.go:158`) exists to guard +exactly this and would have kept passing: it feeds `{"parent": "null"}`, a map +the new parser can never produce. It is rewritten to feed `""`. + +**A present-but-empty `title` is an error in `update`.** `create` already +errors on any empty title, absent or present (`create.go:554`), and needs no +change. `update` currently falls back to the live page title for *any* empty +title; that stays for an **absent** `title` key — a positive statement that the +file does not manage the title, and the shape `fix.go:217` already reasons +about — and becomes an error for a **present-but-empty** one (`title:`, +`title: null`, `title: ""`), which is a typo that should not silently publish +under the live title. Needs a `MarkdownFile` accessor that distinguishes the +two; the map already carries it, and the `MarkdownFile` doc comment +(`frontmatter.go:231-233`) says it is exported for exactly this. + +**Reads refuse a multi-line scalar, and a second document.** The flat contract +has to be enforced rather than assumed. 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 continued plain scalar comes back as a `|-` block that the parser +then rejects, so a write would produce a file markfluence cannot read -- in +`create`, only after the page was made. A multi-line single-quoted scalar is +worse, re-emitting on one line and silently turning `"sq\nline"` into +`"sq line"`. Detected on the value token's origin with trailing whitespace +stripped, since a token's origin runs up to the next one; a newline markfluence +wrote is a two-character escape inside a double-quoted scalar and occupies one +physical line, so it is unaffected. A `...` line starts a second document, and +reading only the first would drop every later key in silence. + +**The writer verifies its own output.** goccy's default emission is wrong for +at least four shapes (verified items 11-12), and any predicate we wrote to +catch them would be incomplete -- those three non-tab cases turned up only by +probing ~80 strings, after two review rounds. So the writer does not *predict* +which values goccy mishandles, it **checks**: emit with goccy's default, +re-parse, compare; on mismatch re-emit with `token.DoubleQuoteType`, which +round-trips all 15 probed cases. If even that fails, return a typed error +rather than write a value known to be wrong -- unreachable in probing, which is +exactly what makes the fuzz target worth having. + +This is the one place the plan overrides goccy, and the justification is +different from the two style overrides it declines: those were cosmetic, this +is a correctness bug that produces either silent data loss or an unreadable +file. It also upgrades C2 from "goccy is correct" to "the writer verifies its +own output", which holds against goccy regressions rather than only today's +bugs. Cost is one extra parse per value written -- at most five per file, +each over a block of under ten lines. + +**`UpdateField` becomes surgical -- at node granularity, not text.** Worth +being precise, because "minimum diff" oversells it: re-emitting normalizes +intra-line whitespace (`title: Spaced Out` becomes `title: Spaced Out`), +drops a trailing blank line before the closing `---`, and moves an interior +blank line, since a blank lives in the *preceding* value's `Origin`. Quote +style on untouched keys is preserved. This is much less churn than today's +whole-block rewrite, but it is not zero, and a test pins what actually +survives rather than claiming everything does. + +Change the touched key's value node in place; insert a *new* key **before the first existing key that sorts after it** +(well-defined even in a jumbled block); never move an existing key. Minimum +diff on a file that lives in git, and it makes the comment-travel problem +disappear on the common path. + +**Editing and building are separate entry points.** `UpdateField` parses, so it +returns `(string, error)`; only `create.go:451-455` and `fix.go:144` call it and +both already have error paths. `frontmatter.Render(fields) string` builds a +block from scratch and **cannot fail** — it constructs nodes and never parses — +so `pagedoc.RenderFrontmatter` keeps its signature and nothing ripples into +`read` or `export`. It also stops re-parsing and re-emitting a five-field block +five times. CLAUDE.md's "read and export cannot drift" concern is real, so a +test pins that `Render` and repeated `UpdateField` agree. + +**Field order is normalized by `fix` and `create`, not by every write.** *(Deferred to `_plans/033`.)* Stable +ordering matters; doing it as a side effect of writing one field does not. +`fix` normalizes by default with no flag. `create --persist` normalizes too — +the minimal-diff argument behind a surgical `UpdateField` is about not churning +lines the caller did not ask to touch, and `create --persist` already writes all +five fields by definition. `update` stays out of it: it never writes back. +README:339 and CLAUDE.md both currently say `fix` "writes a file only when a +field actually changed" and must be updated rather than treated as a +constraint — that sentence describes today's behavior, not a guarantee in +`docs/guarantees.md`. + +**A reorder-only file is `changed`, not `consistent`.** *(Deferred to `_plans/033`.)* Otherwise you run `fix`, +see `consistent`, and still have a jumbled file. Keeping `consistent` honest +also keeps `--dry-run` a faithful preview. + +*(Deferred to `_plans/033`.)* **Reported as a dedicated `reordered: boolean` on `fixResult`,** not a +pseudo-entry in `changes[]`. `changes[].field` is an actual key name everywhere +else; a non-field there makes the slot polymorphic. The existing `"(none)"` +sentinel lives in `old`, explicitly a *display* slot (`oldDisplay`) — `field` is +an identity slot. No equivalent on `createResult`: `create` writes all five +fields, so its output is always canonical. + +*(Deferred to `_plans/033`.)* **`Normalize` is a no-op when the order is already canonical.** Blank lines die +only as a consequence of an actual reorder, so `reordered` means exactly "keys +moved" and the blank-line behavior follows from a property of the file rather +than from whatever else `fix` was doing that run. A canonical file with blank +lines keeps them: `fix` normalizes ordering, it is not a formatter. + +*(Deferred to `_plans/033`.)* **When it does reorder, comments travel with their key.** Better than today's +hoist-to-top — a comment about `page_id` belongs next to `page_id`. Not claimed +to be free: a header comment written above a key that is not `title` sinks with +it, which is visible in the diff and accepted. + +**Take goccy's quote style.** `Bob's Guide: Part 2` is a realistic title, and +`"Bob's Guide: Part 2"` beats `'Bob''s Guide: Part 2'`, whose doubled quote +reads as a typo. Forcing single quotes means hand-setting `token.SingleQuoteType` +and an `Origin` on every write. + +**Quote only when needed.** The original proposal was to always quote `title`. +Its correctness argument is gone — it existed because we could not trust +ourselves to detect the hazards, and now a serializer does. + +**Parse errors are one-liners with our own position prefix.** `yaml.FormatError(err, +false, false)`, wrapped as `filename:line:col`, with the line corrected by +1 +for the `---` opener. goccy's default four-line source art would land verbatim +in `check --json`'s `error` string field, and every line number in it is wrong +by one. Matches `internal/convert`'s existing `line %d: ` reporting, which has +`lineOffset` for the same reason. + +**C2 verification is manual and issue-driven.** No second YAML implementation in +`go.mod`. If a real tool reports a divergence, that is an issue with the +offending frontmatter attached. + +**There is still a write-path round-trip test**, and it is not testing goccy — +it guards that we still go *through* goccy. The failure it catches is someone +later adding a fast path that string-concatenates a frontmatter line, which is +how the current bug exists. Same reasoning as `internal/schematest` validating +against the embed. + +## Implementation + +### `go.mod` + +Add `github.com/goccy/go-yaml v1.19.2`. + +### `internal/frontmatter` + +**Deleted:** `ParseValue`, `scanQuoted`, `stripInlineComment`, `quoteValue`, +`renderValue`, `splitFrontmatter`, `inlineCommentRE`, `Extract`. `Extract` and +`ParseValue` are exported but called only by this package's own tests; +`ParseValue` is a hand-parser concept that would be misleading to keep. + +**Kept unchanged:** `frontmatterRE` and the `ErrUnterminatedFrontmatter` +pre-check — lexical, about the `---` delimiters, and goccy never sees it, so the +two error kinds cannot collide. `MarkdownFile` and `coordinate()`. + +**New internals:** + +- `parseBlock(fmText string) (*ast.MappingNode, error)` — `parser.ParseBytes` + with `parser.ParseComments`. A `nil` or `CommentGroupNode` body returns an + empty mapping; a `CommentGroupNode`'s comment is carried onto the first key + later inserted, so a `---\n# note\n---` block does not silently lose it. Any + other body kind (`StringNode` for `just text`, `SequenceNode` for `- a`) is a + typed error. Errors go through `yaml.FormatError(err, false, false)` with the + line offset applied. Known limit: goccy's duplicate-key message embeds a + second position (`already defined at [1:1]`) which stays block-relative. +- `scalarValue(key string, n ast.Node) (string, error)` — a **whitelist**: + `StringNode`, `IntegerNode`, `FloatNode`, `BoolNode` → `GetToken().Value`; + `NullNode` → `""` regardless of spelling; everything else (`Sequence`, + `Mapping`, `Anchor`, `Alias`, `Tag`, `Literal`) is a typed error naming the + key. +- `toMap(m *ast.MappingNode) (map[string]string, error)`. +- `keyLess(a, b string) bool` — the canonical comparator from `fieldOrder`. The + single source of ordering, shared by insertion and `Normalize`. +- `valueNode(key, value string) ast.Node` — the typing rule. For `page_id` and + `parent`: `pageref.IsDigits` → integer, `"null"` → null. Otherwise string. + Positions built with `Column: 1`. + +**Changed:** + +- `Parse` — lexical `---` check, then `parseBlock`, then `toMap`. Wraps the + goccy error with the filename and the corrected line. +- `UpdateField(content, key, value, comment string) (string, error)` — + surgical. Replace the value node if the key exists, else insert before the + first key that sorts after it. Comment goes on the **value** node. Content + with no frontmatter block gets one created, as today (`frontmatter.go:164-166`); + `create --persist` on a flag-only file depends on it. Emit via + `MappingNode.String()`, then verify-and-retry the value. + +**New exported:** + +- `Render(fields []Field) string` — build a block from scratch. Cannot fail. +- `Normalize(content string) (string, bool, error)` — if `keyLess` order already + holds, return unchanged with `false`; likewise for content with no block. + Otherwise reorder, drop blank lines textually, return `true`. +- A `MarkdownFile` accessor distinguishing an absent `title` from a + present-but-empty one. + +### `cmd/fix` + +Only the call-site change `UpdateField`'s new error return forces, plus the +`plannedChanges` fix above. Normalization is `_plans/033`. + +### `cmd/create` + +`writeBackFrontmatter` collects the five `UpdateField` calls +(`create.go:451-455`) so one error path covers them; a failure routes through +`failKeepingPage`, since the page already exists by then. `create.go:554`'s +empty-title error is unchanged. Normalization is `_plans/033`. + +### `cmd/update` + +`resolveTitlePageID` gains a third return distinguishing an absent `title` key +from a present-but-empty one -- it cannot express that with two strings. The +error fires in `processFile` **before** `GetPageOrNil` (`update.go:167`), +matching the `IsDigits` pre-flight at `:155`: a local validation failure should +not cost a request. `--title` still wins, as every other override does, so +`--title X` against a present-but-empty frontmatter title succeeds. The +live-title fallback at `:174-175` stays for an absent key. + +### `internal/pagedoc` + +`RenderFrontmatter` builds a `[]Field` and calls `frontmatter.Render` once +instead of chaining five `UpdateField` calls. Signature unchanged. + +### `schema/json-output/v1.json` + +`checkResult`'s description at `:436` enumerates the `failed` causes -- +"unterminated frontmatter, bad page_width, non-numeric page_id" -- and is now +incomplete. `fixResult` gaining `reordered` is `_plans/033`. + +### Not changed + +`cmd/check` — `check.go:120` already routes any `ParseFile` error to +`CodeValidation`/`status: failed`. `check` does not flag jumbled field order: +ordering is not a publishability defect. `internal/linkindex` already skips a +file whose frontmatter fails to parse. + +## Tests + +- **Write-path round trip** (the C2 guard): table over the printable hazard set + — `UpdateField` writes it, `parser.ParseBytes` re-reads from scratch, assert + identical. No fuzz target, no control characters. +- **`Render` and `UpdateField` agree** on the same field set — the anti-drift + pin for the two entry points. +- **Typing**: `page_id`/`parent` emit bare `123` and `null`; a *title* of `123` + or `null` emits quoted. +- **Null spellings**: `title:`, `title: null`, `title: ~`, `title: Null` all + read as `""`; `parent: ~` no longer reads as an id. +- **Reads reproduce today's map** for `page_id: 123` → `"123"` and + `space: ~abc` → `"~abc"`. +- **New hard errors**: nested value, `|` block, anchor, alias, tag, duplicate + key, tab indentation — each naming the key. An unterminated block still + reports `ErrUnterminatedFrontmatter`, not a goccy error. Error text is a + single line with a file-accurate position. +- **Empty block** (`---\n\n---` and a comment-only block) parses to an empty map. +- **Surgical `UpdateField`**: existing key keeps position; new key inserts + before the first key sorting after it; blank lines and comments elsewhere + survive *modulo* the emitter's own normalization (intra-line whitespace, + trailing blank line, interior blank position) -- pinned explicitly; the + `parent` comment renders inline and the value round-trips without it; a + hand-built node does not panic; a comment-only block's note survives the + first insert. +- **`cmd/fix`**: a canonical top-level page with `parent: null` is + `consistent` and converges -- the regression 1a would have caused, with + `TestPlannedChangesParentNullNormalizes` rewritten to feed `""` rather than + `"null"`, which the new parser can never produce; a reorder-only file is + `changed` with `reordered: true` and is written; `--dry-run` reports without writing; canonical consistent file still + `consistent`. +- **`cmd/update`**: present-but-empty title errors before any request; absent + title still falls back to the live page title; `--title X` wins over a + present-but-empty frontmatter title. +- **`cmd/check`**: present-but-empty title is `broken`; absent title is not. + +**Existing tests that change** (not an exhaustive diff, but the ones known now): +`frontmatter_test.go:80-83` (quote style, `4 #` → `4 #`), `:107`, `:117` +(surgical no longer reorders), `:129` (surgical keeps blanks), `:171` (literal +`null` title); `pagedoc_test.go:13,22,30,39`; `fix_test.go:279` (jumbled +fixture flips to `changed`), `:328`. + +## Docs + +- `docs/guarantees.md` — add **C2** `frontmatter-is-valid-yaml`, status + **Holds**, enforced by the writer verifying its own output rather than by + trusting the serializer -- goccy owns quoting, we own typing for `page_id` + and `parent`, and the verify-and-retry loop is what makes the guarantee hold + regardless of either. The entry names the one gap honestly: the check is that + *goccy* can re-read what goccy wrote, not that another implementation can. Add a verification-table row: + review judgement. One sentence relating C2 to **L7** — C2 is the YAML half, + split out because it is checked against a different external spec. +- `README.md` — the frontmatter section (~1057-1093): the block is YAML, values + are quoted when YAML requires it, flat-key is now enforced rather than + assumed, and what an author can no longer write bare. The `fix` section + (:334-340): drop "writes a file only when a field actually changed", add + order normalization. +- `CLAUDE.md` — the `internal/frontmatter` bullet; the `fix` sentence; the + `check` bullet, whose "deliberately narrow" list is now incomplete (duplicate + keys, tabs, nested values, reserved indicators all fail too) and whose + create-vs-update justification no longer covers `title`. + +## Commits + +1. `build: add goccy/go-yaml` +2. `refactor(frontmatter): read and write the block with goccy` -- the reader + and writer swap in **one** commit. Split, commit 2 leaves the old + `renderValue` writing `title: a: b` bare while the new reader rejects it, so + `TestWriteThenReadRoundTrips` (`frontmatter_test.go:95`) fails mid-series and + the per-commit `make check` rule breaks. +3. `feat(frontmatter): surgical UpdateField, Render, and Normalize` +4. `fix(fix): a present-but-null coordinate matches a live null` +5. `fix(update): error on a present-but-empty title` +6. `feat(check): report a present-but-empty title as broken` +7. `test(frontmatter): pin that every write verifies its own output` +8. `docs: C2 (frontmatter-is-valid-yaml), README, CLAUDE.md` + +## Consequences found during implementation + +- **`page_width: null` was an error and is now "unset".** It used to reach + `pagewidth.Declared` as the string `"null"`, which is not in the width + vocabulary. Every null spelling now reads as `""`, so it means "not set" and + the default applies -- and `check` no longer reports it. Follows from the + uniform null rule and is the better behaviour, but it was a consequence rather + than a decision. +- **A `create --persist` write can now fail on frontmatter.** `UpdateField` + returns an error, and it arrives after `CreatePage`. Routed through + `failKeepingPage`, the same path the existing `os.WriteFile` failure uses, so + the page id survives in the result rather than becoming an orphan. +- **The commit split in the plan was not achievable.** `UpdateField` gaining an + error return forces every call site into the same commit as the package, so + the reader/writer swap, the `plannedChanges` convergence fix, and the `update` + title check land together -- the tests do not pass otherwise. + +## Out of scope + +- **A tab in a value under goccy's own default emission.** Fixed incidentally by + the double-quote fallback, but the *cross-parser* gap remains: `1e3` is a + string to goccy and a float to yaml.v3, and no self-check can see that. +- **#100** (`markfluence.yaml` project-wide settings) now has a YAML parser + available, but this change does not touch that file. +- **Field-order normalization**, split out to `_plans/033`. +- **A repair path for an unparseable file.** Circular, as argued above. +- **#38** (sidecar frontmatter) and **#21** (`layout:` directive) both get + easier after this; neither is in here. From fef45b5551a3d782d89c6d0d3c98ac1e818cc6bd Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:28:28 -0400 Subject: [PATCH 2/5] refactor(frontmatter)!: parse and emit the block as real YAML Closes #130. The hand-rolled parser split each line at the first ":", so it read its own `title: Deploy Runbook: Part 2` back perfectly while every real YAML parser rejected the file. renderValue decided quoting by asking "would our own ParseValue round-trip this?" rather than "is this valid YAML?", and the colon was one member of a class: booleans, numbers, nulls, flow collections and the reserved indicators were all written bare. goccy/go-yaml does the parsing and emitting now. Deleted: ParseValue, scanQuoted, stripInlineComment, quoteValue, renderValue, splitFrontmatter, Extract -- the last two exported but called only by this package's tests. The writer verifies its own output rather than trusting the serializer: emit with goccy's chosen style, re-read, fall back to a double-quoted scalar when the two disagree. That fallback is load-bearing. goccy drops a tab, emits a value beginning "? " as a document it then refuses to parse, and writes .inf/.nan bare where any conforming reader sees a float. A hand-written predicate listing those shapes would be incomplete; they turned up only by probing ~80 strings, so checking beats predicting. The check compares the re-read node *kind*, not just its text. Comparing text alone says ".inf" round-trips -- markfluence reads it back as ".inf" either way -- while the file still says "float" to every other tool. That is the same mistake renderValue made, one level down. Quoting is goccy's, typing is ours: page_id and parent are written as YAML integers and nulls, because `page_id: "123"` is valid YAML that says the wrong thing. Confined to two keys whose domains are closed -- a title of "123" is still a string. Reads enforce the flat contract instead of assuming it. A whitelist of scalar kinds, because an anchor, alias, tag or "|" block each reports its *indicator character* as its token value. A scalar whose source spans lines is refused: an untouched key is re-emitted from the node the parser produced, and goccy's re-emission is not identity, so `title: plain\n continued` came back as a "|-" block the parser then rejected -- writing a file markfluence cannot read, in create only after making the page. A multi-line single-quoted scalar was quieter and worse, re-emitting on one line and turning "sq\nline" into "sq line". A "..." line is refused too, since reading only the first document would drop every later key in silence. Every null spelling is unset. The old parser matched only the literal "null", so `parent: ~` read as though it were a page id -- a bug that predates this change. In fix, that made a present-but-blank coordinate take the "(none)" branch and plan `parent: (none) -> null` on every run, write it, and read "" again: a correct top-level page reported changed forever. plannedChanges now falls through to norm, which equates them. TestPlannedChangesParentNullNormalizes guarded exactly this and would have kept passing -- it fed {"parent": "null"}, a map the parser can no longer produce. Two entry points, deliberately separate. Render builds from scratch and cannot fail, keeping a parse error out of pagedoc/read/export, and a repeated key resolves last-wins rather than emitting a duplicate the parser would reject. UpdateField edits and returns an error, surgically: an existing key keeps its own key node, since a blank line before it lives in that node's token origin and swapping the whole pair would delete it. page_width: null was an invalid-width error and now means unset, since pagewidth.Declared sees "" rather than "null". A create --persist frontmatter failure routes through failKeepingPage, the path the existing os.WriteFile failure uses, so the new page is not orphaned. --- cmd/create/create.go | 28 +- cmd/create/create_test.go | 20 + cmd/fix/fix.go | 24 +- cmd/fix/fix_test.go | 22 +- go.mod | 1 + go.sum | 2 + internal/frontmatter/frontmatter.go | 619 +++++++++++++++-------- internal/frontmatter/frontmatter_test.go | 500 +++++++++++++----- internal/pagedoc/pagedoc.go | 17 +- internal/pagedoc/pagedoc_test.go | 2 +- 10 files changed, 872 insertions(+), 363 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 0ec55e1..1de0644 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -447,12 +447,10 @@ func reserveOne( if persist { parentValue, parentComment := parentField(r.parent, parentID) - content := r.mdfile.Content - content = frontmatter.UpdateField(content, "title", r.title, "") - content = frontmatter.UpdateField(content, "space", r.spaceKey, "") - content = frontmatter.UpdateField(content, "parent", parentValue, parentComment) - content = frontmatter.UpdateField(content, "page_id", pageID, "") - content = frontmatter.UpdateField(content, "page_width", string(r.width), "") + content, err := writeBackFrontmatter(r.mdfile.Content, r, pageID, parentValue, parentComment) + if err != nil { + return res.failKeepingPage(err, jsonout.CodeValidation), "", 0, false + } if err := os.WriteFile(r.filename, []byte(content), 0o644); err != nil { // The page above was already created; keep its id/url in the result or // it becomes an orphan with no local trace at all. @@ -786,6 +784,24 @@ func overrideNeedsSingleFile(cliTitle string, nFiles int) bool { return cliTitle != "" && nFiles != 1 } +// writeBackFrontmatter sets every field create persists. +func writeBackFrontmatter(content string, r record, pageID, parentValue, parentComment string) (string, error) { + fields := []struct{ key, value, comment string }{ + {"title", r.title, ""}, + {"space", r.spaceKey, ""}, + {"parent", parentValue, parentComment}, + {"page_id", pageID, ""}, + {"page_width", string(r.width), ""}, + } + var err error + for _, f := range fields { + if content, err = frontmatter.UpdateField(content, f.key, f.value, f.comment); err != nil { + return "", err + } + } + return content, nil +} + // resolveTitle returns the effective title: --title overrides the frontmatter. func resolveTitle(cliTitle string, mf *frontmatter.MarkdownFile) string { if cliTitle != "" { diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index 80ec5ce..7039414 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -409,3 +409,23 @@ func TestTopoSortOrdersParentsBeforeChildren(t *testing.T) { t.Errorf("order = %v, want parent before child before grandchild", got) } } + +// TestWriteBackFrontmatterQuotesAColonTitle is #130 at the layer that writes it. +func TestWriteBackFrontmatterQuotesAColonTitle(t *testing.T) { + r := record{title: "Deploy Runbook: Part 2", spaceKey: "ENG", width: pagewidth.Max} + + got, err := writeBackFrontmatter("---\ntitle: x\n---\nbody\n", r, "123", "null", "") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(got, `title: "Deploy Runbook: Part 2"`) { + t.Errorf("writeBackFrontmatter =\n%s\nwant the colon title quoted", got) + } + mf, err := frontmatter.Parse("f.md", got) + if err != nil { + t.Fatalf("wrote frontmatter it cannot read back: %v", err) + } + if mf.Title() != r.title { + t.Errorf("title round-tripped as %q, want %q", mf.Title(), r.title) + } +} diff --git a/cmd/fix/fix.go b/cmd/fix/fix.go index cc83154..d8e1c50 100644 --- a/cmd/fix/fix.go +++ b/cmd/fix/fix.go @@ -141,7 +141,10 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult { content := mf.Content for _, ch := range r.changes { - content = frontmatter.UpdateField(content, ch.field, ch.newValue, "") + var err error + if content, err = frontmatter.UpdateField(content, ch.field, ch.newValue, ""); err != nil { + return r.fail(err, jsonout.CodeValidation) + } } if err := os.WriteFile(filename, []byte(content), 0o644); err != nil { return r.fail(err, jsonout.CodeIO) @@ -207,10 +210,16 @@ func plannedChanges(fm map[string]string, page *client.Page, liveWidth string) [ } current, present := fm[lv.field] switch { - case !present || strings.TrimSpace(current) == "": + case !present: changes = append(changes, change{lv.field, "(none)", lv.value}) case norm(current) != norm(lv.value): - changes = append(changes, change{lv.field, current, lv.value}) + // A present-but-blank value goes through norm, not straight to + // "(none)": every null spelling now parses to "", so a top-level + // page's `parent: null` reads as "" and norm makes it equal to the + // 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}) } } @@ -244,6 +253,15 @@ func norm(value string) string { return t } +// orNone renders a frontmatter value for the "old" column, naming a blank as +// "(none)" the way an absent field is named. +func orNone(s string) string { + if strings.TrimSpace(s) == "" { + return "(none)" + } + return s +} + func orNull(s string) string { if s == "" { return "null" diff --git a/cmd/fix/fix_test.go b/cmd/fix/fix_test.go index 78f2c1b..c3a434e 100644 --- a/cmd/fix/fix_test.go +++ b/cmd/fix/fix_test.go @@ -156,9 +156,11 @@ func TestPlannedChangesUpdatesFieldsThatDiffer(t *testing.T) { } func TestPlannedChangesParentNullNormalizes(t *testing.T) { - // A top-level live page (no ParentID) already recorded as "null" must not be - // treated as a diff -- orNull("") and the frontmatter's "null" must compare equal. - fm := map[string]string{"parent": "null"} + // A top-level live page (no ParentID) already recorded as null must not be + // treated as a diff. The frontmatter side is "", not "null": every null + // spelling parses to "" now, so feeding "null" here would test a map the + // 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, "") for _, ch := range got { @@ -383,3 +385,17 @@ func TestOrNull(t *testing.T) { t.Errorf(`orNull("123") = %q, want "123"`, got) } } + +// TestProcessFileTopLevelPageConverges is the regression for a fix that planned +// `parent: (none) -> null` forever: a null parent parses to "", which the old +// present-but-blank branch read as "no value" and re-wrote on every run. +func TestProcessFileTopLevelPageConverges(t *testing.T) { + content := "---\ntitle: X\nspace: ENG\nparent: null\npage_id: 1\npage_width: max\n---\nbody\n" + path := writeFixture(t, content) + c := fixServer(t, pageJSON("1", "X", "", "/spaces/ENG/pages/1/X"), `"max"`) + + r := processFile(path, c) + if r.status != statusConsistent { + t.Fatalf("status = %q with changes %+v, want consistent", r.status, r.changes) + } +} diff --git a/go.mod b/go.mod index 31f3686..47372e3 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.25 require ( github.com/charmbracelet/lipgloss v1.1.0 + github.com/goccy/go-yaml v1.19.2 github.com/santhosh-tekuri/jsonschema/v6 v6.0.3 github.com/spf13/cobra v1.10.2 github.com/yuin/goldmark v1.8.5 diff --git a/go.sum b/go.sum index faa2caa..50ada4b 100644 --- a/go.sum +++ b/go.sum @@ -13,6 +13,8 @@ github.com/charmbracelet/x/term v0.2.1/go.mod h1:oQ4enTYFV7QN4m0i9mzHrViD7TQKvNE github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/dlclark/regexp2 v1.11.0 h1:G/nrcoOa7ZXlpoa/91N3X7mM3r8eIlMBBJZvsz/mxKI= github.com/dlclark/regexp2 v1.11.0/go.mod h1:DHkYz0B9wPfa6wondMfaivmHpzrQ3v9q8cnmRbL6yW8= +github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= +github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/lucasb-eyer/go-colorful v1.2.0 h1:1nnpGOrhyZZuNyfu1QjKiUICQ74+3FNCN69Aj6K7nkY= diff --git a/internal/frontmatter/frontmatter.go b/internal/frontmatter/frontmatter.go index 4f92319..7cb2831 100644 --- a/internal/frontmatter/frontmatter.go +++ b/internal/frontmatter/frontmatter.go @@ -1,238 +1,435 @@ -// Package frontmatter parses and rewrites the flat YAML frontmatter block that +// Package frontmatter parses and rewrites the YAML frontmatter block that // markfluence markdown files carry, and models a parsed file as a MarkdownFile. // -// It handles flat key: value pairs only -- no nested structures, lists, or -// multiline values -- with single/double-quote support and inline-`#`-comment -// stripping, plus surgical single-line write-back. +// 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. +// +// 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. package frontmatter import ( "errors" + "fmt" "os" "regexp" "sort" "strings" - "unicode" + + "github.com/goccy/go-yaml" + "github.com/goccy/go-yaml/ast" + "github.com/goccy/go-yaml/parser" + "github.com/goccy/go-yaml/token" ) // frontmatterRE matches a leading `---\n...\n---\n` block (DOTALL, non-greedy), // anchored at the start of the document. var frontmatterRE = regexp.MustCompile(`(?s)^---\n(.*?)\n---\n`) -// inlineCommentRE finds the first whitespace-then-`#` (a YAML inline comment). -var inlineCommentRE = regexp.MustCompile(`\s#`) +// ErrUnterminatedFrontmatter is returned by Parse/ParseFile when content opens +// with a "---\n" delimiter that never closes. This is a lexical check on the +// delimiters, made before any YAML parsing, so it cannot be confused with a +// goccy error about the block's contents. +// +// It is a lexical check, not a parse: a document whose very first line is a +// bare thematic break (a markdown horizontal rule) is indistinguishable from +// unterminated frontmatter and is flagged the same way. Accepted deliberately. +var ErrUnterminatedFrontmatter = errors.New( + `unterminated frontmatter block: starts with "---" but has no closing "---" line`) + +// fieldOrder is the canonical leading order of frontmatter keys; any key not +// listed here sorts after these, alphabetically. Every write that orders fields +// goes through keyLess, so this is the single source of frontmatter field order +// across all commands. +var fieldOrder = []string{"title", "space", "parent", "page_id"} -// Extract pulls the YAML frontmatter from content, returning the flat -// key->value map and the body (content with the frontmatter block removed). With -// no frontmatter it returns an empty map and content unchanged. +// typedFields are the keys whose value domain is not free text: a numeric id or +// the null that means "unset". They are written as YAML integers and nulls +// rather than strings, because `page_id: "123"` and `parent: "null"` are valid +// YAML that says the wrong thing. // -// Full-line `#` comments are skipped; a trailing inline `#` comment is stripped -// from each unquoted value; quoted values are read via ParseValue. -func Extract(content string) (map[string]string, string) { - return extractFrom(frontmatterRE.FindStringSubmatchIndex(content), content) -} +// Quoting is goccy's job; this typing is ours, and it is deliberately confined +// to two keys whose domains are closed. A title of "123" is still a string. +var typedFields = map[string]bool{"page_id": true, "parent": true} -// extractFrom is Extract's body, taking an already-computed match location so -// Parse can share the one frontmatterRE evaluation it also needs for -// ErrUnterminatedFrontmatter, rather than running the same regex over content -// a second time. -func extractFrom(loc []int, content string) (map[string]string, string) { - if loc == nil { - return map[string]string{}, content +// keyLess orders two frontmatter keys: fieldOrder first, in that order, then +// everything else alphabetically. +func keyLess(a, b string) bool { + ra, aok := fieldRank(a) + rb, bok := fieldRank(b) + switch { + case aok && bok: + return ra < rb + case aok != bok: + return aok + default: + return a < b } - fmText := content[loc[2]:loc[3]] - body := content[loc[1]:] +} - fm := map[string]string{} - for _, line := range strings.Split(fmText, "\n") { - line = strings.TrimSpace(line) - if line == "" || strings.HasPrefix(line, "#") { - continue - } - if i := strings.Index(line, ":"); i >= 0 { - key := strings.TrimSpace(line[:i]) - fm[key] = ParseValue(line[i+1:]) +func fieldRank(key string) (int, bool) { + for i, k := range fieldOrder { + if k == key { + return i, true } } - return fm, body + return 0, false } -// ParseValue parses a frontmatter value (the text after the first `:`). A value -// whose first non-space rune is `'` or `"` is read as a quoted string (inline -// `#` comments inside it are preserved); otherwise a trailing inline comment is -// stripped. An unterminated quote falls back to unquoted handling. -func ParseValue(raw string) string { - stripped := strings.TrimLeftFunc(raw, unicode.IsSpace) - if len(stripped) > 0 && (stripped[0] == '\'' || stripped[0] == '"') { - if v, ok := scanQuoted(stripped); ok { - return v - } - } - return stripInlineComment(raw) +// pos is the position every hand-built token carries. Column must be at least +// 1: MappingValueNode.String() indents by column-1 and panics on 0. +func pos() *token.Position { return &token.Position{Line: 1, Column: 1} } + +// block is a parsed frontmatter block: its mapping, plus any comment that had +// no key to attach to (a block holding nothing but a comment). The orphan is +// carried onto the first key later inserted rather than dropped. +type block struct { + mapping *ast.MappingNode + orphan *ast.CommentGroupNode } -// scanQuoted parses a leading quoted token from s (which starts with `'` or `"`). -// It returns the unquoted value, or ok=false if the quote is unterminated. Single -// quotes are literal with `”` -> `'`; double quotes honor `\"` and `\\` escapes. -// Anything after the closing quote is ignored. -func scanQuoted(s string) (string, bool) { - r := []rune(s) - quote := r[0] - var out []rune - for i := 1; i < len(r); { - c := r[i] - if quote == '\'' { - if c == '\'' { - if i+1 < len(r) && r[i+1] == '\'' { // doubled '' -> literal ' - out = append(out, '\'') - i += 2 - continue - } - return string(out), true // closing quote - } - out = append(out, c) - i++ - } else { // double quote - if c == '\\' && i+1 < len(r) && (r[i+1] == '"' || r[i+1] == '\\') { - out = append(out, r[i+1]) - i += 2 - continue - } - if c == '"' { - return string(out), true // closing quote - } - out = append(out, c) - i++ - } +func emptyMapping() *ast.MappingNode { + return ast.Mapping(token.New("", "", pos()), false) +} + +// parseBlock parses a frontmatter block's inner text. An empty block, and one +// holding only comments, are empty mappings rather than errors -- neither is +// invalid YAML. Any other shape (a bare scalar, a top-level list) is an error. +func parseBlock(fmText string) (*block, error) { + f, err := parser.ParseBytes([]byte(fmText), parser.ParseComments) + if err != nil { + return nil, formatParseError(err) + } + // A "..." line inside the block starts a second document, and reading only + // the first would drop every key after it without a word: `update` would + // then report "no page id" about a file that visibly has one. + if len(f.Docs) > 1 { + return nil, errors.New(`frontmatter must be a single document: remove the "..." line`) + } + if len(f.Docs) == 0 || f.Docs[0].Body == nil { + return &block{mapping: emptyMapping()}, nil + } + switch b := f.Docs[0].Body.(type) { + case *ast.MappingNode: + return &block{mapping: b}, nil + case *ast.MappingValueNode: + m := emptyMapping() + m.Values = append(m.Values, b) + return &block{mapping: m}, nil + case *ast.CommentGroupNode: + return &block{mapping: emptyMapping(), orphan: b}, nil + default: + return nil, fmt.Errorf("frontmatter must be a flat mapping of key: value pairs, found %s", + b.Type()) } - return "", false // unterminated } -// stripInlineComment removes a trailing whitespace-then-`#` comment and trims. -func stripInlineComment(value string) string { - if loc := inlineCommentRE.FindStringIndex(value); loc != nil { - value = value[:loc[0]] +// formatParseError reduces a goccy error to a single line and corrects its +// position for the "---" opener, which the block text does not include. +// +// goccy's default Error() renders a multi-line source excerpt with ASCII +// pointer art, which would land verbatim in check --json's error string. +// Known limit: a duplicate-key message embeds a second position ("already +// defined at [1:1]") that stays block-relative. +func formatParseError(err error) error { + msg := yaml.FormatError(err, false, false) + return errors.New(shiftLeadingPosition(msg)) +} + +// positionRE matches a leading "[line:col] " position stamp. +var positionRE = regexp.MustCompile(`^\[(\d+):(\d+)\] `) + +// shiftLeadingPosition rewrites a leading [line:col] to account for the "---" +// line that opens the block. +func shiftLeadingPosition(msg string) string { + m := positionRE.FindStringSubmatch(msg) + if m == nil { + return msg + } + var line, col int + if _, err := fmt.Sscanf(m[1]+" "+m[2], "%d %d", &line, &col); err != nil { + return msg } - return strings.TrimSpace(value) + return fmt.Sprintf("[%d:%d] %s", line+1, col, msg[len(m[0]):]) } -// quoteValue quotes value for frontmatter, preferring single quotes. -func quoteValue(value string) string { - if !strings.Contains(value, "'") { - return "'" + value + "'" +// 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 "|". +// +// 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 +// the literal "null", which meant "parent: ~" read as though it were a page id. +func scalarValue(key string, n ast.Node) (string, error) { + if spansLines(n.GetToken().Origin) { + return "", fmt.Errorf("frontmatter %q must be a single-line scalar; "+ + "a value split over several lines is not supported", key) } - if !strings.Contains(value, `"`) { - return `"` + value + `"` + switch v := n.(type) { + case *ast.NullNode: + return "", nil + case *ast.StringNode, *ast.IntegerNode, *ast.FloatNode, *ast.BoolNode, + *ast.InfinityNode, *ast.NanNode: + return v.GetToken().Value, nil + default: + return "", fmt.Errorf("frontmatter %q must be a single scalar value, found %s", + key, n.Type()) } - escaped := strings.ReplaceAll(value, `\`, `\\`) - escaped = strings.ReplaceAll(escaped, `"`, `\"`) - return `"` + escaped + `"` } -// renderValue renders a value for a frontmatter line, quoting it only when a bare -// round-trip through ParseValue wouldn't reproduce it. -func renderValue(value string) string { - if ParseValue(" "+value) != value { - return quoteValue(value) +// 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". +// +// 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. +// A value markfluence wrote is never affected: a newline inside one is emitted +// as a two-character \n escape inside a double-quoted scalar, which occupies a +// single physical line. +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) { + fm := make(map[string]string, len(m.Values)) + for _, v := range m.Values { + key := v.Key.GetToken().Value + s, err := scalarValue(key, v.Value) + if err != nil { + return nil, err + } + fm[key] = s } - return value + return fm, nil } -// fieldOrder is the canonical leading order of frontmatter keys; any key not -// listed here is emitted after these, in alphabetical order. Every write goes -// through UpdateField, so this is the single source of frontmatter field order -// across all commands. -var fieldOrder = []string{"title", "space", "parent", "page_id"} +// --- writing ------------------------------------------------------------------ -// UpdateField adds or updates key in content's frontmatter, returning the new -// content. An existing key's value is replaced; a missing key is added; with no -// frontmatter block one is created at the top. The value is auto-quoted when -// needed to round-trip. A non-empty comment is written as a trailing ` # ...` -// annotation, kept distinct from the value so the value round-trips cleanly. +// isDigits reports whether s is one or more ASCII digits. Local rather than +// internal/pageref's copy, which cannot be imported: pageref reads frontmatter. +func isDigits(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + +// doubleQuoted builds a double-quoted scalar. The origin is left empty so goccy +// renders and escapes it from the value; hand-escaping would be one more thing +// to get wrong. +func doubleQuoted(value string) ast.Node { + t := token.New(value, "", pos()) + t.Type = token.DoubleQuoteType + return ast.String(t) +} + +// valueNodeFor builds the node to write for key = value. // -// The whole block is rewritten in the canonical field order (fieldOrder, then -// the rest alphabetically). Full-line `#` comments are preserved at the top of -// the block; blank lines are dropped. -func UpdateField(content, key, value, comment string) string { - rendered := renderValue(value) - if comment != "" { - rendered += " # " + comment +// page_id and parent are typed: digits become an integer and the unset +// spellings become a null, so the emitted YAML says what it means rather than +// `page_id: "123"`. Everything else is a string in whatever style goccy picks, +// verified by round-tripping it and falling back to a double-quoted scalar when +// goccy's choice does not read back. +// +// Double quotes are the last resort, so there is nothing to fall back to if +// they fail too and no error is returned; the fuzz test is what guards that, +// and no probed input has ever reached it. +func valueNodeFor(key, value string) ast.Node { + if typedFields[key] { + switch { + case value == "" || value == "null": + return ast.Null(token.New("null", "null", pos())) + case isDigits(value): + return ast.Integer(token.New(value, value, pos())) + } } - newLine := key + ": " + rendered + n, err := yaml.ValueToNode(value) + if err != nil || !readsBackAs(n, value) { + return doubleQuoted(value) + } + return n +} - loc := frontmatterRE.FindStringSubmatchIndex(content) - if loc == nil { - return "---\n" + newLine + "\n---\n" + content +// readsBackAs emits n as the only value of a one-key mapping, re-parses it, and +// reports whether it survived as a string holding want. +// +// Requiring a *string* is the point, not a detail. scalarValue flattens every +// scalar kind to its token text, so comparing text alone says ".inf" round-trips +// -- markfluence reads it back as ".inf" either way. But `title: .inf` is a +// float to any conforming reader, which is the #130 class all over again. The +// node kind is the only thing that distinguishes "we wrote a string" from "we +// wrote something that happens to spell the same". +func readsBackAs(n ast.Node, want string) bool { + m := emptyMapping() + m.Values = append(m.Values, ast.MappingValue(token.New("", "", pos()), + ast.String(token.New("v", "v", pos())), n)) + f, err := parser.ParseBytes([]byte(m.String()+"\n"), 0) + if err != nil || len(f.Docs) == 0 { + return false + } + parsed, ok := f.Docs[0].Body.(*ast.MappingNode) + if !ok || len(parsed.Values) != 1 { + return false + } + if _, ok := parsed.Values[0].Value.(*ast.StringNode); !ok { + return false } - body := content[loc[1]:] + got, err := scalarValue("v", parsed.Values[0].Value) + return err == nil && got == want +} - comments, fields := splitFrontmatter(content[loc[2]:loc[3]]) - fields[key] = newLine +// commentGroup builds a trailing "# text" comment. +func commentGroup(text string) *ast.CommentGroupNode { + return ast.CommentGroup([]*token.Token{token.New(" "+text, "# "+text, pos())}) +} - lines := comments - for _, k := range orderedKeys(fields) { - lines = append(lines, fields[k]) +// 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)) } - return "---\n" + strings.Join(lines, "\n") + "\n---\n" + body + return v } -// splitFrontmatter parses a frontmatter block's inner text into its full-line -// comments (in order) and a key->line map (each value the raw `key: ...` line, -// so quoting and inline comments are preserved). Blank lines are dropped. -func splitFrontmatter(fmText string) (comments []string, fields map[string]string) { - fields = map[string]string{} - for _, line := range strings.Split(fmText, "\n") { - trimmed := strings.TrimSpace(line) - switch { - case trimmed == "": +// mappingValue builds one `key: value` pair. +func mappingValue(key, value, comment string) *ast.MappingValueNode { + return ast.MappingValue(token.New("", "", pos()), + ast.String(token.New(key, key, pos())), valueWithComment(key, value, comment)) +} + +// Field is one frontmatter entry for Render. +type Field struct { + Key string + Value string + Comment string +} + +// Render builds a frontmatter block from scratch, in canonical order, +// delimiters included. It cannot fail: it constructs nodes and never parses a +// caller's text. +// +// Separate from UpdateField because building and editing are different jobs and +// only editing can fail. Chaining UpdateField to build a five-field block would +// mean parsing and re-emitting it five times, and would push a parse error into +// pagedoc, read, and export, none of which have anything to do with it. A test +// pins that the two agree on the same fields. +func Render(fields []Field) string { + // Last write wins, so a repeated key cannot emit a duplicate the parser + // would then reject. No caller repeats one; this is what makes "cannot fail" + // true rather than true-for-well-behaved-input. + seen := make(map[string]int, len(fields)) + ordered := make([]Field, 0, len(fields)) + for _, f := range fields { + if i, dup := seen[f.Key]; dup { + ordered[i] = f continue - case strings.HasPrefix(trimmed, "#"): - comments = append(comments, trimmed) - default: - if i := strings.Index(trimmed, ":"); i >= 0 { - fields[strings.TrimSpace(trimmed[:i])] = trimmed - } } + seen[f.Key] = len(ordered) + ordered = append(ordered, f) + } + sort.SliceStable(ordered, func(i, j int) bool { return keyLess(ordered[i].Key, ordered[j].Key) }) + + m := emptyMapping() + for _, f := range ordered { + m.Values = append(m.Values, mappingValue(f.Key, f.Value, f.Comment)) + } + if len(m.Values) == 0 { + return "---\n---\n" } - return comments, fields + return "---\n" + m.String() + "\n---\n" } -// orderedKeys returns the keys of fields in canonical order: those in fieldOrder -// first (in that order), then any remaining keys alphabetically. -func orderedKeys(fields map[string]string) []string { - rank := map[string]int{} - for i, k := range fieldOrder { - rank[k] = i +// UpdateField adds or updates key in content's frontmatter, returning the new +// content. With no frontmatter block one is created at the top. +// +// The edit is surgical: an existing key keeps its position and only its value +// node is replaced, and a new key is inserted before the first key that sorts +// after it. Nothing else moves, so a file in git gets the smallest diff the +// emitter can produce -- though not a literally minimal one, since re-emitting +// normalizes intra-line whitespace and can move a blank line, which lives in the +// preceding value's token rather than in a node of its own. +// +// The value node is replaced rather than mutated in place. Mutating a plain +// 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) { + loc := frontmatterRE.FindStringSubmatchIndex(content) + if loc == nil { + return Render([]Field{{Key: key, Value: value, Comment: comment}}) + content, nil } - keys := make([]string, 0, len(fields)) - for k := range fields { - keys = append(keys, k) + b, err := parseBlock(content[loc[2]:loc[3]]) + if err != nil { + return "", err } - sort.Slice(keys, func(i, j int) bool { - ri, iok := rank[keys[i]] - rj, jok := rank[keys[j]] - switch { - case iok && jok: - return ri < rj - case iok: - return true - case jok: - return false - default: - return keys[i] < keys[j] + setField(b, key, value, comment) + 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) { + // 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) + return } - }) - return keys + } + mv := mappingValue(key, value, comment) + // 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 { + _ = mv.SetComment(b.orphan) + b.orphan = nil + } + at := len(b.mapping.Values) + for i, v := range b.mapping.Values { + if keyLess(key, v.Key.GetToken().Value) { + at = i + break + } + } + b.mapping.Values = append(b.mapping.Values, nil) + copy(b.mapping.Values[at+1:], b.mapping.Values[at:]) + b.mapping.Values[at] = mv } +// --- MarkdownFile --------------------------------------------------------------- + // MarkdownFile is a markdown source file parsed once: its path, raw text, // frontmatter map, 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: PageID/Space/Parent treat missing, blank, or -// literal "null" as unset (returning ""), while Title only collapses missing or -// blank -- a title is free text, so a literal "null" is kept. +// methods provide normalized reads: every null spelling and a blank value alike +// read as "". type MarkdownFile struct { Filename string Content string @@ -240,30 +437,30 @@ type MarkdownFile struct { Body string } -// ErrUnterminatedFrontmatter is returned by Parse/ParseFile when content opens -// with a "---\n" delimiter that never closes. Extract is deliberately lenient -// about this shape -- a regex miss just falls back to "no frontmatter, whole -// file is body" -- which would otherwise hide a common paste mistake -// completely, including from every command that calls Parse. -// -// This is a lexical check, not a parse: a document whose very first line is a -// bare thematic break (a markdown horizontal rule, "---" with nothing after it -// that closes with a second "---\n") is indistinguishable from unterminated -// frontmatter and is flagged the same way. Accepted deliberately -- a document -// opening cold with a horizontal rule and no heading is unusual, and detecting -// the difference would need real parsing, not a shape this small. -var ErrUnterminatedFrontmatter = errors.New( - `unterminated frontmatter block: starts with "---" but has no closing "---" line`) - // Parse builds a MarkdownFile from an in-memory content string tagged with -// filename, or reports ErrUnterminatedFrontmatter. +// filename, or reports why the frontmatter could not be read. func Parse(filename, content string) (*MarkdownFile, error) { loc := frontmatterRE.FindStringSubmatchIndex(content) - if loc == nil && strings.HasPrefix(content, "---\n") { - return nil, ErrUnterminatedFrontmatter + if loc == nil { + if strings.HasPrefix(content, "---\n") { + return nil, ErrUnterminatedFrontmatter + } + return &MarkdownFile{ + Filename: filename, Content: content, + Frontmatter: map[string]string{}, Body: content, + }, nil } - fm, body := extractFrom(loc, content) - return &MarkdownFile{Filename: filename, Content: content, Frontmatter: fm, Body: body}, 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) + if err != nil { + return nil, fmt.Errorf("%s: %w", filename, err) + } + return &MarkdownFile{ + Filename: filename, Content: content, Frontmatter: fm, Body: content[loc[1]:], + }, nil } // ParseFile reads filename from disk and parses it. @@ -275,29 +472,31 @@ func ParseFile(filename string) (*MarkdownFile, error) { return Parse(filename, string(data)) } -// coordinate reads a page-coordinate field, mapping the no-value sentinels to "". -func (m *MarkdownFile) coordinate(key string) string { - v, ok := m.Frontmatter[key] - if !ok { - return "" - } - v = strings.TrimSpace(v) - if v == "" || v == "null" { - return "" - } - return v +// field reads a frontmatter field, treating whitespace-only as unset. Every +// null spelling already read as "" at parse time. +func (m *MarkdownFile) field(key string) string { + return strings.TrimSpace(m.Frontmatter[key]) } -// Title returns the title, "" if missing or blank ("null" is a legal title). -func (m *MarkdownFile) Title() string { - return strings.TrimSpace(m.Frontmatter["title"]) +// Title returns the title, "" if missing, blank, or null. +func (m *MarkdownFile) Title() string { return m.field("title") } + +// TitleField reports the title and whether a title key was present at all. +// +// The two differ where it matters: an absent title is a positive statement that +// a file does not manage its page's title, which update honours by keeping the +// live one, while a present-but-empty title is a half-finished edit that should +// not silently publish under whatever the page is called now. +func (m *MarkdownFile) TitleField() (title string, present bool) { + raw, present := m.Frontmatter["title"] + return strings.TrimSpace(raw), present } -// PageID returns the page id, "" if missing, blank, or "null". -func (m *MarkdownFile) PageID() string { return m.coordinate("page_id") } +// PageID returns the page id, "" if missing, blank, or null. +func (m *MarkdownFile) PageID() string { return m.field("page_id") } -// Space returns the space key, "" if missing, blank, or "null". -func (m *MarkdownFile) Space() string { return m.coordinate("space") } +// Space returns the space key, "" if missing, blank, or null. +func (m *MarkdownFile) Space() string { return m.field("space") } -// Parent returns the parent, "" if missing, blank, or "null". -func (m *MarkdownFile) Parent() string { return m.coordinate("parent") } +// Parent returns the parent, "" if missing, blank, or null. +func (m *MarkdownFile) Parent() string { return m.field("parent") } diff --git a/internal/frontmatter/frontmatter_test.go b/internal/frontmatter/frontmatter_test.go index f61ae5c..468fa5d 100644 --- a/internal/frontmatter/frontmatter_test.go +++ b/internal/frontmatter/frontmatter_test.go @@ -7,135 +7,309 @@ import ( "github.com/mozilla/markfluence/internal/frontmatter" ) -// value extracts frontmatter and returns the value for key. -func value(t *testing.T, body, key string) string { +// value parses content and returns one frontmatter field. +func value(t *testing.T, content, key string) string { t.Helper() - fm, _ := frontmatter.Extract(body) - v, ok := fm[key] - if !ok { - t.Fatalf("key %q not found in frontmatter of:\n%s", key, body) + mf, err := frontmatter.Parse("doc.md", content) + if err != nil { + t.Fatalf("Parse(%q) = %v", content, err) } - return v + return mf.Frontmatter[key] } -// --- read: quotes suppress inline-comment stripping -------------------------- +func update(t *testing.T, content, key, val, comment string) string { + t.Helper() + got, err := frontmatter.UpdateField(content, key, val, comment) + if err != nil { + t.Fatalf("UpdateField(%q, %q, %q) = %v", content, key, val, err) + } + return got +} + +// --- read --------------------------------------------------------------------- func TestReadValues(t *testing.T) { - tests := []struct { - name string - body string - key string - want string - }{ + tests := []struct{ name, content, key, want string }{ + {"plain", "---\ntitle: Hello\n---\nx\n", "title", "Hello"}, {"double-quoted keeps hash", "---\ntitle: \"Detect # Verify\"\n---\nx\n", "title", "Detect # Verify"}, {"single-quoted keeps hash", "---\ntitle: 'Detect # Verify'\n---\nx\n", "title", "Detect # Verify"}, - {"unquoted strips inline comment", "---\ntitle: Detect # Verify\n---\nx\n", "title", "Detect"}, - {"parent comment form reads value only", "---\nparent: 4 # foo.md\n---\nx\n", "parent", "4"}, {"single-quote escape", "---\ntitle: 'it''s here'\n---\nx\n", "title", "it's here"}, - {"double-quote escapes", "---\ntitle: \"say \\\"hi\\\"\"\n---\nx\n", "title", `say "hi"`}, + {"inline comment stripped", "---\ntitle: Hello # note\n---\nx\n", "title", "Hello"}, + {"colon inside quotes", "---\ntitle: \"a: b\"\n---\nx\n", "title", "a: b"}, + {"integer stays digits", "---\npage_id: 123\n---\nx\n", "page_id", "123"}, + {"tilde space key is a string", "---\nspace: ~abc\n---\nx\n", "space", "~abc"}, + {"unicode escape", "---\ntitle: \"caf\\u00e9\"\n---\nx\n", "title", "café"}, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - if got := value(t, tc.body, tc.key); got != tc.want { - t.Errorf("value = %q, want %q", got, tc.want) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := value(t, tt.content, tt.key); got != tt.want { + t.Errorf("%s = %q, want %q", tt.key, got, tt.want) } }) } } -func TestParseValueUnterminatedQuoteFallsBackToLiteral(t *testing.T) { - if got := frontmatter.ParseValue(` "oops`); got != `"oops` { - t.Errorf("ParseValue = %q, want %q", got, `"oops`) +// TestEveryNullSpellingIsUnset pins that a null is identified by node type, not +// by matching the literal text "null". The old parser matched only "null", so +// `parent: ~` read as though it were a page id. +func TestEveryNullSpellingIsUnset(t *testing.T) { + for _, spelling := range []string{"", " ", " null", " Null", " NULL", " ~"} { + content := "---\nparent:" + spelling + "\npage_id:" + spelling + "\n---\nx\n" + mf, err := frontmatter.Parse("doc.md", content) + if err != nil { + t.Fatalf("Parse(parent:%q) = %v", spelling, err) + } + if got := mf.Parent(); got != "" { + t.Errorf("Parent() for %q = %q, want empty", spelling, got) + } + if got := mf.PageID(); got != "" { + t.Errorf("PageID() for %q = %q, want empty", spelling, got) + } } } -func TestPlainValuesUnaffected(t *testing.T) { - fm, _ := frontmatter.Extract("---\npage_id: 5\nspace: ENG\n---\nx\n") - if fm["page_id"] != "5" || fm["space"] != "ENG" || len(fm) != 2 { - t.Errorf("frontmatter = %v, want {page_id:5, space:ENG}", fm) +func TestNoFrontmatterIsWholeBody(t *testing.T) { + mf, err := frontmatter.Parse("doc.md", "# Heading\n") + if err != nil { + t.Fatal(err) + } + if len(mf.Frontmatter) != 0 || mf.Body != "# Heading\n" { + t.Errorf("fm = %v, body = %q", mf.Frontmatter, mf.Body) } } -// --- write: auto-quote only when needed -------------------------------------- - -// fieldLine writes key/value into a fixed doc and returns the "key: ..." line. -func fieldLine(t *testing.T, key, value, comment string) string { - t.Helper() - md := frontmatter.UpdateField("---\nk: x\n---\nbody\n", key, value, comment) - for _, line := range strings.Split(md, "\n") { - if strings.HasPrefix(line, key+":") { - return line +func TestEmptyBlockIsEmptyMapping(t *testing.T) { + for _, content := range []string{"---\n\n---\nbody\n", "---\n# just a note\n---\nbody\n"} { + mf, err := frontmatter.Parse("doc.md", content) + if err != nil { + t.Fatalf("Parse(%q) = %v, want no error", content, err) + } + if len(mf.Frontmatter) != 0 { + t.Errorf("Parse(%q) fm = %v, want empty", content, mf.Frontmatter) } } - t.Fatalf("no line starting with %q in:\n%s", key+":", md) - return "" } -func TestWriteRendering(t *testing.T) { - tests := []struct { - name, key, value, comment, want string - }{ - {"safe value bare", "title", "Hello World", "", "title: Hello World"}, - {"numeric bare", "page_id", "12345", "", "page_id: 12345"}, - {"colon value bare", "title", "a: b", "", "title: a: b"}, - {"inline-comment marker quoted", "title", "Detect # Verify", "", "title: 'Detect # Verify'"}, - {"leading whitespace quoted", "title", " x", "", "title: ' x'"}, - {"comment separate from value", "parent", "4", "foo.md", "parent: 4 # foo.md"}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - if got := fieldLine(t, tc.key, tc.value, tc.comment); got != tc.want { - t.Errorf("line = %q, want %q", got, tc.want) +func TestUnterminatedFrontmatter(t *testing.T) { + _, err := frontmatter.Parse("doc.md", "---\ntitle: X\nbody with no closing fence\n") + if err == nil || !strings.Contains(err.Error(), "unterminated") { + t.Errorf("err = %v, want ErrUnterminatedFrontmatter", err) + } +} + +// TestRejectedShapes covers everything the flat-scalar 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. +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"}, + {"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"}, + {"duplicate key", "---\ntitle: A\ntitle: B\n---\nx\n", "already defined"}, + {"tab indent", "---\ntitle: T\n\tpage_id: 9\n---\nx\n", "cannot start any token"}, + {"space before colon", "---\ntitle :v\n---\nx\n", "flat mapping"}, + {"top-level scalar", "---\njust text\n---\nx\n", "flat mapping"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := frontmatter.Parse("doc.md", tt.content) + if err == nil { + t.Fatalf("Parse(%q) = nil error, want one", tt.content) + } + if !strings.Contains(err.Error(), tt.wantSubstr) { + t.Errorf("err = %q, want it to mention %q", err, tt.wantSubstr) } }) } } +// TestParseErrorIsOneLineAtTheFilePosition pins both halves of the error +// treatment: goccy's multi-line source art is reduced to a line, and the +// position counts the "---" opener the block text does not include. +func TestParseErrorIsOneLineAtTheFilePosition(t *testing.T) { + _, err := frontmatter.Parse("doc.md", "---\ntitle: a: b\n---\nx\n") + if err == nil { + t.Fatal("want an error") + } + if strings.Contains(err.Error(), "\n") { + t.Errorf("err spans lines:\n%s", err) + } + if !strings.Contains(err.Error(), "[2:") { + t.Errorf("err = %q, want the file line 2, not the block line 1", err) + } +} + +// --- write -------------------------------------------------------------------- + +func TestWriteQuotesWhatYAMLNeeds(t *testing.T) { + tests := []struct{ name, key, value, want string }{ + {"safe value bare", "title", "Hello World", "title: Hello World"}, + {"apostrophe stays bare", "title", "Bob's Runbook", "title: Bob's Runbook"}, + {"colon quoted", "title", "a: b", `title: "a: b"`}, + {"hash quoted", "title", "Detect # Verify", `title: "Detect # Verify"`}, + {"leading space quoted", "title", " x", `title: " x"`}, + {"boolean-looking quoted", "title", "true", `title: "true"`}, + {"number-looking quoted", "title", "123", `title: "123"`}, + {"null-looking quoted", "title", "null", `title: "null"`}, + {"flow sequence quoted", "title", "[draft] Foo", `title: "[draft] Foo"`}, + {"indicator quoted", "title", "@home", `title: "@home"`}, + {"page_id is an integer", "page_id", "123", "page_id: 123"}, + {"parent null is a null", "parent", "null", "parent: null"}, + {"parent path is a string", "parent", "../index.md", "parent: ../index.md"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := update(t, "---\nk: v\n---\nbody\n", tt.key, tt.value, "") + if !strings.Contains(got, tt.want+"\n") { + t.Errorf("UpdateField wrote:\n%s\nwant a line %q", got, tt.want) + } + }) + } +} + +// hazards are values that have to survive a write-then-read unchanged. The last +// four are the ones goccy's own default emission gets wrong: it drops a tab and +// writes "? q" as a document it then refuses to parse. They are here because the +// writer verifies its own output and falls back to a double-quoted scalar, not +// because goccy handles them. +// +// .inf and .nan are in that group too, but this test cannot see the difference: +// scalarValue flattens every scalar kind to its token text, so they read back as +// themselves either way. TestInfinityAndNaNAreQuoted is what pins those. +var hazards = []string{ + "Plain Title", "Bob's Runbook", "a: b", "Detect # Verify", " lead", "trail ", + "true", "false", "no", "123", "1.5", "null", "~", "[draft] Foo", "{a}", + "@home", "*star", "&anchor", "!bang", "|pipe", ">gt", "%pct", "- dash", + "a,b", "0x1f", "12:30", "2026-09-06", "", "café", `say "hi"`, `back\slash`, + "a\tb", "? q", ".inf", ".nan", +} + +// TestWriteThenReadRoundTrips is the guard behind C2. It is not testing goccy: +// it pins that every write still goes through the serializer, so a later fast +// path that concatenates a frontmatter line by hand fails here instead of +// shipping the bug this package was rewritten to remove. func TestWriteThenReadRoundTrips(t *testing.T) { - values := []string{"Detect # Verify", "it's here", `say "hi"`, " pad ", "#lead", "plain", "a: b"} - for _, v := range values { + for _, v := range hazards { t.Run(v, func(t *testing.T) { - md := frontmatter.UpdateField("---\nk: x\n---\nb\n", "k", v, "") - fm, _ := frontmatter.Extract(md) - if fm["k"] != v { - t.Errorf("round-trip of %q = %q", v, fm["k"]) + got := value(t, update(t, "---\nk: v\n---\nbody\n", "title", v, ""), "title") + if got != v { + t.Errorf("round-trip of %q = %q", v, got) } }) } } +// FuzzUpdateFieldRoundTrips is the only thing that can find a value the +// double-quote fallback also fails on. Under plain `go test` it runs the seed +// corpus only, so it costs what a table test costs. +func FuzzUpdateFieldRoundTrips(f *testing.F) { + for _, v := range hazards { + f.Add(v) + } + f.Fuzz(func(t *testing.T, v string) { + if !strings.ContainsRune(v, 0) && strings.ToValidUTF8(v, "") != v { + t.Skip("not valid UTF-8") + } + out, err := frontmatter.UpdateField("---\nk: v\n---\nbody\n", "title", v, "") + if err != nil { + t.Skipf("UpdateField refused %q: %v", v, err) + } + mf, err := frontmatter.Parse("doc.md", out) + if err != nil { + t.Fatalf("wrote frontmatter it cannot read back for %q:\n%s\n%v", v, out, err) + } + if got := mf.Frontmatter["title"]; got != v { + t.Fatalf("round-trip of %q = %q\nwrote:\n%s", v, got, out) + } + }) +} + +func TestRenderAndUpdateFieldAgree(t *testing.T) { + fields := []frontmatter.Field{ + {Key: "title", Value: "a: b"}, + {Key: "space", Value: "ENG"}, + {Key: "parent", Value: "42", Comment: "original.md"}, + {Key: "page_id", Value: "123"}, + {Key: "page_width", Value: "max"}, + } + built := frontmatter.Render(fields) + + chained := "" + for _, f := range fields { + chained = update(t, chained, f.Key, f.Value, f.Comment) + } + if built != chained { + t.Errorf("Render:\n%s\nUpdateField chain:\n%s", built, chained) + } +} + func TestParentValueRoundTripsWithoutTheComment(t *testing.T) { - md := frontmatter.UpdateField("---\nk: x\n---\nb\n", "parent", "4", "foo.md") - fm, _ := frontmatter.Extract(md) - if fm["parent"] != "4" { - t.Errorf("parent = %q, want %q", fm["parent"], "4") + md := update(t, "---\nk: x\n---\nb\n", "parent", "4", "foo.md") + if !strings.Contains(md, "parent: 4 # foo.md") { + t.Errorf("wrote:\n%s\nwant an inline comment", md) + } + if got := value(t, md, "parent"); got != "4" { + t.Errorf("parent = %q, want %q", got, "4") } } -// --- write: canonical field order -------------------------------------------- +// --- surgical edits ------------------------------------------------------------- -func TestUpdateFieldCanonicalOrder(t *testing.T) { - // Fields present in a jumbled order plus an extra key; updating any field - // rewrites the whole block as title, space, parent, page_id, then the rest - // alphabetically. - in := "---\npage_width: max\npage_id: 9\ncustom: z\nparent: 4\nspace: ENG\ntitle: T\n---\nbody\n" - got := frontmatter.UpdateField(in, "page_id", "10", "") - want := "---\ntitle: T\nspace: ENG\nparent: 4\npage_id: 10\ncustom: z\npage_width: max\n---\nbody\n" +func TestUpdateFieldKeepsExistingPositions(t *testing.T) { + in := "---\npage_id: 9\ntitle: T\n---\nbody\n" + got := update(t, in, "page_id", "10", "") + want := "---\npage_id: 10\ntitle: T\n---\nbody\n" if got != want { - t.Errorf("UpdateField reorder =\n%q\nwant\n%q", got, want) + t.Errorf("UpdateField =\n%q\nwant\n%q", got, want) } } -func TestUpdateFieldPreservesCommentsDropsBlanks(t *testing.T) { - in := "---\n# a note\ntitle: T\n\npage_id: 9\n---\nbody\n" - got := frontmatter.UpdateField(in, "space", "ENG", "") - want := "---\n# a note\ntitle: T\nspace: ENG\npage_id: 9\n---\nbody\n" +func TestUpdateFieldInsertsCanonically(t *testing.T) { + in := "---\ntitle: T\npage_id: 9\n---\nbody\n" + got := update(t, in, "space", "ENG", "") + want := "---\ntitle: T\nspace: ENG\npage_id: 9\n---\nbody\n" if got != want { - t.Errorf("UpdateField comments/blanks =\n%q\nwant\n%q", got, want) + t.Errorf("UpdateField =\n%q\nwant\n%q", got, want) + } +} + +func TestUpdateFieldCreatesBlockWhenAbsent(t *testing.T) { + got := update(t, "# Heading\n", "title", "T", "") + want := "---\ntitle: T\n---\n# Heading\n" + if got != want { + t.Errorf("UpdateField =\n%q\nwant\n%q", got, want) + } +} + +func TestUpdateFieldKeepsCommentsAndBlanks(t *testing.T) { + in := "---\n# a note\ntitle: T\n\npage_id: 9\n---\nbody\n" + got := update(t, in, "page_id", "10", "") + for _, want := range []string{"# a note", "title: T", "page_id: 10"} { + if !strings.Contains(got, want) { + t.Errorf("UpdateField =\n%q\nwant it to keep %q", got, want) + } + } + if !strings.Contains(got, "\n\n") { + t.Errorf("UpdateField =\n%q\nwant the blank line kept", got) } } -// --- MarkdownFile accessors -------------------------------------------------- +// TestUpdateFieldKeepsACommentOnlyBlocksNote pins that a block holding nothing +// but a comment does not lose it on the first write. The comment has no key to +// attach to at parse time, so it has to be carried onto the first one inserted. +func TestUpdateFieldKeepsACommentOnlyBlocksNote(t *testing.T) { + got := update(t, "---\n# keep me\n---\nbody\n", "title", "T", "") + if !strings.Contains(got, "keep me") { + t.Errorf("UpdateField =\n%q\nwant the note kept", got) + } +} + +// --- MarkdownFile accessors ----------------------------------------------------- func TestMarkdownFileAccessors(t *testing.T) { md, err := frontmatter.Parse("doc.md", @@ -143,76 +317,140 @@ func TestMarkdownFileAccessors(t *testing.T) { if err != nil { t.Fatal(err) } - if md.Title() != "My Page" || md.PageID() != "123" || md.Space() != "ENG" || md.Parent() != "456" { - t.Errorf("accessors = %q/%q/%q/%q", md.Title(), md.PageID(), md.Space(), md.Parent()) - } - if md.Body != "body\n" { - t.Errorf("Body = %q, want %q", md.Body, "body\n") + for _, tt := range []struct{ name, got, want string }{ + {"Title", md.Title(), "My Page"}, + {"PageID", md.PageID(), "123"}, + {"Space", md.Space(), "ENG"}, + {"Parent", md.Parent(), "456"}, + {"Body", md.Body, "body\n"}, + {"Filename", md.Filename, "doc.md"}, + } { + if tt.got != tt.want { + t.Errorf("%s = %q, want %q", tt.name, tt.got, tt.want) + } } } -func TestCoordinateSentinelsAreUnset(t *testing.T) { - // Missing, blank, and literal "null" all read as "" for coordinate fields. - for _, doc := range []string{ - "---\ntitle: X\n---\nb\n", // page_id missing - "---\ntitle: X\npage_id:\n---\nb\n", // blank - "---\ntitle: X\npage_id: null\n---\nb\n", // literal null - } { - md, err := frontmatter.Parse("doc.md", doc) - if err != nil { - t.Fatal(err) - } - if md.PageID() != "" { - t.Errorf("PageID() = %q for %q, want empty", md.PageID(), doc) - } +// TestTitleFieldSeparatesAbsentFromEmpty is what update and check need: an +// absent title means "this file does not manage the title", a present-but-empty +// one is a half-finished edit. +func TestTitleFieldSeparatesAbsentFromEmpty(t *testing.T) { + tests := []struct { + name, content string + wantPresent bool + }{ + {"absent", "---\npage_id: 1\n---\nx\n", false}, + {"present and empty", "---\ntitle:\npage_id: 1\n---\nx\n", true}, + {"present and null", "---\ntitle: null\npage_id: 1\n---\nx\n", true}, + {"present with a value", "---\ntitle: T\n---\nx\n", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mf, err := frontmatter.Parse("doc.md", tt.content) + if err != nil { + t.Fatal(err) + } + if _, present := mf.TitleField(); present != tt.wantPresent { + t.Errorf("present = %v, want %v", present, tt.wantPresent) + } + }) } } -func TestTitleKeepsLiteralNullButBlankIsEmpty(t *testing.T) { - md, err := frontmatter.Parse("d.md", "---\ntitle: null\n---\nb\n") - if err != nil { - t.Fatal(err) +// TestInfinityAndNaNAreQuoted is the regression for a verify-and-retry loop that +// compared only text. scalarValue flattens every scalar kind to its token, so +// ".inf" appeared to round-trip while being written bare -- and `title: .inf` is +// a float to any conforming reader, which is #130 again. The check requires the +// re-parsed node to be a string, not merely to spell the same. +func TestInfinityAndNaNAreQuoted(t *testing.T) { + for _, v := range []string{".inf", "-.inf", ".nan", ".NaN", ".Inf"} { + t.Run(v, func(t *testing.T) { + got := update(t, "---\nk: v\n---\nbody\n", "title", v, "") + if !strings.Contains(got, `title: "`+v+`"`) { + t.Errorf("wrote:\n%s\nwant %q quoted", got, v) + } + }) } - if md.Title() != "null" { - t.Errorf("Title() = %q, want %q (a title is free text)", md.Title(), "null") +} + +// TestMultiLineScalarIsRejected pins the flat contract at read time. It has to +// be enforced rather than trusted, because an untouched key is re-emitted from +// the node the parser produced and goccy's re-emission is not identity: a +// continued plain scalar comes back as a "|-" block that Parse then refuses, so +// UpdateField would write a file it cannot read -- and in create, only after the +// page had been made. +func TestMultiLineScalarIsRejected(t *testing.T) { + tests := []struct{ name, content string }{ + {"plain continuation", "---\ntitle: plain\n continued\npage_id: 9\n---\nx\n"}, + {"plain across blank", "---\ntitle: plain\n\n continued\npage_id: 9\n---\nx\n"}, + {"single-quoted", "---\ntitle: 'sq\n line'\npage_id: 9\n---\nx\n"}, + {"double-quoted", "---\ntitle: \"dq\n line\"\npage_id: 9\n---\nx\n"}, } - md, err = frontmatter.Parse("d.md", "---\ntitle:\n---\nb\n") - if err != nil { - t.Fatal(err) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := frontmatter.Parse("doc.md", tt.content) + if err == nil { + t.Fatalf("Parse(%q) = nil error, want one", tt.content) + } + if !strings.Contains(err.Error(), "single-line") { + t.Errorf("err = %q, want it to mention single-line", err) + } + }) } - if md.Title() != "" { - t.Errorf("Title() = %q, want empty for blank", md.Title()) +} + +// TestNewlineValueWeWroteStillRoundTrips is the other side of the line: our own +// escaped newline is one physical line, so the single-line rule must not reject +// it. +func TestNewlineValueWeWroteStillRoundTrips(t *testing.T) { + for _, v := range []string{"a\nb", "a\tb"} { + got := update(t, "---\nk: v\n---\nbody\n", "title", v, "") + // The block is exactly two field lines: the escaped newline must not + // have become a real one. + block := strings.SplitN(strings.TrimPrefix(got, "---\n"), "\n---\n", 2)[0] + if lines := strings.Split(block, "\n"); len(lines) != 2 { + t.Errorf("wrote %d lines, want 2:\n%q", len(lines), got) + } + if back := value(t, got, "title"); back != v { + t.Errorf("round-trip of %q = %q", v, back) + } } } -func TestParseUnterminatedFrontmatter(t *testing.T) { - _, err := frontmatter.Parse("d.md", "---\ntitle: T\nno closing delimiter\n") - if err != frontmatter.ErrUnterminatedFrontmatter { - t.Errorf("err = %v, want ErrUnterminatedFrontmatter", err) +// TestSecondDocumentIsRejected covers a "..." line, which starts a new YAML +// document. Reading only the first would drop every key after it in silence, +// and update would report "no page id" about a file that visibly has one. +func TestSecondDocumentIsRejected(t *testing.T) { + _, err := frontmatter.Parse("doc.md", "---\ntitle: T\n...\npage_id: 9\n---\nx\n") + if err == nil || !strings.Contains(err.Error(), "single document") { + t.Errorf("err = %v, want a single-document error", err) } } -// TestParseFlagsALeadingThematicBreakToo pins a known, accepted tradeoff -// (see ErrUnterminatedFrontmatter's doc comment): a document opening with a -// bare "---" horizontal rule and nothing that closes it is indistinguishable -// from unterminated frontmatter using this lexical check, and is flagged the -// same way rather than silently read as "no frontmatter". This is not a bug -// to fix here -- it's what the shape of the check can and cannot tell apart. -func TestParseFlagsALeadingThematicBreakToo(t *testing.T) { - _, err := frontmatter.Parse("d.md", "---\n\nSome body with no frontmatter at all.\n") - if err != frontmatter.ErrUnterminatedFrontmatter { - t.Errorf("err = %v, want ErrUnterminatedFrontmatter (a known false positive, not a regression)", err) +func TestRenderLastDuplicateWins(t *testing.T) { + got := frontmatter.Render([]frontmatter.Field{ + {Key: "title", Value: "first"}, {Key: "title", Value: "second"}, + }) + if got != "---\ntitle: second\n---\n" { + t.Errorf("Render = %q, want the last value and no duplicate key", got) + } + if _, err := frontmatter.Parse("doc.md", got); err != nil { + t.Errorf("Render emitted something Parse rejects: %v", err) } } -func TestParseUnterminatedFrontmatterNoFalsePositives(t *testing.T) { - for name, doc := range map[string]string{ - "proper frontmatter": "---\ntitle: T\n---\nbody\n", - "no frontmatter": "just a document\nwith no frontmatter at all\n", - "--- in a fenced code block, not at the top": "intro\n\n```\n---\nnot frontmatter\n---\n```\n", - } { - if _, err := frontmatter.Parse("d.md", doc); err != nil { - t.Errorf("%s: err = %v, want nil", name, err) +// TestNullPageWidthIsUnsetNotInvalid records a consequence of nulls being unset: +// page_width: null used to reach pagewidth.Declared as the string "null" and be +// rejected as an invalid width. It now means "not set", so the default applies +// and check no longer reports it. +func TestNullPageWidthIsUnset(t *testing.T) { + for _, spelling := range []string{"", " null", " ~"} { + mf, err := frontmatter.Parse("doc.md", "---\npage_width:"+spelling+"\n---\nx\n") + if err != nil { + t.Fatalf("Parse(page_width:%q) = %v", spelling, err) + } + if got := mf.Frontmatter["page_width"]; got != "" { + t.Errorf("page_width for %q = %q, want empty", spelling, got) } } } diff --git a/internal/pagedoc/pagedoc.go b/internal/pagedoc/pagedoc.go index d2f51b3..5bf5b8d 100644 --- a/internal/pagedoc/pagedoc.go +++ b/internal/pagedoc/pagedoc.go @@ -279,20 +279,19 @@ func Frontmatter(c *client.ConfluenceClient, page *client.Page, parentOverride s } // RenderFrontmatter assembles the frontmatter block from resolved field values, -// omitting space/parent/page_width when empty. UpdateField emits them in the -// canonical order and auto-quotes values as needed. +// 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 { - fm := "" - fm = frontmatter.UpdateField(fm, "title", title, "") + fields := []frontmatter.Field{{Key: "title", Value: title}} if space != "" { - fm = frontmatter.UpdateField(fm, "space", space, "") + fields = append(fields, frontmatter.Field{Key: "space", Value: space}) } if parent != "" { - fm = frontmatter.UpdateField(fm, "parent", parent, "") + fields = append(fields, frontmatter.Field{Key: "parent", Value: parent}) } - fm = frontmatter.UpdateField(fm, "page_id", pageID, "") + fields = append(fields, frontmatter.Field{Key: "page_id", Value: pageID}) if width != "" { - fm = frontmatter.UpdateField(fm, "page_width", width, "") + fields = append(fields, frontmatter.Field{Key: "page_width", Value: width}) } - return fm + return frontmatter.Render(fields) } diff --git a/internal/pagedoc/pagedoc_test.go b/internal/pagedoc/pagedoc_test.go index b3c7af3..4d177dc 100644 --- a/internal/pagedoc/pagedoc_test.go +++ b/internal/pagedoc/pagedoc_test.go @@ -36,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", "") - want := "---\ntitle: '# Sharp'\npage_id: 1\n---\n" + want := "---\ntitle: \"# Sharp\"\npage_id: 1\n---\n" if got != want { t.Errorf("RenderFrontmatter =\n%q\nwant\n%q", got, want) } From 84a3401dc04f2ac7e0c1118307cb3d448885b03d Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:28:44 -0400 Subject: [PATCH 3/5] fix(update): error on a present-but-empty title An empty title used to fall back to the live page's title whether the key was absent or present. The two mean different things: no title key is a positive statement that a file does not manage its page's title, which is the shape fix.go's own "fill in a missing title" branch already reasons about, while `title:` present and empty is a half-finished edit that should not silently publish under whatever the page happens to be called now. Reads as empty for every null spelling too, so `title: null` is caught alongside `title:` -- which is new: the old parser kept a literal "null" as a legal title, so it would have published a page named "null". The check fires before GetPageOrNil, matching the IsDigits pre-flight above it: a local defect should not cost a round trip. --title still wins, as every other override does, so it satisfies a present-but-empty frontmatter title rather than tripping over it. resolveTitlePageID needs a third return to say this at all; two strings cannot distinguish absent from present-and-empty. create already errors on any empty title (create.go:554) and is unchanged. --- cmd/update/update.go | 26 +++++++++--- cmd/update/update_test.go | 87 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 105 insertions(+), 8 deletions(-) diff --git a/cmd/update/update.go b/cmd/update/update.go index 4f75e22..a9ee7d5 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -143,7 +143,16 @@ func processFile( return r.fail(err, jsonout.CodeValidation) } - title, pageID := resolveTitlePageID(titleFlag, pageIDFlag, mf) + title, titlePresent, pageID := resolveTitlePageID(titleFlag, pageIDFlag, mf) + // Before the request, like the page-id check below: an empty title is a + // local defect, and paying for a round trip to discover it is waste. Only a + // title that is *present* and empty is wrong -- an absent title means the + // file does not manage the page's title, which is honoured further down. + if title == "" && titlePresent { + return r.fail(errors.New( + "frontmatter has an empty 'title:'; give it a value, remove it to keep the "+ + "live page title, or pass --title"), jsonout.CodeValidation) + } if pageID == "" { return r.fail(errors.New("no page id: set page_id in frontmatter or pass --page-id"), jsonout.CodeValidation) @@ -298,18 +307,23 @@ func overrideNeedsSingleFile(cliTitle, cliPageID string, nFiles int) bool { } // resolveTitlePageID resolves the effective title and page id, letting the CLI -// flags override the file's frontmatter. Either may be "" (an empty title falls -// back to the live page title later; an empty page id is an error). -func resolveTitlePageID(cliTitle, cliPageID string, mf *frontmatter.MarkdownFile) (title, pageID string) { +// flags override the file's frontmatter. An empty page id is an error; an empty +// title is an error only when the frontmatter key is present, which is what +// titlePresent reports. An absent title falls back to the live page title later. +// +// --title wins over both, as every other override does, so it satisfies a +// present-but-empty frontmatter title rather than tripping over it. +func resolveTitlePageID(cliTitle, cliPageID string, mf *frontmatter.MarkdownFile) ( + title string, titlePresent bool, pageID string) { title = cliTitle if title == "" { - title = mf.Title() + title, titlePresent = mf.TitleField() } pageID = cliPageID if pageID == "" { pageID = mf.PageID() } - return title, pageID + return title, titlePresent, pageID } // resolveWidth resolves the page width to assert. It returns apply=false when diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index c83f732..cb60f92 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -37,7 +37,7 @@ func TestResolveTitlePageID(t *testing.T) { } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - title, pageID := resolveTitlePageID(tc.cliTitle, tc.cliPageID, mf) + title, _, pageID := resolveTitlePageID(tc.cliTitle, tc.cliPageID, mf) if title != tc.wantTitle || pageID != tc.wantPageID { t.Errorf("resolveTitlePageID = %q/%q, want %q/%q", title, pageID, tc.wantTitle, tc.wantPageID) @@ -51,7 +51,10 @@ func TestResolveTitlePageIDEmptyWhenAbsent(t *testing.T) { if err != nil { t.Fatal(err) } - title, pageID := resolveTitlePageID("", "", mf) + title, present, pageID := resolveTitlePageID("", "", mf) + if present { + t.Error("titlePresent = true, want false for a file with no frontmatter") + } if title != "" || pageID != "" { t.Errorf("resolveTitlePageID = %q/%q, want empty/empty", title, pageID) } @@ -299,3 +302,83 @@ func TestProcessFileForceBypassesMtimeSkip(t *testing.T) { t.Fatal("want UpdatePage to have been called despite the old mtime") } } + +// TestResolveTitlePageIDSeparatesAbsentFromEmpty pins the distinction the +// empty-title check rests on. An absent title means the file does not manage +// its page's title, which update honours by keeping the live one; a present but +// empty title is a half-finished edit. +func TestResolveTitlePageIDSeparatesAbsentFromEmpty(t *testing.T) { + tests := []struct { + name, content, cliTitle string + wantTitle string + wantPresent bool + }{ + {"absent", "---\npage_id: 1\n---\nb\n", "", "", false}, + {"present and empty", "---\ntitle:\npage_id: 1\n---\nb\n", "", "", true}, + {"present and null", "---\ntitle: null\npage_id: 1\n---\nb\n", "", "", true}, + {"present with value", "---\ntitle: T\npage_id: 1\n---\nb\n", "", "T", true}, + // --title wins, as every other override does, so it satisfies a + // present-but-empty frontmatter title rather than tripping over it. + {"flag over empty", "---\ntitle:\npage_id: 1\n---\nb\n", "CLI", "CLI", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mf, err := frontmatter.Parse("f.md", tc.content) + if err != nil { + t.Fatal(err) + } + title, present, _ := resolveTitlePageID(tc.cliTitle, "", mf) + if title != tc.wantTitle || present != tc.wantPresent { + t.Errorf("resolveTitlePageID = %q/%v, want %q/%v", + title, present, tc.wantTitle, tc.wantPresent) + } + }) + } +} + +// TestProcessFileRejectsEmptyTitle exercises the error path itself, not just +// resolveTitlePageID. The client points at a URL nothing serves: reaching it +// would be the failure, since the check has to fire before any request the way +// the non-numeric page_id check does. +func TestProcessFileRejectsEmptyTitle(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "f.md") + if err := os.WriteFile(path, []byte("---\ntitle:\npage_id: 123\n---\nbody\n"), 0o644); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + c := client.New(client.Config{SiteURL: "https://wiki.invalid"}) + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if r.ok { + t.Fatal("a present-but-empty title must fail the file") + } + if !strings.Contains(r.errMsg, "empty 'title:'") { + t.Errorf("errMsg = %q, want the empty-title sentence", r.errMsg) + } + if r.code != jsonout.CodeValidation { + t.Errorf("code = %q, want %q", r.code, jsonout.CodeValidation) + } +} + +// TestProcessFileKeepsLiveTitleWhenAbsent is the other half: no title key is a +// legitimate shape, and update takes the page's own title. +func TestProcessFileKeepsLiveTitleWhenAbsent(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/pages/123") && r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"id":"123","title":"Live Title","version":{"number":3},` + + `"_links":{"webui":"/spaces/ENG/pages/123/Live"}}`)) + return + } + w.WriteHeader(http.StatusNotFound) + }) + dir := t.TempDir() + path := filepath.Join(dir, "f.md") + if err := os.WriteFile(path, []byte("---\npage_id: 123\n---\nbody\n"), 0o644); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if r.title != "Live Title" { + t.Errorf("title = %q, want the live page's title", r.title) + } +} From ba6c295703481cacdb8342cf8224c54903a8f185 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:28:44 -0400 Subject: [PATCH 4/5] feat(check): report a present-but-empty title as broken check's frontmatter validation is deliberately narrow, and the stated reason is that it cannot know whether the caller is about to create or update, so a false positive is worse than a miss. That reasoning stops covering title once both verbs reject an empty one: there is no verb under which it is valid, so there is no false positive to have, and leaving it out means check passes a file that update then refuses -- the exact failure check exists to catch, without credentials or a network. An absent title stays unreported. update accepts one and keeps the live page's title, so it is a legitimate shape rather than a defect. Reported as broken rather than failed, matching the convention that broken/warnings are document defects while failed is a file that never reached the converter. Collected before the conversion, because a name collision aborts it: gathering the frontmatter check afterwards made it unreachable for exactly the files with two defects. checkResult's schema description enumerates the failed causes and now mentions the YAML and flat-mapping ones as well. --- cmd/check/check.go | 20 +++++++++++++-- cmd/check/check_test.go | 51 ++++++++++++++++++++++++++++++++++++++ schema/json-output/v1.json | 2 +- 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/cmd/check/check.go b/cmd/check/check.go index e6d814f..d8cd355 100644 --- a/cmd/check/check.go +++ b/cmd/check/check.go @@ -140,6 +140,22 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache return r.fail(fmt.Errorf("building the link index: %w", err), jsonout.CodeIO) } + // Collected before the conversion, which can bail out: a frontmatter defect + // is independent of anything the converter finds, and reporting it only when + // the body happens to convert would hide it behind an unrelated failure. + // + // A title that is present and empty is a guaranteed publish failure needing + // no network to see: create and update both reject it. The narrowness + // elsewhere -- never reporting whether page_id/space/parent are set -- holds + // because check cannot know which verb is coming, and that reasoning stops + // applying once both verbs agree. An absent title stays unreported: update + // accepts it and keeps the live page's title. + var frontmatterBroken []string + if title, present := mf.TitleField(); present && title == "" { + frontmatterBroken = append(frontmatterBroken, + "frontmatter has an empty 'title:'; give it a value or remove it") + } + page, err := convert.MdToConfluence(mf, root, index, checkBaseURL, checkSpaceKey, buildinfo.Stamp()) if err != nil { // Two assets wanting one attachment name is a defect in the document, @@ -156,13 +172,13 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache // past a document it has already refused to publish. var collision *convert.NameCollisionError if errors.As(err, &collision) { - r.broken = []string{collision.Error()} + r.broken = append(frontmatterBroken, collision.Error()) r.status = statusBroken return r } return r.fail(err, jsonout.CodeConvert) } - r.broken = page.Broken + r.broken = append(frontmatterBroken, page.Broken...) r.warnings = page.Warnings if showHTML { r.debugHTML = page.HTML diff --git a/cmd/check/check_test.go b/cmd/check/check_test.go index d36f78e..c7abb8d 100644 --- a/cmd/check/check_test.go +++ b/cmd/check/check_test.go @@ -341,3 +341,54 @@ func TestNeverImportsClient(t *testing.T) { } } } + +// TestRunEmptyTitleIsBroken pins that a present-but-empty title is reported. +// The narrowness elsewhere -- check never reports whether page_id/space/parent +// are set -- rests on check not knowing whether create or update is coming, and +// that reasoning stops applying to title once both verbs reject an empty one. +func TestRunEmptyTitleIsBroken(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "main.md"), "---\ntitle:\npage_id: 1\n---\n# Main\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error", err) + } + if !strings.Contains(out, "empty 'title:'") { + t.Errorf("output = %q, want the empty-title message", out) + } +} + +// TestRunAbsentTitleIsNotReported is the other half: a file with no title key is +// the normal shape for update, which keeps the live page's title. +func TestRunAbsentTitleIsNotReported(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "main.md"), "---\npage_id: 1\n---\n# Main\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if err != nil { + t.Fatalf("run = %v, want success", err) + } + if strings.Contains(out, "title") { + t.Errorf("output = %q, want no complaint about the absent title", out) + } +} + +// TestRunEmptyTitleReportedEvenWhenConversionFails pins that a frontmatter +// defect is not hidden behind an unrelated one. A name collision aborts the +// conversion, and collecting the title check afterwards made it unreachable. +func TestRunEmptyTitleReportedEvenWhenConversionFails(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "arch", "diagram.png"), "PNG") + write(t, filepath.Join(dir, "ops", "diagram.png"), "PNG") + write(t, filepath.Join(dir, "main.md"), + "---\ntitle:\n---\n![a](arch/diagram.png)\n\n![b](ops/diagram.png)\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error", err) + } + if !strings.Contains(out, "empty 'title:'") { + t.Errorf("output = %q, want the empty-title message alongside the collision", out) + } +} diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index 5493e39..452dc62 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -433,7 +433,7 @@ } }, "checkResult": { - "description": "One checked file. broken/warnings are always [] (never null), matching ConfluencePage's own convention. status=broken means broken is non-empty (frontmatter or converter); status=failed means the file never reached a clean answer at all (unreadable, unterminated frontmatter, bad page_width, non-numeric page_id) -- code is VALIDATION in that case. debug is non-null only when --show-html was passed and the file reached the converter (never on a failed file).", + "description": "One checked file. broken/warnings are always [] (never null), matching ConfluencePage's own convention. status=broken means broken is non-empty (frontmatter or converter); status=failed means the file never reached a clean answer at all (unreadable, unterminated frontmatter, frontmatter that is not valid YAML or not a flat mapping of single-line scalars, bad page_width, non-numeric page_id) -- code is VALIDATION in that case. debug is non-null only when --show-html was passed and the file reached the converter (never on a failed file).", "type": "object", "additionalProperties": false, "required": ["ok", "status", "file", "broken", "warnings", "debug", "error", "code"], From 96697be773beb988e66580092b8af85c33e98cd4 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:28:54 -0400 Subject: [PATCH 5/5] docs: add C2 (frontmatter-is-valid-yaml) C2 sits in Conformance rather than Laws because that section is defined as agreement with an external specification, and because its own note says C1 is the guarantee that could in principle be traded away. markfluence did trade YAML conformance away, for years, which is what #130 is. Kept separate from L7 (output-is-valid-markdown) because the two are checked against different specs. A file with broken frontmatter still renders as markdown -- GitHub shows it, and it was VSCode's YAML extension that complained -- so folding this into L7 would leave its status ambiguous about which spec had failed. Holds, with three limits stated rather than papered over: typing is ours even though quoting is goccy's; the flat contract is enforced on read because goccy's re-emission of a parsed node is not identity; and the verification is a self-check -- it proves goccy can re-read what goccy wrote, not that another implementation can. There is deliberately no second YAML library in go.mod to settle that last one, so a divergence reported by a real tool is an issue to fix, with the frontmatter as evidence. README's frontmatter section described our own quoting rules; it now describes YAML's, and says which values need quoting when hand-writing a block. --- CLAUDE.md | 4 ++-- README.md | 26 ++++++++++++++++--------- docs/guarantees.md | 48 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8178179..20e649b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,7 +51,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `cmd/root.go` — the cobra root: `--url`/`--username`/`--debug`/`--no-color` persistent flags, version from `internal/buildinfo`, and registration of every subcommand. `Execute()` prints cobra-generated errors (bad args/flags) but not `ui.ErrSilent`, which marks a failure a command already reported. - `cmd/{update,create,fix,check,info,read,export,children,find,search}/` — one package per command (each exports `Cmd`), orchestrating the `internal` packages and `internal/ui` output. `create` is two-phase and transactional (validate all, then create parents-first in topological order); `fix` is read-only on the server; `check` is read-only on both the server and disk and touches neither, since it never constructs one (see its own bullet below). `create` accepts a **page or a folder** as `parent`: `checkParentInSpace` asks the page route, then `/folders/{id}`, and finding nothing as a page proves nothing until both have been asked — that single missing fallback was #68, since Confluence itself accepts a folder `parentId` with no other accommodation. It also validates a frontmatter `page_id` **first** (before space/parent/title lookups: most specific error, three fewer API calls) and treats all three outcomes as failures — non-numeric, resolves to nothing, or already taken. The middle one is the bug from #18: publishing anyway would create a second page and overwrite the id, so an id that can't be explained is never "create a new page". Both "already exists" errors (page_id taken, title clash) link the page in the way; the `pageIDFailure` typed error carries `page_id`/`url` so `--json` reports them as fields, which is the only case where a failed result names a page. -- `cmd/check/` — `check` (#42): validate one or more markdown FILEs against the converter and frontmatter rules with **no network access, no credentials, and no writes** — the first command whose `run()` never constructs a `client.ConfluenceClient` (`root.go`'s `PersistentPreRunE` doesn't force one into existence either, so nothing upstream requires it). It builds `root`/`index` per file exactly like `update`/`create` (`internal/project.Cache`/`internal/linkindex.Cache`), against hardcoded `baseURL`/`spaceKey` (the regression suite's own `https://wiki.example.net`/`ENG`) rather than flags — both are read only to build a rewritten doc-link's *text*, and nothing in `Broken`/`Warnings` reads either, so hardcoding them costs nothing and makes `check` byte-identical across machines. Frontmatter validation is deliberately narrow: an unparseable/unterminated block (`frontmatter.ErrUnterminatedFrontmatter`), an invalid `page_width` (`pagewidth.Declared`), a present-but-non-numeric `page_id` (`pageref.IsDigits`) — never whether `page_id`/`space`/`parent` are set at all, since `check` cannot know whether the caller is about to `create` or `update`, and a false positive there is worse than a miss. A `broken` result is `ok: false` with `error`/`code` both left `null`: unlike every other failure, `broken`/`warnings` already say everything there is to say, so `code: VALIDATION` is reserved for `status: failed` (a file that never reached the converter at all). `--show-html` surfaces `ConfluencePage.HTML`/`Attachments` — nothing else in the CLI ever prints either — as `debug: {html, attachments} | null`; `html` stays compact/unindented in `--json` (matching what `update`/`create` would literally publish) while human output indents it by nesting depth (`indentHTML`, a per-line indent based on tag-open/close counting, not a whitespace-normalizing reformat, since the renderer already breaks lines at every structural boundary and reformatting within a line could alter meaningful inline text). +- `cmd/check/` — `check` (#42): validate one or more markdown FILEs against the converter and frontmatter rules with **no network access, no credentials, and no writes** — the first command whose `run()` never constructs a `client.ConfluenceClient` (`root.go`'s `PersistentPreRunE` doesn't force one into existence either, so nothing upstream requires it). It builds `root`/`index` per file exactly like `update`/`create` (`internal/project.Cache`/`internal/linkindex.Cache`), against hardcoded `baseURL`/`spaceKey` (the regression suite's own `https://wiki.example.net`/`ENG`) rather than flags — both are read only to build a rewritten doc-link's *text*, and nothing in `Broken`/`Warnings` reads either, so hardcoding them costs nothing and makes `check` byte-identical across machines. Frontmatter validation is deliberately narrow: an unparseable/unterminated block (`frontmatter.ErrUnterminatedFrontmatter`, plus everything real YAML now refuses — a nested value, a `|` block, a duplicate key, a tab indent, a reserved indicator), an invalid `page_width` (`pagewidth.Declared`), a present-but-non-numeric `page_id` (`pageref.IsDigits`), and a **present-but-empty `title`** — never whether `page_id`/`space`/`parent` are set at all, since `check` cannot know whether the caller is about to `create` or `update`, and a false positive there is worse than a miss. `title` is the one exception to that reasoning and only in its present-but-empty form: `create` and `update` both reject it, so no verb makes it valid and there is no false positive to have. An *absent* title stays unreported, since `update` accepts one and keeps the live page's title. A `broken` result is `ok: false` with `error`/`code` both left `null`: unlike every other failure, `broken`/`warnings` already say everything there is to say, so `code: VALIDATION` is reserved for `status: failed` (a file that never reached the converter at all). `--show-html` surfaces `ConfluencePage.HTML`/`Attachments` — nothing else in the CLI ever prints either — as `debug: {html, attachments} | null`; `html` stays compact/unindented in `--json` (matching what `update`/`create` would literally publish) while human output indents it by nesting depth (`indentHTML`, a per-line indent based on tag-open/close counting, not a whitespace-normalizing reformat, since the renderer already breaks lines at every structural boundary and reformatting within a line could alter meaningful inline text). - `cmd/export/` — `export`: `pagedoc` for the body, `attachfile` for the attachments. **`--depth` exports a subtree** (`0` default / a number / `all`), **`--space KEY` a whole space** (requiring an explicit `--depth`, since the default would export nothing and defaulting to `all` would make a typo walk a whole space), and a **folder** may be the target — a folder and a space have no file of their own, so their children become the top level, which is why `layout`'s `rootRef` carries the id children hang off *separately* from whether anything is written for it: the walk's top-level nodes report a folder as their parent but report nothing for a space, and conflating the two placed every page at the destination root. `layout` owns every path an export writes (mirrored hierarchy: `.md` plus a `/` for children and unrecorded attachments, a folder as a bare directory) and the `-` suffix for a group of siblings that slug the same — applied to *every* member so a filename never depends on walk order, and disambiguating rather than refusing because a space nobody can retitle would otherwise be unexportable over a punctuation variant (an exported filename is ergonomic; identity is `page_id`, per L8). `parent:` is a relative path to the parent's own `.md` so the tree publishes into fresh pages, except for the export root and a page whose parent is a folder, which keep an id. Two things the layout buys that are easy to miss: page directories are unique, which is what makes page-scoped attachment placement collision-free — so `pagedoc.Placement` must carry the *disambiguated* directory (`AttachmentDirFor`), or two colliding siblings silently share one attachment file, which no checksum catches because a native attachment has none. `destClaims` reserves every page's destination before any attachment is written, since a recorded `path=` is server data that can name a page's own file and a parent's attachments are written before its children exist — otherwise the attachment lands first and the page is reported `skipped (exists)`. A page already on disk skips its *render* but not its attachment pass, so a retry resumes a run that died mid-download. `markfluence.yaml` is planted at `dest` for a multi-page export **before the first page**, because a partial tree with no marker republishes every shared asset as `IMAGE BROKEN`. Markdown only. An attachment with a recorded `path=` lands there; one without is page-scoped. Still no `--attachments-dir`, but for a different reason than before the naming change: moving an asset no longer renames its attachment, so it is no longer unsafe — it would simply reintroduce the collision a base name has to refuse, since two pages' `diagram.png` cannot share a directory. Only referenced attachments are exported, found by scanning raw storage for `ri:filename` (not just `ac:image`, which is all the converter special-cases, so a link target or a macro-internal reference would otherwise be dropped). A reference with no attachment is a warning, not a failure. - `cmd/children/` — `children`: list the pages and folders under a page or folder, via `internal/pagetree`. `--depth` is a **string** vocabulary (a positive number or `all`, default `1`), not an int: `all` is not a number, and `0` is refused rather than read as "unlimited" the way it is elsewhere, because silently walking a whole space for someone who meant "none" is worse than an error that names `all`. Folder rows are emitted with a `type` column, which is what makes "a folder counts as a level" safe. Empty is a success: `No children.` and exit 0. **`--space KEY` lists a whole space instead of a page** (#98), which makes `PAGE` optional — exactly one of the two, checked before credentials. Depth 1 is then the space's **root pages**, not the homepage's children: a space can have several roots (`create` with a null parent makes one), so seeding the walk from `homepageId` would drop a root and its whole subtree, and there is no root-level *folder* to miss because a folder created with no parent lands under the homepage ([docs/confluence/spaces.md](docs/confluence/spaces.md)). A root row's `parent_id` is `null` — the one place `childrenResult` needs `stringOrNull` for it — since a space is not a node. The key is resolved through `ResolveSpaceID` before the walk even though the v1 route it feeds takes a key: an unknown key must fail as a typo (exit 2) the way it does for `find`/`search`, and the v1 route reports one as a 404, which is also what a rejected credential looks like. Because a space's top level is usually one row, human output adds a `--depth` reminder when `--depth` was left at its default — on **stderr**, via `ui.Hint`, so the table stays pipeable; `--json` never sees it. A failing space walk likewise reports an `errorObject` on stderr rather than a `results[0]` failure, since `SingleOpFailure.page_id` would otherwise carry a space key. - `cmd/find/` — `find`: resolve a title to the ids carrying it, via `client.FindByTitle`. A title is the one handle `internal/pageref` cannot resolve. It reports **current pages, archived pages, and folders**, which takes two requests because no single API sees all three — and the three-way split is the thing to keep straight before touching it ([docs/confluence/search.md](docs/confluence/search.md)). An **archived** page is reported, with a `status` column, because it is absent from the page tree yet still reserves its title; a **folder** is reported because a folder id is a legitimate `parent`, but a folder reserves nothing, so a folder row must never be treated as a naming conflict. `--space` is a space **key**, and an unknown one is a hard error rather than an empty result — CQL answers an unknown key with zero rows, which reads exactly like "no such page". Either half failing fails the whole command: a partial answer reads as "nothing found", and the caller's next move on that is to create a duplicate. Empty is a success: `No matches found.` and exit 0. Its operational failure is an `errorObject` on stderr rather than a `results[0]` entry — there is no page id to name — which it shares with `search` and with `children --space`, and nothing else. @@ -66,7 +66,7 @@ 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`. A 403 that is not one of the two measured credential phrasings gets no hint, because that is what a genuine permission denial looks like ([docs/confluence/api.md](docs/confluence/api.md#scopes)). **Retry rules**: 429 for any method; 502/503/504 for idempotent methods; **any other 5xx only when the response carries `Retry-After`** — that is how a 500 becomes retryable, and it is why `parseRetryAfter` reports the header's *presence* apart from its delay (`Retry-After: 0` means "retry now", not "no header"). The exponential delay is jittered, a server-supplied `Retry-After` never is. Decisions go to a package-level hook (`SetRetryLogger`, set once in `root.go` beside `ui.SetDebug`) and fire whichever way they went, because `internal/client` prints nothing and a silent twelve-minute retry storm is otherwise indistinguishable from a hang. **A versioned PUT is not as idempotent as its method**: `SetContentProperty` retry-once on top (recovers a lost create-POST response) and `UpdatePage`'s `updateLanded` both exist for the same reason — a write whose response was lost gets re-sent, and the re-sent version is refused. `updateLanded` requires version *and* title *and* body to match what was sent, since a concurrent edit could have produced the version alone and claiming success over someone else's content is worse than a false failure ([docs/confluence/api.md](docs/confluence/api.md)). `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; only the current comment form is parsed — an attachment stamped by a markfluence predating a comment-format change reads as unmanaged and is re-uploaded once, the same as any hand-uploaded file — except that a *recorded path disagreeing with the local source* is an update even when the checksum matches, so a mangled path repairs itself instead of surviving every later publish; a comment with no source recorded at all is not a disagreement. Every text part of the upload form must go through `writeTextField`, never `multipart.Writer.WriteField`, which emits no charset and gets decoded as Latin-1), `_links.next` pagination. **Three pagination schemes, and picking the wrong one truncates silently.** v1 *child/attachment* collections page through the generic `listV1` helper by `start`/`limit` offset, never `_links.next` (absent when the results fit one page, so it cannot terminate a loop); `ListAttachments`, `ListChildPages`, and `ListChildFolders` all go through it. v2 collections page through `listV2` by the cursor in `_links.next`, which is a `/wiki`-prefixed absolute path `resolveNext` handles unchanged; `ListContentProperties` and `SearchPagesByTitle` share it. **`/wiki/rest/api/search` is neither**: it ignores `start` outright, its `next` is context-relative so it needs the `/wiki` prefix `resolveNext` does not add, a short page does *not* mean the end, and `totalSize` can be nonzero against an empty `results` — so `searchCQL` terminates only on a missing `next` and nothing may branch on `totalSize` ([docs/confluence/search.md](docs/confluence/search.md)). `searchCQLBounded` adds a row bound under it (`SearchCQL` is that call with no bound, which is why `find` is unaffected): it asks for `max+1` and reports the surplus as `more`, since `totalSize` cannot supply a count. Full text goes through `SearchText`/`SearchRawCQL`, which return the cleaned `SearchMatch` the way `FindByTitle` returns `TitleMatch` — and **every field of a match comes from the row's `content` object**, because the row-level `title` is HTML-escaped *and* wrapped in `@@@hl@@@` markers where `content.title` is neither. The `excerpt` exists only at row level, so `cleanExcerpt` strips those markers, unescapes once, and collapses to one line — in the client, so the human and `--json` paths cannot disagree about it. `excerpt=highlight` is passed explicitly and **re-attached when following the cursor** (the `next` link carries `cql` and `limit` but not `excerpt`, and `doJSON` appends params with a bare `?`); an unrecognized value there yields an empty excerpt with a 200, so a rename by Atlassian degrades to no excerpts rather than an error. A row with no `content` object is skipped and **counted** — `type = space` answers with hundreds of them, and a silent skip would report a successful empty result. A bare v1 child row already carries `webui`, `status`, and `extensions.position`, so child listing needs no `expand`. `DownloadAttachment` goes through `send` (inheriting retry/backoff) against `_links.download`; **never** add a `CheckRedirect` that forwards headers — it would leak site credentials to Atlassian's media host, which neither needs nor wants them. `config.go` holds `Resolve` and the `.env` reader. Why each of these is shaped this way, with the evidence: [docs/confluence/api.md](docs/confluence/api.md) and [attachments.md](docs/confluence/attachments.md). - `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, root *project.Root, index *linkindex.Index, baseURL, spaceKey, version string) (*ConfluencePage, error)`. `root` bounds which images and parent references may be read (S1/S2) and is what an image's recorded `Source` is relative to; `index` is the tree-wide link/anchor index for `root` (`internal/linkindex.Build`), built once and shared across every file converted under it rather than rebuilt per conversion — both are discovered/built by the caller (`internal/project`/`internal/linkindex`), which is why this package stays client-free. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `aclink.go` is the *inverse* direction's one element with enough shape to need its own file — ``, which the editor writes for every internal link and `MdToConfluence` never emits, so nothing in the regression suite covers it. One rule decides its whole mapping: **convert when the markdown republishes to a link resolving to the same target, pass the storage through when it would not** — so a page link and a space link convert, while a mention (80% of all real usage), an attachment link (only images are uploaded, so a relative href would be dead) and an unresolvable target stay raw, which the shield republishes byte-identical. A page target is a **title, never an id**, so `PageLinkTargets` reports what needs resolving and `StorageOptions.PageLinks` carries the answers back. An `ac:anchor` is **percent-encoded** where `confluenceSlug` output is not: decode it before matching a heading, leave it encoded inside a URL. A same-page anchor recovers its heading from the document rather than inverting the slug, which is impossible — `confluenceSlug` turns both a space and a hyphen into `-`. The survey the mapping rests on, and the `xml.HTMLAutoClose` trap that made `` crash the parser outright (#88), are in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); `attachname.go` owns the source-path→attachment-name mapping, 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 `