Persist a created page to markfluence.yaml

_plans/039_project-file-pages.md

@@ -722,3 +722,198 @@ is something to reconcile it with.
 - `layout:` (#21) and any later field: they should need nothing here, and a test
   that an unknown-to-the-test field survives a manifest round-trip is what would
   prove it.
+
+## PR 2, as built — 2026-09-12
+
+Scoped to `create` only. 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 the honest
+reason it was descoped rather than blocked on #151.
+
+**`frontmatter.SetNested`** (`nested.go`) is the writer, here rather than in
+`internal/project` so goccy stays confined to one package. Its sharp edge was
+measured, not reasoned: **a node's column is what goccy indents by**, so a
+mapping value built at the default position emits at the left margin however
+deep it was appended — appending `space: ENG` inside a `pages:` entry produces a
+*top-level setting*. Valid YAML, entirely different meaning, and nothing
+downstream would refuse it. `indentColumn` is two spaces per level, the same fix
+`seqIndentColumn` already applies to block sequence items.
+
+So `verifyNested` re-reads the result and asks whether each field landed **at
+the path** holding what was asked for, not merely whether the document parses —
+"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" rather than writing a corrupt file.
+
+**`project.SetPageEntry`** owns the file: read-modify-write once per page, per
+D10, and it verifies twice — `SetNested`'s own check, then `parseConfig` (split
+out of `loadConfig`) re-running the loader's rules, so a write producing a file
+markfluence could not read fails before touching disk. A test pins that such a
+file is left byte-identical.
+
+**`create`** infers the destination per D9 and needed no new flag.
+
+Two things the live run found that the unit tests had not:
+
+- **`pages: {}` is a *flow* mapping**, and it is exactly the shape a project
+  that has chosen the manifest and registered nothing has. Appending a block
+  entry to it emits `pages: {` then 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. `verifyNested` caught it, so
+  the failure was a refusal rather than a corrupt file — which is the whole
+  argument for verifying.
+- **A `parent:` naming a `.md` file is spelled differently in the two
+  locations** — root-relative in an entry, file-relative in frontmatter and on
+  `create --parent`. That is the translation this plan settled deliberately, 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 end to end: a pristine parent created, a pristine child created
+under it, then `update docs/*.md` publishing both from the manifest — with
+neither markdown file carrying any frontmatter. Pages purged.
+
+## What PR 2's review found — 2026-09-12
+
+Ten findings. 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 is 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.
+
+**The one that produced a broken state from a *successful* run**, and the reason
+this needed fixing before the flow could be recommended: `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 until somebody edits one by hand. D9's "wherever that file's metadata
+already is" means *frontmatter* for a file whose metadata is already there, and
+the rule is now `HasManifest(root) && !meta.InFile()`.
+
+The dialect gaps, each of which made `create` create a page and record nothing:
+
+- **A constant indent.** Any `pages:` block not indented by exactly two spaces
+  was unwritable; four-space, which the loader reads happily, produced a parse
+  failure. 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. Fixing it needed two
+  more measured details: an *empty* mapping has no sibling to ask its column
+  from and must use its own, and `toBlock` has to reset the converted mapping's
+  own column, since a flow mapping's token sits at the brace.
+- **Keys were never quoted.** Values have had the `readsBackAs` fallback since
+  #130; keys had nothing, so a filename holding any YAML indicator (`#x.md`,
+  `- a.md`, `a: b.md`, a tab, a leading space) was refused.
+- **A BOM.** `parseConfig` strips one deliberately, so it is a shape markfluence
+  accepts — but the writer parsed the first key as `\ufeffpages` and created a
+  second `pages:` block beside it.
+- **An un-normalized existing key.** `./b.md` *is* the entry for `b.md`, so
+  matching on raw text appended a duplicate the loader then refused.
+- **A file with no manifest key** (one outside the root) wrote an entry keyed by
+  the empty string.
+
+Smaller: a persist I/O failure was reported as `VALIDATION` where the
+frontmatter path reports `IO` (#133); the manifest path wrote a *sixth* field,
+`labels`, which `writeBackFrontmatter` has never written — and writing them
+rewrote them into their normalized form while the case warning still told the
+author to update the file to match. Both fixed by symmetry with the frontmatter
+path.
+
+The write is atomic now (temp file plus rename). `os.WriteFile` truncates
+first, and unlike the frontmatter path — which risks one page's metadata — this
+file holds *every* entry in the project.
+
+Two test-quality findings, both real: `TestCreateAllRecordsEachPageAsItGoes`
+claimed to pin D10 while exercising no failure at all, so a single deferred
+write at the end would have passed it; and nothing reached `SetPageEntry`'s
+second verification, because the test for it used a shape `SetNested` refuses
+first. Both now have tests that fail against the old code.
+
+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, which is every
+placement probed except that one.
+
+## A concurrency window PR 2 opened — 2026-09-12
+
+Found by following the review's own pattern into a dimension neither review pass
+was pointed at, after `/ultrareview` turned out to be Enterprise-gated.
+
+`SetPageEntry` is a read-modify-write with no serialization, and the project
+file is shared by every page. **Before the manifest, each page's metadata went
+into its own file, so two concurrent `create`s 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. So it is a regression this PR
+introduces, not a pre-existing gap.
+
+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
+Claude sessions against one repository are a normal working arrangement here,
+which is what makes it worth closing.
+
+**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
+would bring stale-lock handling with it, which is more machinery than a verb a
+person invokes by hand warrants. One retry is enough because the window is a
+single file rewrite: the loser re-reads the winner's file and merges into it.
+
+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.
+
+Worth recording how the test got written: the first version landed the competing
+write *before* calling `SetPageEntry`, so the initial read already saw it and no
+collision occurred. It passed with the detection removed, which is how it was
+caught — the same sabotage check that has now found a trivially-true test twice.
+
+## A second, local review pass — 2026-09-12
+
+Run after the adversarial one, with a different brief. Five findings, and the
+most useful of them is a **correction to a fix the previous section claims to
+have made**, which is the argument for a second reviewer that does not inherit
+the first one's framing.
+
+**The `CodeOr` classification was dead code.** `jsonout.CodeOr` only consults
+`CodeFor` when `client.FromRequest(err)` is true, and `SetPageEntry` always
+returns a `*ConfigError` — so every failure fell through to the hardcoded
+`VALIDATION`, which is exactly what the earlier commit said it had fixed.
+Claiming symmetry with the frontmatter path was therefore wrong in the plan,
+the commit message and the PR comment. `ConfigError` now carries `IO`, and
+`project.IsIOFailure` is what a caller asks.
+
+The lesson is narrow and worth keeping: a fix that routes through a helper is
+not 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. Third instance of
+the same class, and the reason it kept recurring is that the fix was applied
+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 with `the rewritten file lost "pages"`.
+
+**`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.
+
+**`create`'s help still said metadata goes into the frontmatter**, and
+CLAUDE.md makes `CMD --help` the reference, so that is where it had to be
+fixed rather than in a doc. Both persist flags and the `Long` now say 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.
+
+**`SetNested` dropped `Field.Comment`** where `Render` and `setField` honour
+it — exported, taking `[]Field`, so the next caller to set one would have lost
+it in silence.