Read page metadata from markfluence.yaml's pages: key - #150
Conversation
Design for #139: a pages: key mapping a root-relative path to that file's page metadata, so a .md can be published while staying pristine. Lands on #100's loader and makes #29's Action usable -- one step, a glob, no per-file inputs. The core move is making #139's "an entry is a whole frontmatter block that lives elsewhere" literal: an entry carries the same two maps frontmatter.MarkdownFile does. That is not just tidy, it is the only shape that works -- internal/project cannot import internal/pagewidth or internal/labels, both of which reach internal/client, which holds a *project.Cache, so a typed validated Entry would need a broken cycle or a second copy of every field's rules. With the two-map shape, labels.Declared(e.Lists, e.Fields) and pagewidth.Declared(e.Fields) work unchanged in the commands that already call them. Facts from the code that shaped it. linkindex.Build already takes the root and already keys idx.pages by the exact root-relative slash path pages: uses, so the merge needs no signature change. The dialect refuses nesting and must keep refusing it for frontmatter, so nesting becomes a MaxDepth on Dialect rather than a key-name list. create writes each file's frontmatter as that page publishes, inside the loop, so a crash leaves every already-created page recorded -- the manifest writer has to keep that. Landing as two PRs: read and consume first (the whole CI use case), then the nested surgical writer plus create and fix. The writer is the part most likely to need a second pass, and holding the read half hostage to it would mean re-reviewing all of it. Also records a considered-and-rejected alternative -- pages: as a nested hierarchy -- because the question will come back. It cannot replace parent: (which is null, an opaque page or folder id, or a path, and nesting expresses only the third), every consumer looks up by path so a hierarchy is flattened on load anyway, and the correctness argument does not survive: cycles are already rejected in create's topological sort. Two follow-ups came out of the discussion and are filed: #148 (a status command) and #149 (update silently overwriting a Confluence-side edit).
markfluence.yaml's pages: key needs two levels -- pages, then a path, then that page's fields -- where the fenced frontmatter block must keep refusing nesting entirely. So the allowance is a depth on Dialect rather than a list of keys permitted to nest, which keeps the reader learning a kind and not a name: the project file asks for two levels and says so, and a mapping under a key whose own table says scalar is refused by that table with its own message. MaxDepth defaults to zero, which is what blockReader keeps, so every message and refusal the fenced block makes is unchanged -- two tests pin that from both sides. Past the allowance a mapping falls through to the scalar path and is refused there, so a flat dialect behaves exactly as the reader did before nesting existed rather than by a separate branch that could drift from it. Item gains Map, and the nil-ness of List and Map is what says which of the three shapes a value has. Both goccy spellings of a one-key mapping are accepted (MappingValueNode as well as MappingNode, the same split soleMappingPair exists for on the write side), or a single-field entry would read as a broken scalar.
An Entry is shaped exactly like a parsed frontmatter block -- the same two maps MarkdownFile carries, scalars by key and sequences by key. That is the design and not a convenience: internal/pagewidth and internal/labels both reach internal/client, which holds a *project.Cache, so this package can never import them, and a typed validated Entry would need a broken cycle or a second copy of every field's rules. With two maps it needs neither -- labels.Declared(e.Lists, e.Fields) and pagewidth.Declared(e.Fields) work on an entry unchanged, in the commands that already call them. entryFields is the manifest's schema and the only place it is written down. It mirrors frontmatter's fields deliberately: adding one there without adding it here would make a field expressible in a file and not in an entry, which is the "same keys" half of the equivalence. What is checked at load is structure -- a mapping of mappings, legal paths, known field names, right shapes. What is not checked is any field's value, because #139 requires a semantically bad entry to be reported only when its file is one of the arguments, and this function has no idea which files the command was given. An unknown field *name* is the exception and is load-time: it is the same typo class as an unknown top-level setting, and it means the manifest was written against a different markfluence. NormalizePageKey is used on both sides -- the manifest's keys and a file being published -- because a mismatch means a silent skip rather than an error, so the two have to agree exactly. Lexical only, no symlink resolution (L2 forbids a key whose meaning depends on the checkout's layout); an escaping or absolute key and two keys normalizing to one are load-time errors naming both spellings, since YAML only catches literal duplicates. Config.Pages is nil when there is no pages: key and empty-non-nil for `pages: {}`, which is how a command tells "has not chosen the manifest" from "has, and has registered nothing" -- D9's write-destination inference turns on exactly that. One existing message changed: `space:` holding a mapping is now refused by the settings table ("must be a single value, not a mapping") rather than by the reader, since the project dialect allows depth for pages: and the value reaches the table. That is the intended split and the table's wording is the better one.
…nifest The merge update, create, fix, check and linkindex all need. A package rather than a helper because a per-command copy is exactly how two commands come to publish one file to two different pages. It imports only frontmatter and project, so it stays out of every cycle -- which is also why it validates no value: pagewidth and labels are unreachable from here, and the commands that need them already call them on the maps this returns. Frontmatter and an entry are two spellings of *one* level, not two levels of a precedence chain. When both speak the rule is not "the higher wins" but grading: page_id/space/parent are coordinates and a disagreement fails the file, while title/page_width/labels are visible and recoverable, so they warn and frontmatter wins. Agreement is silent, which is what makes migration incremental. Three things that took a second pass to get right. Source and Managed are computed from different predicates, deliberately. Source answers "who contributed metadata" for --json's metadata_source; Managed answers "should update act on this file". A file is claimed when it has an entry or names a page_id -- a page_id is the only field that identifies a page, and #139 is precise that a *registered* file is one with an entry. So `a.md: {}` is a claim that must error for want of a page_id rather than be skipped. Contributed metadata is counted over the fields markfluence knows, via a new project.IsPageField, not over any key at all. A docs tree carrying Jekyll or Hugo frontmatter (layout:, date:, draft:) has said nothing about Confluence, and counting every key would have read a whole such tree as claimed. labels is compared as a set. Confluence has no label order, so [a, b] and [b, a] say the same thing and warning that one "overrides" the other would be noise about a difference that cannot reach the page. A blank value is not a disagreement: every null spelling already reads as "", so a key with nothing after it says no more than no key.
Build read each sibling's own frontmatter for page_id/title, so a file whose coordinates live in markfluence.yaml's pages: block had no entry in the index -- and every link to it would degrade to the "exists on disk, not published yet" warning and republish as plain text. #139 calls this out as the sharp touchpoint, and it is sharp in consequence rather than in plumbing: Build already takes the root and already keys idx.pages by the exact root-relative slash path pages: uses, so the change is to resolve through pagemeta instead of from mf alone. Resolving per path is also what makes a half-migrated tree correct throughout, with some files declaring their own coordinates and some leaving them to the manifest. A disagreement costs the file its page *entry* and nothing else. The first attempt skipped the whole file, matching what an unreadable or malformed one gets, and a test caught why that is worse: the anchors go unrecorded too, so FileExists goes false and every link *to* that file becomes LINK BROKEN across the tree, where the right answer is the ordinary unpublished warning.
…page flags update now takes each file's title and page id from its own frontmatter or from a pages: entry for it, indistinguishably. Both places are legal and agreement is silent, which is what makes migration incremental; a coordinate disagreement (page_id/space/parent) fails that file, and a soft one (title/page_width/labels) warns with frontmatter winning. --title, --page-id and --page-width are gone. The rule is that flags describe the run and files describe the page: the first two were single-FILE-only, so docs/**/*.md could not be expressed at all, which is the whole reason #139 exists. --page-width was the only batch-ok one and so the real loss, but a project-wide page_width: says the same thing permanently instead of once per invocation. overrideNeedsSingleFile goes with them, and a test pins their absence so re-adding one is a deliberate act. A file neither place mentions is skipped, ok, exit 0, with no request made. Repositories legitimately hold markdown that is not published and drafts are a normal state, so a glob-driven CI run must not go red because somebody added a file. A file that *is* registered but has no page id still fails -- somebody claimed it and create has not run -- and human output distinguishes the two skips, since "no changes" and "not published by markfluence" are different facts. create reads entries too, which is read-side work and belongs here rather than in the write PR: ignoring an entry's page_id would publish a second page and leave the entry pointing at the first, which is #18's bug reached from the other direction. Its flags are unchanged -- create establishes metadata, so --space/--parent/--title are how an entry that does not exist yet gets bootstrapped. --json gains metadata_source on both commands ("frontmatter", "manifest", or null), because debugging "why did it publish to *that* page" in a CI log otherwise means reproducing the resolution by hand. Required and nullable, no omitempty.
check now resolves each file's metadata through pagemeta before validating it, so an entry's page_width, page_id, labels and title are checked exactly as a file's own are -- and only for a file the invocation names, which is #139's per-file scoping made real: one bad entry never blocks checking the rest of a repository. Two locations naming different pages is an error, and offline-visible, which is the kind of defect check exists to catch before a publish does. The half-and-half lint is a *warning*, never an error, and that is the whole point: agreement between the two locations is legal, so "keep it in one place" has to be sayable without becoming a wall somebody hits halfway through a migration. fix moving the keys is the remedy (PR 2). One bug found by its own test: r.warnings was *assigned* from the label and converter warnings, so anything set earlier -- the new lint -- was silently discarded. It accumulates now.
docs/root-model.md gains a pages: section beside the settings: that an entry is a frontmatter block living elsewhere, the disagreement grading, that an unclaimed file is skipped rather than failed, and the path-key rules -- lexical, root-relative, no case folding, with an escaping or duplicate key failing the load. docs/github-actions.md is the one that changes most, because it was the document #139 exists to make true. Its example was `update --page-id=12345 --force docs/some_doc.md`; it is now `update docs/**/*.md` with no per-file inputs at all, plus the pristine-markdown shape, and notes on the skip, on metadata_source, and on why creating pages stays a human act. docs/guarantees.md extends the L2 note: a pages: entry is #100's argument taken further, and it is *why* the keys are lexical -- resolving a symlink would make a key mean different things on two checkouts. update losing its three page flags narrows L2's carve-out for flags, since which page a file publishes to no longer depends on how the command was invoked. CLAUDE.md gets an internal/pagemeta bullet, and internal/project's gains pages: -- including the three things that took a second pass: Source and Managed computed from different predicates, contributed metadata counted only over known fields (or a Jekyll tree reads as claimed), and labels compared as a set. docs/json-output.md documents metadata_source. The README block and docs/markdown_file.md both point at the new section rather than restating the field table.
The metadata_source addition went in via a JSON round-trip, which reformatted the whole document -- every compact inline object exploded across lines, for a 1728-line diff in a file that is hand-formatted and read by people. Reverted and applied as text: 10 lines, and only the two definitions that change. The enum also becomes a shared metadataSourceOrNull $def rather than being written twice, matching stringOrNull and the other *OrNull refs beside it.
…ntry Two merge blockers, both in create, which "read entries" for title/space/page_id/page_width/labels while two other paths still read frontmatter alone. **A .md parent could not resolve in a manifest project, and this plan's own example was one of the failing cases.** Two independent halves. resolveParent joined the parent path onto the *file's* directory, so a root-relative spelling looked for docs/docs/engineering-docs.md; and it read the parent's id with pmf.PageID(), frontmatter only, so a parent whose page_id lived in its own entry reported "not yet published". The second is #139's linkindex trap in a second place, where it fails a create rather than degrading a link. A path in the manifest is root-relative, like every pages: key, and #139's own example spells parent: that way -- but a parent: path everywhere else in markfluence is relative to the file that names it. pagemeta now translates at the boundary, so the manifest stays internally consistent and nothing downstream has to learn where a value came from. The plan claimed the two resolved "exactly the same way", which was self-contradictory and was implemented as neither; it is corrected. **create no longer persists frontmatter into a file a pages: entry claims.** It was writing page_id and a resolved `parent: <id>` into a file whose entry said something else, so the next update or check failed as a coordinate disagreement. D9's rule is that new metadata goes where that file's metadata already is, and PR 1 has no manifest writer -- so the right behaviour is not to write, and to say so: the result carries a warning naming the new page id and the entry to put it in. D13 called the cost "a page_id copied by hand"; this was silently creating the thing that needed repairing. Also from review: a present-but-blank frontmatter key no longer vanishes when an entry exists (the same file was broken without one and clean with one); metadata_source is set after the Managed() check, so an unmanaged skip reports null as the schema says; and create reports meta.Warnings, which D6 promises wherever metadata is resolved and only update delivered. Nothing in cmd/create exercised pagemeta at all, which is how both blockers hid. Both halves of the parent fix are sabotage-checked.
check's project-width lint read mf.Frontmatter rather than the resolved metadata, so a file whose *entry* declared a width was failed over the project's invalid one -- the exact false positive the surrounding comment forbids, while update published the same file fine. check also dropped meta.Warnings, so a soft disagreement between the two locations was reported only by update, against D6's promise that it is reported wherever metadata is resolved. Two more from review, both in the same class of "the code and its own comment disagreed": NormalizePageKey judged absoluteness before normalizing separators, so `\foo.md`, `C:\foo.md` and a UNC path were accepted as keys KeyFor can never produce -- a silently unreachable entry, worse than an error because nothing says so. And failure.metadataSource was never assigned while its comment claimed it was populated, so it is gone and abortedResult's null is explained. create's "no title given" and the --parent conflict message now name the entry as a place a value can live. Tests: the width false positive, the check-side warning, the Windows absolute forms, that a kind is never zero (IsPageField reads 0 as absent), that a valueless `pages:` is refused, the enum values of metadata_source against the schema, the unmanaged skip's own document, and that the two skips render differently -- which the PR claimed and nothing checked.
_plans/039 gains the nine findings and the corrected parent claim: an entry's .md parent is root-relative, translated at the boundary, which is *not* "the same way as frontmatter" however much the equivalence elsewhere holds. Separately: the comments justifying IsPageField cited Jekyll and Hugo frontmatter as if that were a known scenario here. It is not, and the grounded version is inside markfluence -- internal/frontmatter deliberately preserves keys it does not understand, pinned by a test on `reviewers: [ana, bo]`, so a file carrying only such keys is a shape markfluence explicitly supports and must not read as claimed. No third-party tool required, and verifiable in this repo.
|
Adversarial review found nine; all fixed in 3 commits. Two were merge blockers and both were in 1. A
|
Found by asking what the review's pattern implied rather than by the review itself, which was scoped to the diff: every one of its nine findings was at a seam -- a call site updated incompletely, an ordering, or old code receiving new inputs -- and not one was in the new core. That shape is enumerable, so one grep over every place page metadata is read from a file turned up a whole class still unfixed. pageref.Resolve read mf.PageID() and nothing else, so seven commands could not name a file update could publish: info, read, children, export and the three attachment verbs. They failed with "no page_id in frontmatter", safely -- but the page argument meant one thing to update and another to everything else, which is the inconsistency this package exists to prevent. Fixed in pageref rather than at seven call sites. Resolve already stats the file, so it discovers the root from that file's own directory -- the same per-file discovery every other read does -- and resolves through pagemeta. No signature change, no call-site churn, no import cycle. One deliberate asymmetry: a malformed markfluence.yaml is *not* fatal here. A disagreement between the two locations is, since that is exactly the question being asked and either id would be a guess; but a project file this function never consults should not make `info` fail, and the commands that bound reads by the root report the problem themselves. fix had no project concept at all, being the one verb that reads the page and writes the file. It gets one, which is what lets it locate a pristine registered page -- and it refuses to write into a file whose metadata lives only in the manifest, because reconciling that means editing a pages: entry and attempting it would reproduce the create bug review caught. A file carrying some inline keys is still fixable: those keys exist already, and the two locations agreed about the coordinates or resolution would have failed first. Its planners take the resolved maps rather than a MarkdownFile, so they reconcile what the file effectively declares.
CLAUDE.md's internal/pageref bullet said a .md argument resolves by its frontmatter page_id, which is now only half of it -- and the reason the lookup belongs there rather than at the seven call sites is worth writing down, since that is what keeps the page argument meaning the same thing to update and to info. The malformed-vs-disagreement asymmetry goes with it. fix's passage gains what it refuses and why: it can locate a page whose coordinates live in an entry, but writing frontmatter into such a file is the bug review caught in create, so it says so instead. docs/root-model.md notes the reader-facing consequence: `markfluence info docs/deploy-runbook.md` works on a file that says nothing about Confluence.
|
Fixed the seam the review's pattern pointed at but its scope didn't cover. All nine review findings were at seams — a call site updated incompletely, an ordering, or old code receiving new inputs — and none were in the new core. That shape is enumerable, so I enumerated it: one grep over every place page metadata is read from a file outside Seven commands could not name a file
|
1 participant
PR 1 of 2 for #139 — the read half, which is the whole CI use case. A markdown file can now be published while carrying no markfluence keys at all, with its page metadata in a
pages:entry inmarkfluence.yaml. PR 2 adds the write half —createpersisting entries. (The plan also gave PR 2fixmigrating inline keys; #151 proposes removingfixaltogether, so that half is now an open question rather than planned work.) Design:_plans/039_project-file-pages.md.This is what it makes possible, and the point is what is absent from the workflow step:
or something like this if we build a GitHub action:
No matrix, no per-file inputs, no flags. Adding a page is a repository change rather than a workflow change.
An entry is a parsed frontmatter block
Entrycarries the same two mapsfrontmatter.MarkdownFiledoes —Fields map[string]stringandLists map[string][]string. That is the design rather than a convenience, because it is the only shape that works:internal/pagewidthandinternal/labelsboth reachinternal/client, which holds a*project.Cache, sointernal/projectcan never import them, and a typed validated entry would need either a broken cycle or a second copy of every field's rules. With two maps it needs neither —labels.Declared(e.Lists, e.Fields),pagewidth.Declared(e.Fields)andpageref.IsDigits(e.Fields["page_id"])all work on an entry unchanged, in the commands that already call them.entryFieldsis the manifest's schema and the only place it is written down. It mirrors frontmatter's fields deliberately: adding one there without adding it here would make a field expressible in a file and not in an entry, which is the "same keys" half of the equivalence.Frontmatter and an entry are two spellings of one level
Not two levels of a precedence chain — writing it as one is wrong, and the plan says so after I got it wrong first. When both locations speak, the rule is not "the higher wins" but a grading by what a disagreement can destroy:
page_id,space,parentpage_idpasted from an old file publishes over a live pagetitle,page_width,labelsAgreement is silent, which is what makes migration incremental: copy values into the manifest, verify, delete them from the files later, with everything working throughout. There is no project "mode" and no all-or-nothing switch. Precedence only applies below these two, for a field neither declares — which is where #100's project-wide defaults sit.
A file nothing claims is skipped, not failed
updateerrored on a file with nopage_id. It now reportsskipped,ok: true, exit 0, with no request made. Repositories legitimately hold markdown that is not published, drafts are a normal state, and a glob-driven CI run must not go red because somebody added a file.A file that is registered but has no
page_idstill fails: somebody claimed it andcreatehas not run. Human output distinguishes the two skips, since "no changes" and "not published by markfluence" are different facts.updateloses--title,--page-id,--page-widthThe rule is flags describe the run; files describe the page. The first two were single-FILE-only, so
docs/**/*.mdcould not be expressed at all — which is the entire reason #139 exists.--page-widthwas the only batch-ok one and so the real loss, but #100's project-widepage_width:says the same thing permanently instead of once per invocation.--message/--force/--dry-runstay, being invocation metadata and behaviour.overrideNeedsSingleFilegoes with them, and a test pins the flags' absence so re-adding one is a deliberate act. This also settles permanently what #138 kept bumping into: there is no--labels, and no flag for whatever field comes after it.createkeeps its flags, for a principled reason rather than squeamishness: it is the verb that establishes metadata, so--space/--parent/--titleare how an entry that does not exist yet gets bootstrapped.updateonly ever consumes metadata, so it should have no way to invent any.The sharp touchpoint, verified live
linkindex.Buildread each sibling's own frontmatter forpage_id/title, so a manifest-only file had no index entry — and every link to it would degrade to the "exists on disk, not published yet" warning and republish as plain text. It resolves throughpagemetanow.Verified against
mozilla-hub.atlassian.netwith a pristine tree (no frontmatter in any file, a shared asset above the pages, a draft). Published storage fordocs/deploy-runbook.md:That link target is only knowable from the manifest. The whole glob in one command:
Pages purged afterwards.
Elsewhere
internal/frontmatter—Dialect.MaxDepth, becausepages:needs two levels and the fenced block must keep refusing nesting entirely. A depth rather than a list of keys allowed to nest, so the reader still learns a kind and not a name; past the allowance a mapping falls through to the scalar path and is refused there, so a flat dialect behaves exactly as before rather than by a parallel branch that could drift.MaxDepthdefaults to 0, whichblockReaderkeeps, and two tests pin that from both sides.internal/pagemeta(new) —Resolve, the one mergeupdate,create,checkandlinkindexshare, because a per-command copy is how two commands come to publish one file to two different pages.check— validates a file's metadata wherever it lives, and only for a file the invocation names, so one bad entry never blocks checking the rest of a repository. Two locations naming different pages is an error. The half-and-half lint (inline keys in a project that usespages:) is a warning, never an error — agreement is legal, so "keep it in one place" has to be sayable without becoming a wall somebody hits mid-migration.--json—metadata_sourceonupdate/create:"frontmatter","manifest", ornull. Debugging "why did it publish to that page" in a CI log otherwise means reproducing the resolution by hand.Every command that takes a page accepts a pristine file
Added after review, from an audit rather than from the review itself.
internal/pageref.Resolvereadmf.PageID()alone, so seven commands could not name a fileupdatecould publish —info,read,children,exportand the threeattachment-*verbs. They failed safely (no page_id in frontmatter of docs/a.md), but the page argument meant one thing toupdateand another to everything else, which is the inconsistency that package exists to prevent.Fixed in
pagerefrather than at the seven call sites:Resolvealready stats the file, so it discovers the root from that file's own directory — the same per-file discovery every other read does — and resolves throughpagemeta. No signature change, no call-site churn. One deliberate asymmetry: a disagreement between the two locations is fatal there (it is the question being asked), while a malformedmarkfluence.yamlis not, since a project file that resolver never consults must not makeinfo 123fail.fixalso changed, and more than the plan expected. It had no project concept at all, being the one verb that reads the page and writes the file; it now resolves through the root, which is what lets it locate a pristine registered page. And it refuses to write into a file whose metadata lives only in the manifest — reconciling that means editing apages:entry, and writing frontmatter instead is precisely the bug review caught increate. A file carrying some inline keys is still fixed. Its planners take the resolved maps rather than a*MarkdownFile, so they reconcile what the file effectively declares.Things that took a second pass
Recorded because each was wrong first and the wrong version passed its tests. These are the ones found while building; adversarial review then found nine more and an audit found the
pagerefclass above — all fixed, and detailed in the comments below rather than here.A file carrying only keys markfluence preserves read as claimed. Counting any frontmatter key at all as "this file declares metadata" meant a file whose only frontmatter was, say,
reviewers: [ana, bo]reported as managed — andupdatewould then fail it for want of apage_id. That is a shape markfluence explicitly supports:internal/frontmatterdeliberately keeps keys it does not understand, pinned by a test on exactly that key, because alabelsspecial case "would satisfy every other test and break #21/#100". Contributed metadata is now counted only over fieldsproject.IsPageFieldknows. (An earlier version of this description justified it with Jekyll and Hugo frontmatter, which is a scenario nobody here has; the reason is inside markfluence, and the code comments say so now.)SourceandManagedneed different predicates.Sourceanswers "who contributed metadata", for reporting;Managedanswers "shouldupdateact on this file", which is true when an entry exists or apage_idis named. Apage_idis the only field that identifies a page, and #139 is precise that a registered file is one with an entry — soa.md: {}is a claim that must fail for want of an id rather than be skipped, and a file carryingtitle:alone has not said which page it is.A disagreement must not cost a file its anchors. The first
linkindexversion skipped the whole file, matching what an unreadable one gets. A test caught why that is worse:FileExistsgoes false, so every link to that file becomesLINK BROKENacross the tree, where the right answer is the ordinary unpublished warning. It costs the file its page entry and nothing else.Plus two found by their own tests:
checkassignedr.warningsfrom the label and converter warnings, silently discarding the new lint; andlabelswas compared in order, so[a, b]vs[b, a]warned about a difference Confluence cannot represent.Also
The
metadata_sourceaddition first went in via a JSON round-trip, which reformatted the entire schema — every compact inline object exploded, 1728 lines of diff in a hand-formatted file. Reverted and applied as text: 10 lines, two definitions, plus a sharedmetadataSourceOrNull$defbeside the other*OrNullrefs.docs/github-actions.mdchanges most, since it is the document this exists to make true: its example wasupdate --page-id=12345 --force docs/some_doc.md.make checkclean. Out of scope and deliberate: creating pages in CI (a workflow would have to commit the newpage_idback),exportwriting entries, a--persist-toflag, andupdateenforcingspace/parent(#10).