Normalize frontmatter field order in fix and create

README.md

@@ -336,8 +336,11 @@ Usage: markfluence fix FILE... [flags]
 Reconcile each file's frontmatter (`page_id`, `space`, `parent`, `page_width`, and
 a missing `title`) to match its live Confluence page. The page is located by
 `page_id`, or by searching for the `title` when `page_id` is absent. `fix` never
-creates, updates, or moves pages — it's read-only on the server and writes a file
-only when a field actually changed. `--dry-run` reports the changes without writing.
+creates, updates, or moves pages — it's read-only on the server. It writes a file
+when a field changed, and also when the frontmatter fields are out of canonical
+order (`title`, `space`, `parent`, `page_id`, then the rest alphabetically),
+which is reported separately as `reordered`. `--dry-run` reports both without
+writing.
 
 ```sh
 markfluence fix docs/*.md

_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.

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.

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}

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)
 	}

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.

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