From 3067016fb1ab016e7c0b4e35841a204b67c91bef Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:29:58 -0400 Subject: [PATCH 1/5] docs: plan for normalizing frontmatter field order Split out of _plans/032, which established the decisions this rests on. 032's surgical UpdateField is what removes ordering as a side effect of every write; this is the explicit replacement for the two commands that rewrite frontmatter wholesale anyway. Separate because it is a feature rather than a consequence of #130, it carries the only --json contract change across both plans, and bundling the two buried the part that fixes the bug. --- _plans/033_frontmatter-field-order.md | 171 ++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 _plans/033_frontmatter-field-order.md diff --git a/_plans/033_frontmatter-field-order.md b/_plans/033_frontmatter-field-order.md new file mode 100644 index 0000000..5001243 --- /dev/null +++ b/_plans/033_frontmatter-field-order.md @@ -0,0 +1,171 @@ +# Plan: normalize frontmatter field order + +Make `fix` and `create` leave a file's frontmatter in canonical field order, +reported as `reordered`. Split out of `_plans/032`, which established the +decisions this rests on; read its Decisions section first. + +## Why this is separate + +032 replaced the hand-rolled frontmatter parser with `goccy/go-yaml` (#130). One +of its decisions was that `UpdateField` becomes **surgical** -- an existing key +keeps its position and only its value node is replaced -- where the old writer +rewrote the whole block in canonical order on every single write. + +That is the right default for an edit. These files live in git, and a write +should not churn lines nobody asked it to touch. But it means ordering is no +longer normalized as a side effect, and stable ordering is worth having, so the +two commands that already rewrite frontmatter wholesale do it explicitly. + +Kept out of 032 for three reasons. It is a feature rather than a consequence: +nothing about #130 requires it. It carries the only `--json` contract change in +either plan, which is the part a consumer can be broken by. And bundled together +the two came to ~600 lines against ~400 for the parser swap alone, which buried +the thing that actually fixes the bug. + +## Current state of the codebase + +After 032: + +- `frontmatter.UpdateField(content, key, value, comment) (string, error)` is + surgical: replace in place, or insert before the first key that sorts after + it. `keyLess` is the canonical comparator, from `fieldOrder` + (`title, space, parent, page_id`, then the rest alphabetically). +- `frontmatter.Render(fields) string` builds a block from scratch, already in + canonical order, and cannot fail. +- `cmd/fix`'s `processFile` plans changes, returns `statusConsistent` with **no + write** when `plannedChanges` is empty, and otherwise applies each `change` + through `UpdateField`. +- `fixResult` carries `changes []change` where `change` is + `{field, oldDisplay, newValue}`; `jsonFixResult` mirrors it. +- `cmd/create`'s `writeBackFrontmatter` sets all five persisted fields through + `UpdateField`. +- `README.md` says `fix` "writes a file only when a field actually changed". + +## Decisions + +These were all settled while designing 032; the reasoning is repeated here +because this is the plan that implements them. + +**`fix` normalizes by default, with no flag.** Adding `--normalize` or a +separate `fmt`-style verb would keep "reconcile with the server" and "tidy the +file" conceptually apart, which is cleaner on paper. It is not worth a flag +nobody would remember to pass: a tidy-ordering feature you have to opt into +leaves the files untidy. + +**A reorder-only file is `changed`, not `consistent`.** Otherwise you run `fix`, +are told there is nothing to do, and still have a jumbled file. It also keeps +`--dry-run` a faithful preview, which this codebase protects elsewhere: a +dry-run's per-file output is identical to a real run apart from the banner. + +The cost, stated plainly: `fix` now touches files it previously left alone, so +the first run after this lands produces a diff across the tree. `README.md`'s +"writes a file only when a field actually changed" becomes false and is +rewritten -- it describes today's behaviour rather than promising anything, and +it is not in `docs/guarantees.md`, where the load-bearing promises live. + +**Reported as a dedicated `reordered: boolean` on `fixResult`,** not a +pseudo-entry in `changes[]`. `changes[].field` is an actual frontmatter key +name everywhere else, built from real fields; a non-field there makes the slot +polymorphic, and a consumer doing `changes | map(.field)` gets a phantom key it +has to know to filter. The existing `"(none)"` sentinel lives in `old`, which is +explicitly a *display* slot (`oldDisplay` in the struct) -- `field` is an +identity slot, which is a different thing. Costs a schema edit; that is what the +schema is versioned for. + +No equivalent field on `createResult`: `create --persist` writes all five +fields, so its output is always canonical and there is nothing to report. + +**`Normalize` is a no-op when the order already holds.** This is what makes the +boolean mean exactly "keys moved", and it is what keeps the blank-line handling +predictable -- blank lines are dropped only as a consequence of a real reorder, +never as a side effect of some unrelated field being written. A canonical file +keeps its blank lines: this normalizes ordering, it is not a formatter. + +**Blank lines are dropped textually, and comments travel with their key.** A +blank line is not a node -- it lives in the preceding value's token origin -- so +reordering carries it to a position that means nothing. Dropping them is a text +filter over the emitted block, which is safe only because 032 refuses a +multi-line scalar: nothing this package emits spans more than one line, so a +blank line in the output is always a real blank line. + +Comments travelling is better than the old writer's hoist-every-comment-to-the- +top: a comment about `page_id` belongs next to `page_id`. Not claimed to be +free -- a block-header comment written above a key that is not `title` sinks +with that key. Visible in the diff, and accepted. + +**`create --persist` normalizes too.** The minimal-diff argument behind a +surgical `UpdateField` does not apply there: persist rewrites all five fields by +definition, so there is no untouched line left to protect. It also means the +frontmatter markfluence *authors* is always canonical, rather than "canonical +unless it came from a jumbled file and you have not run fix yet". + +## Implementation + +### `internal/frontmatter` + +- `Normalize(content string) (string, bool, error)` -- returns content unchanged + with `false` when `keyLess` order already holds, and likewise for content with + no frontmatter block. Otherwise sorts `MappingNode.Values` by `keyLess`, drops + blank lines, and returns `true`. +- `isCanonical(*ast.MappingNode) bool` -- an adjacent-pairs check, which equals + global sortedness because the parser rejects duplicate keys. +- `dropBlankLines(string) string` -- the text filter, with the safety argument + above in its doc comment. + +### `cmd/fix` + +- `fixResult` gains `reordered bool`; `jsonFixResult` gains + `Reordered bool \`json:"reordered"\``. +- `processFile` calls `Normalize` on `mf.Content` during planning and stores the + boolean. The `len(r.changes) == 0` early return becomes + `len(r.changes) == 0 && !r.reordered`. +- The write path applies each `change` through `UpdateField`, then `Normalize` + **last**, so a key inserted above lands canonically rather than wherever the + surgical insert put it. +- Computing `reordered` on pre-change content is stable for two reasons, not + one: a surgical `UpdateField` never moves an existing key, *and* inserting + before the first key that sorts after it cannot flip canonicity in either + direction -- an existing inversion survives the insert, and a canonical + sequence stays canonical. +- Human output gains one line, `normalized frontmatter field order`, printed + before the per-field lines. + +### `cmd/create` + +`writeBackFrontmatter` runs `Normalize` after its five `UpdateField` calls, and +discards the boolean: there is nothing to report. + +### `schema/json-output/v1.json` + +`fixResult` gains `reordered` in `properties` and in `required` +(`additionalProperties: false` needs both). + +## Tests + +- **`Normalize`**: reorders a jumbled block and drops its blank lines; no-op on + a canonical block, *including one that has a blank line* (the case that pins + "ordering, not formatting"); no-op on content with no block; a full-line + comment stays attached to its key across a reorder. +- **`cmd/fix`**: a file whose values all match its page but whose fields are + jumbled is `changed` with `reordered: true`, `changes` empty, and is written + in canonical order; `--dry-run` reports it without writing; a canonical + consistent file still reports `consistent` and is not written. +- **`cmd/create`**: persist output is canonical from jumbled input. +- **Schema conformance**: `fixResult` with `reordered`, built through the + command's own `jsonResult()`. + +## Docs + +- `README.md` -- the `fix` section: drop "writes a file only when a field + actually changed", add order normalization and `reordered`. +- `CLAUDE.md` -- the `fix` sentence, and the `internal/frontmatter` bullet's + entry-point list (`Normalize` becomes the third). + +## Out of scope + +- **`check` reporting a jumbled file.** Ordering is not a publishability defect, + and `check` is about what would fail a publish. +- **`update` normalizing.** It never writes back to files, and that stays true. +- **Normalizing anything else about the block** -- intra-line whitespace, blank + lines in a canonical file, comment placement. `fix` orders fields; it is not + `gofmt` for frontmatter. From 7497fee88eceaa8dc81574dbba577eda5dcd1d73 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:30:34 -0400 Subject: [PATCH 2/5] feat(frontmatter): add Normalize Reorders a block into canonical field order, reporting whether anything moved. Nothing calls it yet. It is a no-op when the order already holds, which is what makes the reported boolean mean exactly "keys moved" -- and what keeps the blank-line handling predictable: blank lines are dropped only as a consequence of a real reorder, never as a side effect of some unrelated field being written. A canonical file keeps its blank lines; this normalizes ordering, it is not a formatter. Blank lines are dropped textually rather than structurally because a blank line is not a node: it lives in the preceding value's token origin, so reordering carries it to a position that means nothing. A text filter is only safe because reads refuse a multi-line scalar, so nothing this package emits spans more than one line and a blank line in the output is always a real one. isCanonical checks adjacent pairs, which equals global sortedness because the parser rejects duplicate keys. Comments travel with their key across a reorder, which is better than the old writer hoisting every comment to the top of the block -- a comment about page_id belongs next to page_id. Not free: a block-header comment written above a key other than title sinks with that key. Visible in the diff, and accepted. --- internal/frontmatter/frontmatter.go | 55 ++++++++++++++++++++++++ internal/frontmatter/frontmatter_test.go | 51 ++++++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/internal/frontmatter/frontmatter.go b/internal/frontmatter/frontmatter.go index 7cb2831..8f9d90b 100644 --- a/internal/frontmatter/frontmatter.go +++ b/internal/frontmatter/frontmatter.go @@ -421,6 +421,61 @@ func setField(b *block, key, value, comment string) { b.mapping.Values[at] = mv } +// Normalize rewrites content's frontmatter in canonical field order, reporting +// whether anything moved. +// +// It is a no-op when the order already holds, which is what makes the reported +// boolean mean exactly "keys moved" and keeps the blank-line handling +// predictable: blank lines are dropped only as a consequence of a real reorder, +// never as a side effect of some unrelated field being written. A canonical +// file keeps its blank lines -- this normalizes ordering, it is not a formatter. +func Normalize(content string) (string, bool, error) { + loc := frontmatterRE.FindStringSubmatchIndex(content) + if loc == nil { + return content, false, nil + } + b, err := parseBlock(content[loc[2]:loc[3]]) + if err != nil { + return "", false, err + } + if isCanonical(b.mapping) { + return content, false, nil + } + sort.SliceStable(b.mapping.Values, func(i, j int) bool { + return keyLess(b.mapping.Values[i].Key.GetToken().Value, + b.mapping.Values[j].Key.GetToken().Value) + }) + return "---\n" + dropBlankLines(b.mapping.String()) + "\n---\n" + content[loc[1]:], true, nil +} + +// isCanonical reports whether a mapping's keys are already in keyLess order. +func isCanonical(m *ast.MappingNode) bool { + for i := 1; i < len(m.Values); i++ { + if keyLess(m.Values[i].Key.GetToken().Value, m.Values[i-1].Key.GetToken().Value) { + return false + } + } + return true +} + +// dropBlankLines removes blank lines from an emitted block. +// +// Textual rather than structural because a blank line is not a node: it lives in +// the preceding value's token origin, so reordering carries it to a position +// that means nothing. Safe as a text filter because nothing this package emits +// spans more than one line -- a value containing a newline is written as a +// double-quoted scalar with an escape, never as a "|" block. +func dropBlankLines(s string) string { + lines := strings.Split(s, "\n") + kept := lines[:0] + for _, line := range lines { + if strings.TrimSpace(line) != "" { + kept = append(kept, line) + } + } + return strings.Join(kept, "\n") +} + // --- MarkdownFile --------------------------------------------------------------- // MarkdownFile is a markdown source file parsed once: its path, raw text, diff --git a/internal/frontmatter/frontmatter_test.go b/internal/frontmatter/frontmatter_test.go index 468fa5d..948a7f0 100644 --- a/internal/frontmatter/frontmatter_test.go +++ b/internal/frontmatter/frontmatter_test.go @@ -309,6 +309,57 @@ func TestUpdateFieldKeepsACommentOnlyBlocksNote(t *testing.T) { } } +// --- Normalize ------------------------------------------------------------------- + +func TestNormalizeReordersAndDropsBlanks(t *testing.T) { + in := "---\npage_width: max\npage_id: 9\ncustom: z\n\nparent: 4\nspace: ENG\ntitle: T\n---\nbody\n" + got, reordered, err := frontmatter.Normalize(in) + if err != nil { + t.Fatal(err) + } + if !reordered { + t.Error("reordered = false, want true") + } + want := "---\ntitle: T\nspace: ENG\nparent: 4\npage_id: 9\ncustom: z\npage_width: max\n---\nbody\n" + if got != want { + t.Errorf("Normalize =\n%q\nwant\n%q", got, want) + } +} + +// TestNormalizeIsANoOpWhenCanonical is what makes "reordered" mean exactly "keys +// moved", and is why a canonical file keeps its blank lines: normalize orders +// fields, it is not a formatter. +func TestNormalizeIsANoOpWhenCanonical(t *testing.T) { + for _, in := range []string{ + "---\ntitle: T\nspace: ENG\npage_id: 9\n---\nbody\n", + "---\ntitle: T\n\npage_id: 9\n---\nbody\n", + "# no frontmatter at all\n", + } { + got, reordered, err := frontmatter.Normalize(in) + if err != nil { + t.Fatal(err) + } + if reordered { + t.Errorf("Normalize(%q) reordered = true, want false", in) + } + if got != in { + t.Errorf("Normalize(%q) = %q, want it unchanged", in, got) + } + } +} + +func TestNormalizeKeepsACommentWithItsKey(t *testing.T) { + in := "---\npage_id: 9\n# about the title\ntitle: T\n---\nbody\n" + got, _, err := frontmatter.Normalize(in) + if err != nil { + t.Fatal(err) + } + want := "---\n# about the title\ntitle: T\npage_id: 9\n---\nbody\n" + if got != want { + t.Errorf("Normalize =\n%q\nwant\n%q", got, want) + } +} + // --- MarkdownFile accessors ----------------------------------------------------- func TestMarkdownFileAccessors(t *testing.T) { From bdef33a50fd8cd2dfaeff41fb86a55888932a5a0 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:30:35 -0400 Subject: [PATCH 3/5] feat(fix): normalize frontmatter field order, reported as reordered 032's surgical UpdateField no longer rewrites the whole block in canonical order as a side effect of writing one field. That is the right default for an edit -- these files live in git and a write should not churn lines nobody asked it to touch -- but stable ordering is still worth having, so the commands that already rewrite frontmatter wholesale do it explicitly. A file whose values all match its live page but whose fields are jumbled is changed, not consistent. Reporting it consistent would mean running fix, being told there is nothing to do, and still having a jumbled file. It also keeps --dry-run a faithful preview. Reported as a dedicated boolean rather than an entry in changes[]: changes[].field is an actual frontmatter key everywhere else, and a non-field there makes the slot polymorphic for any consumer grouping by it. The existing "(none)" sentinel lives in old, which is a display slot; field is an identity slot. Normalize runs last in the write path, so a key inserted above lands canonically rather than wherever the surgical insert put it. Computing reordered on pre-change content is stable for two reasons, not one: a surgical UpdateField never moves an existing key, and inserting before the first key that sorts after it cannot flip canonicity in either direction. The cost, stated plainly: fix now touches files it previously left alone, so the first run after this lands produces a diff across the tree. --- cmd/fix/fix.go | 19 ++++++++++++- cmd/fix/fix_test.go | 30 +++++++++++++++++++- cmd/fix/json.go | 56 +++++++++++++++++++++----------------- cmd/fix/json_test.go | 1 + schema/json-output/v1.json | 6 +++- 5 files changed, 84 insertions(+), 28 deletions(-) diff --git a/cmd/fix/fix.go b/cmd/fix/fix.go index d8e1c50..0e9749e 100644 --- a/cmd/fix/fix.go +++ b/cmd/fix/fix.go @@ -128,7 +128,18 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult { } r.changes = plannedChanges(mf.Frontmatter, page, liveWidth) - if len(r.changes) == 0 { + // Field order is reconciled too, and counts as a change: reporting a + // jumbled file "consistent" would mean running fix, being told there is + // nothing to do, and still having a jumbled file. Computed before any edit, + // which is stable because a surgical UpdateField never moves an existing key + // and inserting before the first key that sorts after it cannot flip + // canonicity either way. + _, reordered, err := frontmatter.Normalize(mf.Content) + if err != nil { + return r.fail(err, jsonout.CodeValidation) + } + r.reordered = reordered + if len(r.changes) == 0 && !r.reordered { r.ok = true r.status = statusConsistent return r @@ -146,6 +157,12 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult { return r.fail(err, jsonout.CodeValidation) } } + // Last, so a key inserted above lands in canonical position rather than + // wherever the surgical insert put it. + content, _, err = frontmatter.Normalize(content) + if err != nil { + return r.fail(err, jsonout.CodeValidation) + } if err := os.WriteFile(filename, []byte(content), 0o644); err != nil { return r.fail(err, jsonout.CodeIO) } diff --git a/cmd/fix/fix_test.go b/cmd/fix/fix_test.go index c3a434e..8354177 100644 --- a/cmd/fix/fix_test.go +++ b/cmd/fix/fix_test.go @@ -278,7 +278,7 @@ func fixServer(t *testing.T, page string, widthProperty string) *client.Confluen } func TestProcessFileConsistentDoesNotWrite(t *testing.T) { - content := "---\npage_id: 1\nspace: ENG\nparent: null\ntitle: X\npage_width: max\n---\nbody\n" + 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"`) @@ -386,6 +386,34 @@ func TestOrNull(t *testing.T) { } } +// TestProcessFileNormalizesFieldOrder pins that a file whose values all match +// its live page is still rewritten when its fields are out of canonical order, +// and reports that separately from any value change. +func TestProcessFileNormalizesFieldOrder(t *testing.T) { + content := "---\npage_id: 1\nspace: ENG\nparent: null\ntitle: X\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.ok || r.status != statusChanged { + t.Fatalf("result = %+v, want ok/changed", r) + } + if !r.reordered { + t.Error("reordered = false, want true") + } + if len(r.changes) != 0 { + t.Errorf("changes = %+v, want none: only the order differs", r.changes) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "---\ntitle: X\nspace: ENG\nparent: null\npage_id: 1\npage_width: max\n---\nbody\n" + if string(got) != want { + t.Errorf("file =\n%q\nwant\n%q", got, want) + } +} + // 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. diff --git a/cmd/fix/json.go b/cmd/fix/json.go index 1c94c74..07ee762 100644 --- a/cmd/fix/json.go +++ b/cmd/fix/json.go @@ -22,15 +22,16 @@ const noneDisplay = "(none)" // fixResult captures the outcome of reconciling one file. type fixResult struct { - file string - ok bool - status string - pageID string - dryRun bool - changes []change - warnings []string - errMsg string - code jsonout.Code + file string + ok bool + status string + pageID string + dryRun bool + changes []change + reordered bool + warnings []string + errMsg string + code jsonout.Code } func (r *fixResult) fail(err error, code jsonout.Code) *fixResult { @@ -55,6 +56,9 @@ func (r *fixResult) renderHuman() { ui.Info(prefix + " already consistent") return } + if r.reordered { + ui.Info(prefix + " normalized frontmatter field order") + } // The per-field lines are identical in a dry-run; the leading DRY RUN banner // (and dry_run in --json) is the only signal nothing was written. for _, ch := range r.changes { @@ -64,15 +68,16 @@ func (r *fixResult) renderHuman() { // jsonFixResult is fix's --json result shape. type jsonFixResult struct { - OK bool `json:"ok"` - Status string `json:"status"` - File string `json:"file"` - PageID *string `json:"page_id"` - DryRun bool `json:"dry_run"` - Changes []jsonChange `json:"changes"` - Warnings []string `json:"warnings"` - Error *string `json:"error"` - Code *jsonout.Code `json:"code"` + OK bool `json:"ok"` + Status string `json:"status"` + File string `json:"file"` + PageID *string `json:"page_id"` + DryRun bool `json:"dry_run"` + Changes []jsonChange `json:"changes"` + Reordered bool `json:"reordered"` + Warnings []string `json:"warnings"` + Error *string `json:"error"` + Code *jsonout.Code `json:"code"` } // jsonChange is one reconciled field. old is null when there was no prior value. @@ -84,13 +89,14 @@ type jsonChange struct { func (r *fixResult) jsonResult() jsonFixResult { res := jsonFixResult{ - OK: r.ok, - Status: r.status, - File: r.file, - PageID: nullableStr(r.pageID), - DryRun: r.dryRun, - Changes: toJSONChanges(r.changes), - Warnings: nonNilStrings(r.warnings), + OK: r.ok, + Status: r.status, + File: r.file, + PageID: nullableStr(r.pageID), + DryRun: r.dryRun, + Changes: toJSONChanges(r.changes), + Reordered: r.reordered, + Warnings: nonNilStrings(r.warnings), } if !r.ok { res.Error = &r.errMsg diff --git a/cmd/fix/json_test.go b/cmd/fix/json_test.go index bada11d..9519224 100644 --- a/cmd/fix/json_test.go +++ b/cmd/fix/json_test.go @@ -66,6 +66,7 @@ func TestJSONResultChanged(t *testing.T) { "new": "123" } ], + "reordered": false, "warnings": [], "error": null, "code": null diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index 452dc62..745b1a4 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -407,7 +407,7 @@ "fixResult": { "type": "object", "additionalProperties": false, - "required": ["ok", "status", "file", "page_id", "dry_run", "changes", "warnings", "error", "code"], + "required": ["ok", "status", "file", "page_id", "dry_run", "changes", "reordered", "warnings", "error", "code"], "properties": { "ok": { "type": "boolean" }, "status": { "enum": ["changed", "consistent", "failed"] }, @@ -427,6 +427,10 @@ } } }, + "reordered": { + "description": "Whether fix rewrote the frontmatter into canonical field order. Independent of changes: a file whose values all match its live page can still be reordered, and that counts as changed.", + "type": "boolean" + }, "warnings": { "type": "array", "items": { "type": "string" } }, "error": { "$ref": "#/$defs/stringOrNull" }, "code": { "$ref": "#/$defs/codeOrNull" } From 26c28cd98de6b9015cead060c93b337c7938d6aa Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:30:35 -0400 Subject: [PATCH 4/5] feat(create): normalize field order on write-back The minimal-diff argument behind a surgical UpdateField does not apply here: persist rewrites all five fields by definition, so there is no untouched line left to protect. It also means the frontmatter markfluence authors is always canonical, rather than "canonical unless it came from a jumbled file and you have not run fix yet". No result field: create writes every field, so its output is always canonical and there is nothing to report. --- cmd/create/create.go | 8 ++++++-- cmd/create/create_test.go | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 1de0644..6a6f9fc 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -784,7 +784,10 @@ func overrideNeedsSingleFile(cliTitle string, nFiles int) bool { return cliTitle != "" && nFiles != 1 } -// writeBackFrontmatter sets every field create persists. +// writeBackFrontmatter sets every field create persists, then normalizes the +// block's field order. Normalizing here rather than leaving it to fix costs +// nothing: persist already rewrites all five fields, so there is no untouched +// line left for a surgical edit to protect. func writeBackFrontmatter(content string, r record, pageID, parentValue, parentComment string) (string, error) { fields := []struct{ key, value, comment string }{ {"title", r.title, ""}, @@ -799,7 +802,8 @@ func writeBackFrontmatter(content string, r record, pageID, parentValue, parentC return "", err } } - return content, nil + content, _, err = frontmatter.Normalize(content) + return content, err } // resolveTitle returns the effective title: --title overrides the frontmatter. diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index 7039414..7148c32 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -410,6 +410,24 @@ func TestTopoSortOrdersParentsBeforeChildren(t *testing.T) { } } +// TestWriteBackFrontmatterNormalizes pins that persist leaves the block in +// canonical order even when the author's file was not. Unlike an ordinary +// surgical edit there is nothing to protect here: persist rewrites all five +// fields anyway, so there is no untouched line for a minimal diff to preserve. +func TestWriteBackFrontmatterNormalizes(t *testing.T) { + in := "---\npage_id: null\ntitle: My Page\n---\nbody\n" + r := record{title: "My Page", spaceKey: "ENG", width: pagewidth.Max} + + got, err := writeBackFrontmatter(in, r, "123", "null", "") + if err != nil { + t.Fatal(err) + } + want := "---\ntitle: My Page\nspace: ENG\nparent: null\npage_id: 123\npage_width: max\n---\nbody\n" + if got != want { + t.Errorf("writeBackFrontmatter =\n%q\nwant\n%q", got, want) + } +} + // 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} From 687dd5cb917ff662d88c684777c0d1566912a467 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:30:35 -0400 Subject: [PATCH 5/5] docs: fix normalizes frontmatter field order README's fix section promised it "writes a file only when a field actually changed", which order normalization makes false. That sentence described behaviour rather than promising anything, and it is not in docs/guarantees.md where the load-bearing promises live. --- CLAUDE.md | 4 ++-- README.md | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 20e649b..246fb14 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 `