Parse and emit frontmatter as real YAML

README.md

@@ -1072,15 +1072,23 @@ page_width: max
 
 ### Frontmatter
 
-Frontmatter is a block delimited by `---` lines containing flat `key: value` pairs
-(no nesting, lists, or multi-line values). Full-line `#` comments and trailing
-inline ` # ...` comments (whitespace, then `#`) are ignored.
-
-To include a literal ` #` (or leading/trailing whitespace, or a leading quote) in a
-value, quote it with single or double quotes — e.g. `title: "Detect # Verify"`.
-Single quotes are literal (`''` escapes a quote); double quotes honor `\"` and `\\`.
-markfluence adds quotes automatically when it writes a value back if they're needed
-to round-trip.
+Frontmatter is a **YAML** block delimited by `---` lines, restricted to flat
+`key: value` pairs — no nesting, lists, or multi-line values. That restriction is
+enforced: a nested value, a `|` block, a duplicate key, or a tab indent is an
+error naming the key, not something read as blank. Full-line `#` comments and
+trailing inline ` # ...` comments are preserved when markfluence rewrites a
+block.
+
+Because it is real YAML, a value that YAML would read as something other than a
+plain string has to be quoted — a colon-space (`title: "Deploy Runbook: Part 2"`),
+a leading `#`, `[`, `{`, `@`, `*`, `&`, `%`, `!`, `|`, `>`, `-`, or `?`, leading
+or trailing whitespace, and the words YAML types for you: `true`, `false`, `yes`,
+`no`, `null`, `~`, and anything that looks like a number. **markfluence quotes
+automatically whenever it writes a value**, so this only matters for frontmatter
+you hand-write.
+
+`null` in any spelling (`null`, `Null`, `~`, or an empty value) means *unset*.
+A page genuinely titled `null` is written `title: "null"`.
 
 | Field | Value domain | Notes |
 | --- | --- | --- |

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

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

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 != "" {

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

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"

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

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