Persist a created page to markfluence.yaml

[willkg]

PR 2 of 2 for #139 — the write half. markfluence create now records a new page's metadata in the project file's pages: block, so the bootstrap flow works end to end and a markdown file never needs frontmatter at all.

This is what PR 1 left as a hand-copy step, and it replaces PR 1's interim refusal (this file's metadata lives in markfluence.yaml; fix cannot reconcile a pages: entry yet was fix; create told you to paste the id into the entry yourself).

$ markfluence create docs/parent.md --title "Parent"
$ markfluence create docs/child.md  --title "Child" --parent parent.md
$ markfluence update docs/*.md

After which markfluence.yaml holds both entries and both markdown files still contain nothing but their bodies.

Scoped to create

The plan gave PR 2 fix migrating inline keys as well. #151 now proposes removing fix altogether, so that half is an open question rather than dropped work — and PR 2 needed nothing from it, which is why it was descoped rather than blocked on #151.

The writer, and the trap that shaped it

frontmatter.SetNested lives in internal/frontmatter rather than internal/project so goccy stays confined to one package: the dialect, its refusals, and every rule about emitting a value that reads back as itself are one package's business, and a second package building nodes is how two spellings of "write a YAML scalar" come to disagree.

Its sharp edge was measured, not reasoned about. A node's column is what goccy indents by, so a mapping value built at the default position is emitted at the left margin however deep it was appended — appending space: ENG inside a pages: entry comes out as a top-level setting:

pages:
  docs/a.md:
    title: A
space: ENG        # ← appended "inside" the entry

Valid YAML, entirely different meaning, and nothing downstream would refuse it — the loader would read a project-wide space nobody wrote. indentColumn is two spaces per level, the same fix seqIndentColumn already applies to a block sequence's items.

So the write verifies itself, and asks the right question. verifyNested re-reads its own output and checks each field landed at the path holding what was asked for — not merely that the document parses, because "did it parse" proves nothing against a field at the wrong depth. Sabotaging indentColumn to return 1 turns four tests red with the rewritten file lost "docs/b.md" instead of writing a corrupt file.

project.SetPageEntry verifies twice

Read-modify-write, once per page rather than once per run — because create writes each file's frontmatter as that page is published, so a run that dies partway leaves every already-created page recorded. The manifest had to keep that property (D10).

Then two verifications: SetNested's own, and parseConfig (split out of loadConfig) re-running the loader's rules over the result, so a write producing a file markfluence could not read — or could read as something else — fails before anything reaches disk. A tool that corrupts the file it is recording success in is the worst version of this feature, and a test pins that a file which would not load afterwards is left byte-identical.

A root with no project file refuses rather than creating one: whether markfluence may create a project file is #5's question, not something a create should answer silently.

Where metadata goes is inferred, not flagged

Per D9: to the manifest when a pages: entry already claims the file, or when the project has chosen the manifest and the file says nothing; into the file's frontmatter otherwise. So a project that has chosen the manifest never accidentally grows frontmatter, a project without one behaves exactly as before, and a half-migrated tree does not sprout entries behind the author — a file that already carries its own frontmatter keeps it, which a test pins. --no-persist records nothing anywhere.

persistToManifest is writeBackFrontmatter's counterpart: the same five fields, the same canonical order, the other location. parent is written as the resolved id exactly as the frontmatter path writes it; unlike frontmatter there is no room for a trailing comment naming the original path, and none is needed — the entry's key already says which file this is, which is what that comment existed to disambiguate.

Two things the live run found that the unit tests had not

pages: {} is a flow mapping — and it is precisely the shape a project that has chosen the manifest and registered nothing has, which is the shape the docs recommend. Appending a block entry to a flow mapping emits pages: { followed by a newline and does not parse. A populated flow mapping needed its children re-positioned too, since they carry the columns flow gave them. toBlock handles both, converting to block rather than preserving style: a nested entry cannot be written in flow style without re-emitting the whole line.

Worth noting how this surfaced — verifyNested refused the write and create reported a failure with the page id intact, rather than leaving a corrupt project file. That is the argument for verifying, demonstrated by accident.

A parent: naming a .md file is spelled differently in the two locations — root-relative inside an entry (like every pages: key), file-relative in frontmatter and on create --parent (matching how markdown links work). The translation is deliberate and nothing breaks, but it is a trap for anyone moving a value between locations by hand, so it is now documented in root-model.md and markdown_file.md. create records the resolved id rather than a path, so a round trip never hits it.

Verified live

Against the personal space on mozilla-hub.atlassian.net, from pages: {}:

parent: created | persisted: True | page: 3069804570
child:  created | persisted: True | parent: 3069804570 | parent_file: parent.md

update docs/*.md --force:
  published  src=manifest   docs/child.md
  published  src=manifest   docs/parent.md

Both markdown files verified byte-identical to # Parent / # Child afterwards. Pages purged.

Tests

SetNested on every shape that comes up: a new entry beside an existing one, creating the whole path, a comment-only file (what the marker ships as and what export plants), quoting, page_id as an integer, a null parent, an empty declared labels: [], flow→block for both empty and populated, goccy's one-pair MappingValueNode shape, canonical field order, and a refusal for a non-mapping level. SetPageEntry for the write, the in-place update leaving neighbours alone, the refusal leaving the file byte-identical, and no project file. create for the manifest write, the frontmatter path when there is no pages:, a frontmatter-carrying file keeping its frontmatter, --no-persist, and recording each page as it goes. The two blockers-by-sabotage: indentColumn and toManifest.

make check clean.

[willkg]

Adversarial review found ten; all fixed in four commits. No corruption or data loss — verifyNested and the loader re-check held, and every bad shape ended as a refusal with the file byte-identical.

The recurring defect was one level down, and it's the lesson worth keeping: the writer assumed a file it had written — two-space indent, LF, no BOM, normalized bare keys — while the loader accepts a far wider dialect. Every gap became "page created, entry not written", which is exactly the state D10 and S7 exist to prevent.

1. A successful run could leave a file permanently unpublishable

The worst of the ten, and the only one that produced a broken state without reporting a failure.

toManifest sent a file with keys in both locations to the manifest, writing the resolved parent: 101 into its entry while the file kept parent: p.md. The two then disagree about a coordinate, so every later update and check of that file fails:

markfluence.yaml and this file disagree about where this page is:
parent: frontmatter says "p.md", markfluence.yaml says "101".
Correct one of them; markfluence will not guess

That's PR 1's "self-inflicted corruption" mirrored — and this time from a run that said created. D9's "wherever that file's metadata already is" means frontmatter for a file whose metadata is already there, so the rule is now HasManifest(root) && !meta.InFile(). A test reverts the condition and fails with the message above.

2-5. The dialect gaps, each of which created a page and recorded nothing

A constant indent. Any pages: block not indented by exactly two spaces was unwritable — four-space, which the loader reads happily, gave the rewritten file does not parse. One-space was worse in kind: the new entry nested inside its predecessor, valid YAML saying something else, caught only because verifyNested checks depth rather than parseability. childColumn now takes the column from an existing sibling.

Getting that right needed two more measured details, and I'd have missed both without probing: an empty mapping has no sibling to ask, so it uses its own column — falling back to the constant put a just-created entry's first field at the entry key's column, making it the entry's sibling rather than its child. And toBlock must reset the converted mapping's own column, since a flow mapping's token sits at the brace — leaving it put the first entry written into pages: {} at column 8.

Keys were never quoted. Values have had the readsBackAs fallback since #130; keys had nothing, so a legitimate filename holding any YAML indicator — #x.md, - a.md, a: b.md, !a.md, |a.md, a tab, a leading space — was refused rather than quoted. keyNode applies the same discipline to the other side of the colon, and an existing quoted key is matched rather than duplicated.

A BOM. parseConfig strips one deliberately (#100), so it's a shape markfluence accepts — but the writer parsed the first key as pages and created a second pages: block beside it. It now comes off before the write and goes back on after, rather than being silently dropped from a file the author's editor wrote.

An un-normalized existing key. ./b.md is the entry for b.md, so matching on raw text appended a duplicate and the loader refused the file for having two keys naming one path. existingKeySpelling finds the entry that's there.

A file with no manifest key — one outside --root — wrote an entry keyed by the empty string. Now falls back to frontmatter, which is what it got before this PR.

6-9. Smaller

A persist I/O failure was reported as VALIDATION where the frontmatter path reports IO (#133) — now CodeOr. And the manifest path wrote a sixth field, labels, which writeBackFrontmatter has never written — worse, writing them rewrote them into their normalized form ([Runbook, ci/cd] → [ci/cd, runbook]) while the case warning still told the author to update the file to match. Both fixed by symmetry with the frontmatter path.

10. The write is atomic now

os.WriteFile truncates first, and unlike the frontmatter path — which risks one page's metadata — this file holds every entry in the project. replaceFile writes a temp file in the same directory and renames, so an interrupted run leaves the original untouched. An existing file's mode is preserved.

Test quality

Both findings were fair and both are fixed:

• TestCreateAllRecordsEachPageAsItGoes claimed to pin D10 while exercising no failure at all — a single deferred write at the end would have passed it. There's now a test with a real mid-batch failure that asserts the earlier pages are recorded and the failed one keeps its page id.
• Nothing reached SetPageEntry's second verification, because the test for it used a shape SetNested refuses first. Findings 3 and 4 were the shapes that get there, and they now have tests.

Known and accepted

Matching UpdateField: CRLF is normalized to LF, and a comment on the same line as a replaced value is dropped. D11's "comments survive" holds for key-attached and preceding comments — every placement probed except that one.

make check clean. _plans/039 records all ten.

[willkg]

One more, found by following the review's own pattern into a dimension neither pass was pointed at.

SetPageEntry had a lost-update window, and it is a regression this PR opens

The read-modify-write is not serialized, and the project file is shared by every page. Before the manifest, each page's metadata went into its own file, so two concurrent creates could not collide. They now can: A reads, B reads, A writes, B writes — and A's entry is gone while A's page exists. "Page created, entry not recorded" one more time, arriving from concurrency rather than from a refusal.

Bounded but live. create docs/*.md is sequential within one process, and page creation is deliberately a human act (#139 keeps it out of CI). But concurrent sessions against one repository are a normal working arrangement here, which is what makes it worth closing rather than documenting.

Optimistic, not locked. trySetPageEntry re-reads immediately before replacing and reports a change rather than overwriting; SetPageEntry retries once against the new content and reports the collision if it happens twice. The precedent is client.SetContentProperty, which retries once on top of a versioned PUT for the same shape of reason. A lock file would be stronger and brings stale-lock handling with it — more machinery than a verb a person invokes by hand warrants. One retry suffices because the window is a single file rewrite: the loser re-reads the winner's file and merges into it.

It is not a guarantee, and the comment says so: another writer can still land between the re-read and the rename. What it closes is the window that matters — the seconds a create spends publishing a page between reading the file and writing it back.

The give-up path is driven by beforeReplace, a nil-in-production test hook, the same arrangement SetRetryLogger and SetSecurityWarner use. Racing a real writer makes a slow and flaky test of a branch that exists precisely so a collision is never resolved by guessing.

And a test of mine that was trivially true

Worth reporting because the sabotage check caught it, not review. My first version of the concurrency test wrote the competing entry before calling SetPageEntry, so the initial read already saw it and no collision ever occurred — it passed with the collision detection removed. Deleted rather than kept: it claimed to test concurrency and tested merging, which TestSetPageEntryUpdatesInPlace already covers. The two tests that replace it both fail against the old code, with b.md is missing — the lost update itself.

That is the second trivially-true test this sabotage habit has caught in two PRs, which is an argument for keeping the habit.

make check clean.

[willkg]

A second review pass, local this time, found five — and the most useful of them corrects a fix I claimed to have made in the comment above.

The CodeOr classification was dead code

I reported fable's finding 7 as fixed "by symmetry with the frontmatter path". It wasn't. jsonout.CodeOr only consults CodeFor when client.FromRequest(err) is true, and SetPageEntry always returns a *project.ConfigError — so every failure fell through to the hardcoded VALIDATION, which is precisely what the change was supposed to stop doing. Reproduced: unwritable project directory → code: "VALIDATION", error: "…/markfluence.yaml: permission denied", where the frontmatter path reports IO for the identical condition.

ConfigError now carries IO, project.IsIOFailure exposes it, and create picks the code itself.

The narrow lesson, which I'd rather record than gloss: a fix that routes through a helper isn't a fix until the helper's precondition has been checked. CodeOr reads like "classify this error" and means "classify this error if it came from a request".

toBlock computed its column from an absolute depth

The very bug childColumn was added to fix — still present on the flow path. A document whose root mapping is itself indented ( pages: {}) is legal YAML the loader accepts, and the entry then came out at pages:' own column: the rewritten file lost "pages", or a parse failure at four-space indent. Via create that's a page on the server with nothing recording it — the D10 class this PR set out to close.

Third instance of one class, and worth naming why it kept recurring: each time, the fix went where the bug was found rather than everywhere the pattern lived. Every child column now comes from the parent key's real position; sabotaging it back to the constant fails the new tests.

replaceFile replaced a symlinked project file with a regular one

os.WriteFile — which the frontmatter path uses — follows a symlink, so making the write atomic was a silent behaviour change: a markfluence.yaml symlinked to a shared config became a local copy, and later edits to the shared file stopped applying to that directory. It writes through the link now, with a test asserting the link survives and the shared target receives the entry.

Two documentation findings, both real

create's --help still said metadata is written "back into the frontmatter", and both persist flags said the same. CLAUDE.md makes CMD --help the reference, so that's where it had to be fixed — it now says where a page is recorded and that markfluence.yaml is a shared file being modified. The schema's createResult description covered only a frontmatter persist failure.

And SetNested dropped Field.Comment where Render and setField honour it. Exported and taking []Field, so the next caller to set one — recording the original parent: path the way parentField does — would have lost it in silence.

On the two passes

Worth stating since it bears on how much review is enough here: the adversarial pass found ten by going deep where I pointed it; this one found five by looking where I hadn't, including one where I was wrong about my own work. Neither subsumes the other, and the overlap was zero.

make check clean. _plans/039 records both passes.