Parse and emit frontmatter as real YAML - #131
Merged
Merged
Conversation
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #130. Replaces the hand-rolled flat-key frontmatter parser in
internal/frontmatterwith goccy/go-yaml. Design record:_plans/032_frontmatter-yaml.md. Adds C2 (frontmatter-is-valid-yaml) todocs/guarantees.md.The bug
renderValuedecided whether to quote a value by asking "would our ownParseValueround-trip this?" rather than "is this valid YAML?". A colon round-trips through a first-colon line split, so a title containing": "was written bare:markfluence read that back perfectly and every real YAML parser rejected it, which is why it went unnoticed — VSCode's YAML extension is what surfaced it. The colon was one member of a class:
true,123,null,~,[draft] Foo,@home,*star,%pct,- dashwere all written bare and read back as the wrong type or not at all.frontmatter_test.gohad a test pinningtitle: a: bas correct output, so this was a deliberate decision being reversed rather than an oversight.The writer verifies its own output
This is the part worth reviewing closely. goccy's default emission is not round-trip-safe for four shapes, found by probing ~80 strings:
"a\tb"title: a<TAB>b"ab"— silent loss"? q"title: ? q".inf"title: .inf".nan"title: .nanSo the writer does not predict which values goccy mishandles — it checks: emit with goccy's chosen style, re-parse, compare, and fall back to a forced double-quoted scalar when the two disagree (15/15 probed cases round-trip that way). A hand-written predicate listing those shapes would have been incomplete, since three of the four only turned up after two review rounds. Checking beats predicting, and the check keeps holding if goccy regresses.
The comparison looks at the re-read node kind, not just its text. Comparing text alone said
.infround-tripped —scalarValueflattens every scalar to its token, so markfluence reads it back as.infeither way while the file still says "float" to everyone else. That is the same mistakerenderValuemade, one level down.Quoting is goccy's, typing is ours
page_idandparentare written as YAML integers and nulls, becausepage_id: "123"is valid YAML that says the wrong thing. Confined to two keys whose value domains are closed — a title of"123"is still a string.Reads enforce the flat contract instead of assuming it
The "no nesting, lists, or multi-line values" restriction was documented but never checked; the old parser silently produced
""for a violation. Now:|block each reports its indicator character as its token value — a blacklist of sequences and mappings would silently read&.title: plain\n continuedcame back as a|-block the parser then rejected, so a write produced a file markfluence could not read — and increate, only after the page had been made. A multi-line single-quoted scalar was quieter and worse, re-emitting on one line and turning"sq\nline"into"sq line"....line is refused, since reading only the first document dropped every later key in silence.Every null spelling is unset
null,Null,~, and an absent value all read as"". The old parser matched only the literal"null", soparent: ~read as though it were a page id — a bug that predates this change. That had one sharp consequence infix: a present-but-blank coordinate took the"(none)"branch and plannedparent: (none) -> nullon every run, wrote it, and read""again, so a correct top-level page was reportedchangedforever.TestPlannedChangesParentNullNormalizesguarded exactly this and would have kept passing — it fed{"parent": "null"}, a map the new parser can never produce.Behaviour changes
updaterejects a present-but-emptytitle(title:,title: null), before any request. An absent title keeps today's fallback to the live page title: no title key is a positive statement that a file does not manage its page's title, while a present-but-empty one is a half-finished edit that should not silently publish under whatever the page is called now.--titlestill wins over both.checkreports a present-but-emptytitleasbroken. Its "deliberately narrow" justification — that it cannot know whethercreateorupdateis coming — stops coveringtitleonce both verbs reject an empty one.page_width: nullwas an invalid-width error and now means unset. It used to reachpagewidth.Declaredas the string"null". Follows from the uniform null rule;checkno longer reports it.fix—fixreconciles from the live page and needspage_id, which it cannot read out of a file it cannot parse, so the remedy would be circular for exactly the files needing it.--jsonoutput is unchanged from main. The only schema edit is one description string.Not in this PR
Field-order normalization (
Normalize,fix'sreordered,create --persistnormalizing) is split out to_plans/033and a follow-up branch stacked on this one. It is a feature rather than a consequence of #130, it carries the only--jsoncontract change, and bundling the two buried the part that fixes the bug.gopkg.in/yaml.v3was measured against every requirement and clears them all, but it is archived at v3.0.1 (2022); goccy is maintained.Review notes
Three design passes and one code review, which turned up four real defects now fixed and pinned: the multi-line scalar case above,
.inf/.nanbeing written bare while the hazard test passed vacuously, the...document case, andRenderbeing able to emit a duplicate key.One limit C2 states rather than papers over: the verification is a self-check. It proves goccy can re-read what goccy wrote, not that another implementation can —
1e3is a string to goccy and a float to yaml.v3, and no self-check can see that. There is deliberately no second YAML library ingo.mod; a divergence reported by a real tool is an issue to fix, with the offending frontmatter as evidence.Testing
make checkpasses, and every commit is independently green. Frontmatter round-tripping is pinned by a hazard table plusFuzzUpdateFieldRoundTrips, which runs seed-corpus-only under plaingo testand is the only thing that could find a value the double-quote fallback also fails on.