From d5e29539f45501581a4f9c11d0d73b301ef69595 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 16:52:34 -0400 Subject: [PATCH 01/17] docs: plan for reading settings from markfluence.yaml Implements the design for #100: a project-wide default space and page_width, resolved flag > frontmatter > project file, plus the loader and the malformed-file handling every later key rests on. Five facts from the existing code shape it. open() is the single place a Root is built from a marker hit, so loading there gives Discover, Cache and FromPath one copy of the abort rule. internal/project cannot import internal/pagewidth -- pagewidth imports client and client holds a *project.Cache -- which is what decides where a vocabulary check can run. loadEnvFile swallows a discovery failure today, so a malformed project file would silently move .env resolution to the working directory. update resolves width before it resolves the root and has to be reordered. And update never reads space from the file at all, so a project-wide space: reaches only create until #10. Two questions the plan opened are settled in it. No url: key, and for a sharper reason than #100 gives: basic auth goes to whatever host the resolved URL names, so a committed, walked-up file naming one decides where the token is sent -- a worse version of the .env hole #136 documents. And no space: in the marker export plants, which is redundant by construction, since every exported file already carries its own space: in frontmatter. --- _plans/038_project-file-settings.md | 359 ++++++++++++++++++++++++++++ 1 file changed, 359 insertions(+) create mode 100644 _plans/038_project-file-settings.md diff --git a/_plans/038_project-file-settings.md b/_plans/038_project-file-settings.md new file mode 100644 index 0000000..c57327f --- /dev/null +++ b/_plans/038_project-file-settings.md @@ -0,0 +1,359 @@ +# Plan: read settings from `markfluence.yaml` + +Give the project file its first keys: a project-wide default `space` and +`page_width`, resolved **flag > frontmatter > project file**, plus the loader +and the malformed-file handling that every later key rests on. Implements #100, +and unblocks #139, which builds `pages:` on top of this loader. + +Today `markfluence.yaml` is a bare root marker: `internal/project` stats the +filename and nothing reads its contents (`project.go:12-16`). Use case 5 in +`_plans/025` is a hundred files in one space, each repeating `space: ENG`. That +is the duplication the file exists to absorb. + +## Two halves, and the smaller one is the keys + +The keys are almost trivial: `space` feeds one call site in `create`, +`page_width` feeds `resolveWidth` in `create` and `update`. Everything +interesting is in the loader. + +**The loader decides whether the project's boundary is known.** The root +silently decides every attachment name, bounds every read, and anchors the link +index. So a `markfluence.yaml` that cannot be understood is not a marker for a +root markfluence merely knows less about — it means the boundary is unknown, and +#100 settles that as: abort immediately, naming what is wrong, and do **not** +keep walking upward for a better marker, and do **not** fall back to the +markdown file's own directory. + +**Refusing a file from the future is the point, not a cost.** A +`markfluence.yaml` written for a newer markfluence holds keys an older binary +would ignore — and ignoring a project-wide default means publishing with the +wrong space or the wrong width, silently, everywhere at once. So the file +carries no schema version, unknown keys are fatal, and that must not be loosened +later. What should be good is the *message*: an unknown key most likely means +the binary is older than the project, and the error should say so. + +## What the code says — read 2026-09-12 + +Five facts from the existing code that shape the design. + +1. **`open()` is the single place a `Root` is built from a marker hit.** Three + call sites reach it — `Discover` (`project.go:88`), `Cache.walkAndCache` + (`cache.go:78`) and `FromPath` (`project.go:144`) — so loading inside `open()` + gives exactly one copy of the rule, and discovery, the cache, and `--root` + cannot disagree about it. + +2. **`internal/project` cannot import `internal/pagewidth`.** `pagewidth` + imports `client` (`pagewidth.go:26`) and `client.ResolveOptions.Roots` is a + `*project.Cache` (`internal/client/config.go:44`), so `project → pagewidth → + client → project` is a cycle. This is what decides where a *vocabulary* check + can run (D5). + +3. **`loadEnvFile` swallows a discovery failure.** `internal/client/config.go:151` + is `if root, err := project.Discover(cwd); err == nil`, falling back to the + working directory on any error. Once discovery can fail on a malformed + project file, that silently resolves `.env` from somewhere else. + +4. **`update` resolves width before it resolves the root.** `resolveWidth` is + `update.go:198`; `roots.Resolve` is `update.go:247`. `create` is already in the + right order (`root` at `create.go:683`, width at `696`, space at `722`). + +5. **`update` does not read `space` from the file at all.** `r.space` comes from + the live page (`update.go:224`). So a project-wide `space:` affects only + `create` until #10 makes `update` enforce and move pages. + +## Decisions + +**D1 — The keys are `space` and `page_width`.** Both already have a frontmatter +field, a resolution path, and (for width) a validator, so neither invents a +concept. Deliberately not shipped: + +- **`message`** — `--message` describes the *run*, not the page. #139's rule + ("flags describe the run; files describe the page") puts it on the flag side. +- **`parent`** — varies per file by definition (#100). +- **`url` / `username` / `cloud_id`** — not because a site URL is secret (it + is not), but because a committed, walked-up file naming a host decides where + the token is sent. Settled below, and the reason belongs on #100. + +The whitelist stores an expected **kind** per key (scalar / sequence / mapping), +not just a name, so #139 adds `"pages": mapping` rather than reworking the +reader. + +**D2 — An unknown top-level key is fatal, and the message names the likely +cause.** One line, because these strings land verbatim in `check --json`: + +``` +/repo/markfluence.yaml:3: unknown setting "spce" (known: page_width, space) -- +an unrecognized setting may mean this project needs a newer markfluence +``` + +This is the check the whole feature is for: `spce: ENG` silently ignored is +wrong for every file in the project at once. + +**D3 — A malformed project file is not a valid marker.** Loading happens in +`open()` (fact 1), so `Discover`, `Cache` and `FromPath` all abort identically. +Discovery does not continue upward and does not fall back. The error is a typed +`*project.ConfigError` so a caller can report it without the misleading +`resolving the documentation root:` prefix that `create.go:685` and +`update.go:249` currently add to everything. + +**D4 — An empty or comment-only file stays valid.** That is exactly what ships +today, what the README documents, and what `export` writes +(`cmd/export/projectfile.go:16`). It loads to an empty `Config`. + +**D5 — Load validates structure; a value's vocabulary is validated where it is +consumed, plus an offline `check` lint.** Structure means: parseable YAML, a +flat mapping, a known key, the right kind, and a single-line scalar. `space` has +no offline vocabulary to check at all (a space key is opaque until the API sees +it). `page_width` does, but `internal/project` cannot ask (fact 2) — so +`pagewidth.Declared` catches an invalid value where it already runs, with the +error naming `markfluence.yaml` rather than looking like a frontmatter problem, +and **`check` gains a project-file lint** so there is one offline way to find it +without publishing. + +The alternative is to break the cycle by making `client.ResolveOptions.Roots` an +interface, which would let the loader validate every value at load. That is a +better end state and a wider change than #100 needs — twelve `client.Resolve` +call sites plus the `roots == nil` fallback. Deferred, noted as a follow-up. + +**D6 — The project file is a default, never a conflict.** It is consulted only +when the flag and the frontmatter are both silent. This is `flag > frontmatter > +project file` with the useful property that it needs no new disagreement rules: +`create`'s existing `--space` vs frontmatter conflict error (`create.go:724`) +is untouched, because a project default never participates in a conflict. + +**D7 — `page_width` in the project file makes `update` assert the width on a +file that declares none.** `resolveWidth` returns `apply=false` today when +neither flag nor frontmatter declares (`update.go:389`); a project default makes +it `true`. That is a behavior change and it is deliberate — it is what +"declared means asserted" (**L9**) means one level up, and a project that wants +the live width left alone simply omits the key. Flag and frontmatter still win. +`create` already defaults to `max`, so there a project default only changes +*which* default. + +**D8 — Settings are per-root, so a multi-root batch gets per-file defaults.** +`Config` hangs off `Root`, which is already resolved per file and cached, so two +files under two different projects get their own defaults with no special case — +the same way `docs/root-model.md`'s "Multi-root batches are allowed" falls out +of per-file discovery. + +**D9 — `--root DIR` loads `DIR/markfluence.yaml` when there is one.** `--root` +overrides *discovery*, not the file's contents; `FromPath` already notes the +file (`project.go:137-142`), and it would be strange for the flag that declares +the root to also discard the root's settings. A malformed file there is the same +error as anywhere else. + +**D10 — No `--json` or schema change.** These are inputs, not results. The +resolved settings are reported through `ui.Debug`, beside the root. #139 adds +`metadata_source` because it needs to answer "why *that* page"; a default space +and width need no such forensics. + +**D11 — The YAML dialect gets one copy, in `internal/frontmatter`** (settled +2026-09-12). The rules +worth not duplicating are already there and were all found by probing: the +scalar node-kind whitelist (`plainScalar`), the single-line rule (`spansLines`), +every null spelling reading as `""`, and the flat-mapping refusal. The project +file is a second *use* of markfluence's YAML dialect, not a second dialect, so +`frontmatter` exposes a fence-free reader and `project` calls it. This is the +"third minimal parser" cost #100 warned about, declined. + +The alternative — extract the node primitives into a new `internal/yamlmap` that +both import — is cleaner on the name and worse on timing: #100 needs read-only +flat scalars, so the package would be designed against a guess at what #139's +write side and nested `pages:` need. Extract it when #139 makes those concrete. + +## Implementation + +### `internal/frontmatter` + +Split the fence handling from the dialect so the latter is callable on a whole +document: + +- `ReadMapping(text string) (map[string]string, map[string][]string, error)` — + parse `text` as a single flat YAML mapping and read it through the existing + whitelist. This is `parseBlock` + `toMaps` with the `---` position shift and + the `scalarFields` whitelist left out, both being frontmatter's own. +- `parseBlock`'s error formatting splits into the shared one-lining and + frontmatter's `shiftLeadingPosition`, which exists only because the block text + excludes the `---` opener. A whole file needs no shift. +- `scalarFields` stays frontmatter's; `project` passes its own key/kind table. + +Pure refactor, no behavior change, pinned by the existing 800-line test file. +The package doc comment gains a sentence: `internal/frontmatter` owns +markfluence's YAML *dialect*, and the fenced block is one use of it. + +### `internal/project` (`config.go`, new) + +- `Config` — `Space string`, `PageWidth string`. Raw strings: the vocabulary is + validated by the consumer (D5), and a raw field keeps the loader free of + `pagewidth`. +- `settings` — the key/kind whitelist (D1). +- `loadConfig(path string) (Config, error)` — read the file, `ReadMapping`, + refuse an unknown key or a wrong kind, return a `*ConfigError` carrying the + path. +- `ConfigError` — typed, so callers skip the `resolving the documentation root:` + prefix (D3). +- `Root` gains `Config`. `open()` loads it when `file != ""` (D3), so `Discover`, + `Cache.walkAndCache` and `FromPath` all get it from one place. +- An unreadable file (permissions) is an error, not an empty config: the walk + treats an unstattable *ancestor* as "not here" (`probeMarker`), but a file it + found and cannot read is a boundary it cannot establish. + +### `internal/client` + +`loadEnvFile` stops swallowing the discovery error (fact 3). Without this, a +malformed project file leaves `.env` silently resolved from the working +directory, and a command with no per-file root (`read`, `search`, `info`) never +aborts at all. + +### `cmd/create` + +`resolveWidth` and the space block already sit after `roots.Resolve` (fact 4), +so both take `root.Config` as the last fallback. `space` becomes: `--space`, then +frontmatter, then `root.Config.Space`, then the existing "no space given" error +— whose message gains the project file as a third remedy. + +### `cmd/update` + +Move `resolveWidth` (line 198) below `roots.Resolve` (line 247) so the config is +available, and thread `root.Config.PageWidth` in as the third level. Nothing +between the two depends on the ordering; the label validation that currently +follows width stays before any request, which is the property that matters +(`update.go:202-206`). + +### `cmd/check` + +A project-file lint, scoped like everything else in `check`: for each root the +run resolves, validate `Config.PageWidth` against `pagewidth.Declared` and +report an invalid value. A malformed file already fails the file through +discovery. This is the offline way to catch what D5 leaves to the consumer. + +### `cmd/export` + +`projectFileBody`'s comment and `writeProjectFile`'s doc comment both say +nothing in the file is parsed. Update both. The body stays comment-only, and the +comment itself stays accurate: the file the export plants really does carry no +settings. Why it should not carry a `space:` is settled below. + +## Tests + +- `internal/frontmatter`: `ReadMapping` on a whole document — flat mapping, + comment-only, empty, a nested value, a sequence, a `|` block, a duplicate key, + a tab indent, an anchor/alias/tag (the whitelist), a multi-line scalar, a + `...` second document. These mirror the fenced tests and must agree with them, + which is the point of sharing the reader. +- `internal/project`: a valid file; comment-only and empty (D4); unknown key, + with the message asserted (D2); wrong kind per key; a malformed file aborting + `Discover` **without** continuing to an ancestor that has a good one (D3); + the same through `Cache` (including that a second `Resolve` under the same + root does not re-read the file); the same through `FromPath` (D9); an + unreadable file; `ConfigError` identified by `errors.As`. +- `internal/client`: a malformed project file makes `Resolve` fail rather than + reading `.env` from the working directory. +- `cmd/create`: project `space` used when flag and frontmatter are silent; flag + wins; frontmatter wins; the flag-vs-frontmatter conflict error unchanged; an + invalid project `page_width` names `markfluence.yaml`. +- `cmd/update`: project `page_width` asserted on a file declaring none (D7); + frontmatter and flag each win; no key means no width request at all. +- Multi-root: one invocation, two projects, two different defaults (D8). +- `cmd/check`: an invalid project `page_width` reported offline; a malformed + file fails the file. + +## Docs + +- `docs/root-model.md` — "Its existence is its whole meaning. Nothing in it is + parsed or read" is now false. Rewrite that section: the settings, the + precedence chain, why an unknown key is fatal, and that the file is still + never *executed* (the CVE-2022-24765 framing below it stands, and matters + more now that the file is read). +- `docs/markdown_file.md` — the precedence chain beside the field table, and + which fields have a project-wide default. +- `README.md` — the `markfluence.yaml` block (line ~482) gains the keys; one + line, not a reference. +- `CLAUDE.md` — `internal/project` has **no bullet in the Layout list** today. + Add one, covering discovery, the `Root`/`Cache` split, and now the loader, + the abort rule, and why the vocabulary check lives outside the package. This + is the rule about a change that adds a parser belonging in CLAUDE.md. +- `docs/guarantees.md` — a note under **L2** (`invocation-independent`): a + declared, on-disk default *strengthens* L2, since a `--space` flag is + invocation state and a project file is not. No status change. +- `_plans/038` (this file). + +## Commits + +1. `refactor(frontmatter): expose the YAML dialect reader for whole documents` +2. `feat(project): read markfluence.yaml, refusing a file it cannot understand` +3. `fix(client): stop swallowing a root-discovery failure when locating .env` +4. `feat(create): default space and page_width from markfluence.yaml` +5. `feat(update): default page_width from markfluence.yaml` +6. `feat(check): validate markfluence.yaml offline` +7. `docs(export): the marker file is read now, not merely present` +8. `docs: record the project-file settings and the precedence chain` + +## The two questions, answered — 2026-09-12 + +Both were left open when this plan was written and both are now settled as +**no**, with reasons better than the ones the plan first gave. + +### No `url:` (or `username:`) + +#100's stated reason is that credentials resolve **flag > env > `.env`** and the +project file answers a different question. That reason is weak on its own terms +and invites relitigating, because a site URL genuinely is not a secret — it is +why `--cloud-id` is allowed to be a flag. + +The real reason is sharper: **markfluence sends basic auth on every request to +whatever host the resolved URL names.** A `url:` in a committed, walked-up +project file therefore decides where `CONFLUENCE_TOKEN` is sent, so a pull +request adding one line to `markfluence.yaml` redirects a CI run's token to a +host the author chose. That is credential exfiltration by PR, and it is a +strictly worse version of a hole the project already documents and knowingly +leaves open for `.env` (#136: a writable `.env` "rewriting `CONFLUENCE_URL` +... would redirect the token") — worse because `markfluence.yaml` is committed, +shared, walked up to from a subdirectory, and reviewed as content rather than as +configuration. + +The asymmetry against the keys this plan does add is the whole argument. A wrong +`space:` publishes to the wrong place in *your own* instance: visible, and +recoverable with `fix`. A wrong `url:` hands out the token: neither. + +`username:` fails a duller test — it is per-person, not per-project, so it has no +business in a committed file even setting the redirect risk aside. + +### No `space:` in the tree `export` plants + +This one is moot by construction, which the plan did not notice: **every +exported file already carries `space:` in its own frontmatter.** +`pagedoc.Frontmatter` fills it from `client.SpaceKeyFromWebUI(page.Links.WebUI)` +and `RenderFrontmatter` emits it (`internal/pagedoc/pagedoc.go:387-441`), as it +does `page_width`. Frontmatter beats the project file (D6), so a `space:` in the +planted marker would be a setting that every file in the tree overrides. + +It would matter only for a file somebody *adds* to an exported tree later — and +even there inconsistently, since `writeProjectFile` never overwrites an existing +marker (S3), so the setting would be present or absent depending on whether one +was already there. + +## Out of scope + +- **`pages:`** — #139, blocked on this. +- **`message:`** — D1; a run descriptor, not page metadata. +- **Credentials in the project file** — D1, and settled above: a `url:` would + decide where the token goes. +- **`markfluence init`** — #5. The file is still created by hand. +- **`export` writing a `space:`** — settled above: redundant by construction, + since every exported file already carries its own `space:`. +- **Breaking `client → project`** to allow load-time vocabulary validation — D5, + a follow-up. +- **A `url:` cross-check** — the safe direction of the key settled above: the + project file *declares* the site and markfluence refuses to publish when the + resolved URL disagrees, fencing the token instead of directing it. It would + catch "published our docs to the wrong Confluence." Noted rather than filed; + it waits for someone to want it. + +## Follow-ups + +- Make `client.ResolveOptions.Roots` an interface, then move `page_width`'s + vocabulary check into the loader and drop `check`'s lint (D5). +- A test that every key in `settings` appears in `docs/root-model.md`, the way + `TestSubcommandsDocumentThemselves` pins `--help`. A silently undocumented + project-wide setting is the same class of problem as a silently ignored one. From a4b3881d520db4c1b2e146c5c188c18094606705 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 16:56:47 -0400 Subject: [PATCH 02/17] refactor(frontmatter): expose the YAML dialect reader for whole documents Move the reader -- the scalar node-kind whitelist, the single-line rule, the null spellings, the flat-mapping refusal -- into dialect.go, and add ReadMapping so a document with no "---" fences can be read through it. internal/project needs these rules to read markfluence.yaml (#100) and none of them were reasoned about: each turned up by probing the pinned goccy, so a second copy for a second file would be a second set of the same bugs. What this package really owns is markfluence's YAML dialect; the fenced block is one use of it. Two things stay frontmatter's own. The "---" position shift exists because a block's text excludes its opener, and a whole file needs no correction, so it is a fixup on the reader rather than part of the formatting. And scalarFields -- the keys that must not hold a sequence -- protects `parent: [x]` from reading as absent, which a project file has no equivalent of, and which #139's pages: entries will need to allow. The two nouns a diagnostic uses are now a parameter (Dialect), because `frontmatter must be a flat mapping` and `setting "space" must be a single scalar value` are both right and neither noun works in the other's sentence. Every message frontmatter itself emits is unchanged. ReadMapping reports items in source order with their line numbers rather than as a map: a caller rejecting an unknown key has to be able to say where it is, and a map would lose that. --- internal/frontmatter/dialect.go | 305 +++++++++++++++++++++++++++ internal/frontmatter/dialect_test.go | 178 ++++++++++++++++ internal/frontmatter/frontmatter.go | 215 ++----------------- 3 files changed, 498 insertions(+), 200 deletions(-) create mode 100644 internal/frontmatter/dialect.go create mode 100644 internal/frontmatter/dialect_test.go diff --git a/internal/frontmatter/dialect.go b/internal/frontmatter/dialect.go new file mode 100644 index 0000000..4378dd6 --- /dev/null +++ b/internal/frontmatter/dialect.go @@ -0,0 +1,305 @@ +package frontmatter + +// markfluence's YAML dialect: how a mapping of scalars and sequences is read, +// and what is refused. +// +// The fenced frontmatter block is one use of this, not the only one -- +// markfluence.yaml is read by internal/project through ReadMapping (#100). +// Every rule here was found by probing the pinned goccy rather than reasoned +// about, which is why a second copy of them for a second file would be a +// second set of the same bugs: the scalar node-kind whitelist (plainScalar), +// the single-line rule (spansLines), every null spelling reading as "", and +// the refusal of anything that is not a flat mapping. + +import ( + "errors" + "fmt" + "regexp" + "strings" + + "github.com/goccy/go-yaml" + "github.com/goccy/go-yaml/ast" + "github.com/goccy/go-yaml/parser" +) + +// Dialect names the document a diagnostic is about, so one copy of the reader +// can report for two different files without either borrowing the other's +// wording. +// +// Doc names the document as a whole ("frontmatter", "a project file") and Item +// names one of its keys ("frontmatter", "setting"). They are separate because +// the two kinds of message read differently: `frontmatter must be a flat +// mapping` and `setting "space" must be a single scalar value` are both right, +// and neither noun works in the other's sentence. +type Dialect struct { + Doc string + Item string +} + +// Item is one key/value pair of a mapping, in source order. +// +// List is nil for a scalar value and non-nil (possibly empty) for a sequence, +// which is what distinguishes `labels: []` from an absent key. Line is +// 1-based within the text passed to ReadMapping, and is carried because a +// caller rejecting a key -- an unknown setting in a project file -- has to be +// able to say where it is. +type Item struct { + Key string + Line int + Value string + List []string +} + +// ReadMapping reads text as a single flat YAML mapping, applying every rule +// this file describes. An empty document and a comment-only one are both empty +// mappings rather than errors, since neither is invalid YAML -- and for +// markfluence.yaml that is the shape that ships today. +// +// It reports items in source order rather than as a map, because a caller +// validating keys needs their positions and a map would lose them. +func (d Dialect) ReadMapping(text string) ([]Item, error) { + r := reader{Dialect: d} + b, err := r.parse(text) + if err != nil { + return nil, err + } + return r.items(b.mapping) +} + +// reader is the dialect plus the two things that differ per document and are +// nobody else's business. +// +// scalarOnly names keys that must not hold a sequence, which is frontmatter's +// own concern (a `parent: [x]` read as absent published at the space root and +// wrote `parent: null` over the author's intent). fixup adjusts a parse +// error's text; frontmatter's block excludes the "---" opener, so its +// positions are one line short, and a whole file needs no correction at all. +type reader struct { + Dialect + scalarOnly map[string]bool + fixup func(string) string +} + +// blockReader reads a fenced frontmatter block. Package-level because the +// write-side verification helpers (readsBackAs, readsBackInSeqAs) re-read a +// node they are about to emit and discard the message, so threading a reader +// to them would carry only the parts they ignore. +var blockReader = reader{ + Dialect: Dialect{Doc: "frontmatter", Item: "frontmatter"}, + scalarOnly: scalarFields, + fixup: shiftLeadingPosition, +} + +// parse parses a document's text into a flat mapping. Any other shape (a bare +// scalar, a top-level list) is an error. +func (r reader) parse(text string) (*block, error) { + f, err := parser.ParseBytes([]byte(text), parser.ParseComments) + if err != nil { + return nil, r.parseError(err) + } + // A "..." line starts a second document, and reading only the first would + // drop every key after it without a word: `update` would then report "no + // page id" about a file that visibly has one. + if len(f.Docs) > 1 { + return nil, fmt.Errorf(`%s must be a single document: remove the "..." line`, r.Doc) + } + if len(f.Docs) == 0 || f.Docs[0].Body == nil { + return &block{mapping: emptyMapping()}, nil + } + switch b := f.Docs[0].Body.(type) { + case *ast.MappingNode: + return &block{mapping: b}, nil + case *ast.MappingValueNode: + m := emptyMapping() + m.Values = append(m.Values, b) + return &block{mapping: m}, nil + case *ast.CommentGroupNode: + return &block{mapping: emptyMapping(), orphan: b}, nil + default: + return nil, fmt.Errorf("%s must be a flat mapping of %s: value pairs, found %s", + r.Doc, r.Item, b.Type()) + } +} + +// parseError reduces a goccy error to a single line and applies the reader's +// position correction. +// +// goccy's default Error() renders a multi-line source excerpt with ASCII +// pointer art, which would land verbatim in check --json's error string. +// Known limit: a duplicate-key message embeds a second position ("already +// defined at [1:1]") that no fixup touches. +func (r reader) parseError(err error) error { + msg := yaml.FormatError(err, false, false) + if r.fixup != nil { + msg = r.fixup(msg) + } + return errors.New(msg) +} + +// positionRE matches a leading "[line:col] " position stamp. +var positionRE = regexp.MustCompile(`^\[(\d+):(\d+)\] `) + +// shiftLeadingPosition rewrites a leading [line:col] to account for the "---" +// line that opens a frontmatter block. +func shiftLeadingPosition(msg string) string { + m := positionRE.FindStringSubmatch(msg) + if m == nil { + return msg + } + var line, col int + if _, err := fmt.Sscanf(m[1]+" "+m[2], "%d %d", &line, &col); err != nil { + return msg + } + return fmt.Sprintf("[%d:%d] %s", line+1, col, msg[len(m[0]):]) +} + +// scalar reads a mapping value as a string, rejecting anything that spans more +// than one line. +// +// Every spelling of null -- an absent value, "null", "~", "Null" -- reads as +// "", so a null is unset whatever the author wrote. The old parser mapped only +// the literal "null", which meant "parent: ~" read as though it were a page id. +func (r reader) scalar(key string, n ast.Node) (string, error) { + if spansLines(n.GetToken().Origin) { + return "", fmt.Errorf("%s %q must be a single-line scalar; "+ + "a value split over several lines is not supported", r.Item, key) + } + return r.plainScalar(key, n) +} + +// plainScalar is the node-kind whitelist shared by scalar and element. It is a +// whitelist because every other node kind -- an anchor, an alias, a tag, a "|" +// literal block -- reports GetToken().Value as its indicator character rather +// than its content, so a blacklist of sequences and mappings would silently +// read "&" or "|". A "- |-" sequence element is the case that makes this +// load-bearing twice over: its token does not span lines, so the whitelist is +// the only thing that catches it. +func (r reader) plainScalar(key string, n ast.Node) (string, error) { + switch v := n.(type) { + case *ast.NullNode: + return "", nil + case *ast.StringNode, *ast.IntegerNode, *ast.FloatNode, *ast.BoolNode, + *ast.InfinityNode, *ast.NanNode: + return v.GetToken().Value, nil + default: + return "", fmt.Errorf("%s %q must be a single scalar value, found %s", + r.Item, key, n.Type()) + } +} + +// element reads one sequence element. It shares scalar's whitelist but applies +// the line rule to the origin trimmed at *both* ends, because a leading +// newline in an element's origin is structure rather than content: it means the +// element began on a new line, which is true of every block item and of a flow +// sequence wrapped across lines. Trimming only the right, as scalar does, would +// refuse "[a,\n b]" for no reason. +// +// What it still refuses is an element whose own value runs past its line -- a +// plain scalar continued on the next line, or a multi-line quoted one -- for +// the same reason scalar does. +func (r reader) element(key string, n ast.Node) (string, error) { + if strings.Contains(strings.TrimSpace(n.GetToken().Origin), "\n") { + return "", fmt.Errorf("%s %q must be a single-line scalar; "+ + "a list element split over several lines is not supported", r.Item, key) + } + return r.plainScalar(key, n) +} + +// sequence reads a mapping value as a list of strings. Both YAML spellings are +// accepted -- the flow form "[a, b]" and the block form of "- a" lines -- since +// goccy parses both correctly and a block list survives Normalize intact, so +// refusing one would mean rejecting a file that was understood perfectly. What +// the style does decide is how a rewrite is emitted; see setField. +// +// The element index is carried into the key so a message points at the item +// that is wrong rather than at the field. +func (r reader) sequence(key string, n *ast.SequenceNode) ([]string, error) { + out := make([]string, 0, len(n.Values)) + for i, e := range n.Values { + s, err := r.element(fmt.Sprintf("%s[%d]", key, i), e) + if err != nil { + return nil, err + } + out = append(out, s) + } + return out, nil +} + +// spansLines reports whether a token's source text runs past its own line. +// +// This is what enforces the single-line half of the contract, and it has to be +// enforced at read time rather than trusted: an untouched key is re-emitted +// from the node the parser produced, and goccy's re-emission of a parsed node +// is not identity. +// +// What that costs differs by shape, measured against the pinned goccy rather +// than assumed. A "|" or ">" block is the one that breaks outright -- it +// re-emits as a block, which plainScalar's whitelist then refuses, so a write +// would produce a file markfluence cannot read, in create only after the page +// had been made. A plain scalar continued on the next line and a multi-line +// quoted one both re-emit folded onto one line, which parses but silently +// rewrites the author's file. Neither is something to do on the way past while +// setting some unrelated field, so both are refused up front. +// +// Trailing newlines and spaces are stripped first because a token's origin runs +// up to the next one, so even `title: T` carries the line break that follows it. +// A value markfluence wrote is never affected: a newline inside one is emitted +// as a two-character \n escape inside a double-quoted scalar, which occupies a +// single physical line. +func spansLines(origin string) bool { + return strings.Contains(strings.TrimRight(origin, "\n\t "), "\n") +} + +// items reads a mapping into its key/value pairs, in source order. +func (r reader) items(m *ast.MappingNode) ([]Item, error) { + out := make([]Item, 0, len(m.Values)) + for _, v := range m.Values { + key := v.Key.GetToken().Value + it := Item{Key: key, Line: v.Key.GetToken().Position.Line} + if seq, ok := v.Value.(*ast.SequenceNode); ok { + if r.scalarOnly[key] { + return nil, fmt.Errorf( + "%s %q must be a single value, not a list", r.Item, key) + } + l, err := r.sequence(key, seq) + if err != nil { + return nil, err + } + it.List = l + out = append(out, it) + continue + } + s, err := r.scalar(key, v.Value) + if err != nil { + return nil, err + } + it.Value = s + out = append(out, it) + } + return out, nil +} + +// maps reads a mapping into the two maps frontmatter's own callers use: scalars +// by key, and sequences by key. +// +// A sequence-valued key is absent from the scalar map rather than present in +// some flattened spelling. Leaving it there would be worse than absent -- +// MarkdownFile.field would hand update a title of "[a, b]" -- and re-typing the +// scalar map to hold both would touch every caller for no gain. The parser +// learns a kind, not a name: nothing here knows which keys are lists. +func (r reader) maps(m *ast.MappingNode) (map[string]string, map[string][]string, error) { + items, err := r.items(m) + if err != nil { + return nil, nil, err + } + fm := make(map[string]string, len(items)) + lists := map[string][]string{} + for _, it := range items { + if it.List != nil { + lists[it.Key] = it.List + continue + } + fm[it.Key] = it.Value + } + return fm, lists, nil +} diff --git a/internal/frontmatter/dialect_test.go b/internal/frontmatter/dialect_test.go new file mode 100644 index 0000000..ca51d04 --- /dev/null +++ b/internal/frontmatter/dialect_test.go @@ -0,0 +1,178 @@ +package frontmatter + +import ( + "reflect" + "strings" + "testing" +) + +// testDialect is the wording internal/project uses, so these tests pin the +// nouns a project-file diagnostic will actually carry. +var testDialect = Dialect{Doc: "a project file", Item: "setting"} + +func TestReadMappingReadsScalarsAndSequences(t *testing.T) { + items, err := testDialect.ReadMapping("space: ENG\nlabels: [a, b]\nempty: []\n") + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + want := []Item{ + {Key: "space", Line: 1, Value: "ENG"}, + {Key: "labels", Line: 2, List: []string{"a", "b"}}, + {Key: "empty", Line: 3, List: []string{}}, + } + if !reflect.DeepEqual(items, want) { + t.Errorf("items = %#v, want %#v", items, want) + } +} + +// A nil List is what distinguishes a scalar from a sequence, and an empty but +// non-nil one is what makes "labels: []" expressible. A caller that checked +// len(List) instead would read the two the same way. +func TestReadMappingDistinguishesEmptyListFromScalar(t *testing.T) { + items, err := testDialect.ReadMapping("a: []\nb: \"\"\n") + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + if items[0].List == nil { + t.Error("a: [] read with a nil List; an empty sequence must stay a sequence") + } + if items[1].List != nil { + t.Errorf(`b: "" read with List = %#v, want nil`, items[1].List) + } +} + +// The project file that ships today is a comment and nothing else, and export +// plants exactly that. Both it and an empty file must load as no settings +// rather than as a malformed document. +func TestReadMappingAcceptsEmptyAndCommentOnly(t *testing.T) { + for name, text := range map[string]string{ + "empty": "", + "blank lines": "\n\n", + "comment only": "# Marks the root of a markfluence project.\n# https://example.com\n", + } { + t.Run(name, func(t *testing.T) { + items, err := testDialect.ReadMapping(text) + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + if len(items) != 0 { + t.Errorf("items = %#v, want none", items) + } + }) + } +} + +func TestReadMappingLineNumbersAreDocumentRelative(t *testing.T) { + // No "---" opener to account for, unlike a fenced block: line 1 of the + // text is line 1 of the file. + items, err := testDialect.ReadMapping("# comment\n\nspace: ENG\n") + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + if len(items) != 1 || items[0].Line != 3 { + t.Errorf("items = %#v, want space on line 3", items) + } +} + +func TestReadMappingEveryNullSpellingIsEmpty(t *testing.T) { + items, err := testDialect.ReadMapping("a: null\nb: Null\nc: ~\nd:\n") + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + for _, it := range items { + if it.Value != "" { + t.Errorf("%s = %q, want empty", it.Key, it.Value) + } + } +} + +func TestReadMappingRefusals(t *testing.T) { + tests := map[string]struct { + text string + want string + }{ + "nested mapping": {"space:\n key: ENG\n", `setting "space" must be a single scalar value`}, + "literal block": {"space: |\n ENG\n", `setting "space" must be a single scalar value`}, + "folded block": {"space: >\n ENG\n", `setting "space" must be a single scalar value`}, + "anchor": {"space: &anchor ENG\n", `setting "space" must be a single scalar value`}, + "tag": {"space: !!str ENG\n", `setting "space" must be a single scalar value`}, + "block element block": {"labels:\n - |-\n a\n", `setting "labels[0]" must be a single scalar value`}, + "continued scalar": {"space: ENG\n OPS\n", `must be a single-line scalar`}, + "top-level sequence": {"- ENG\n", "a project file must be a flat mapping of setting: value pairs"}, + "bare scalar": {"ENG\n", "a project file must be a flat mapping of setting: value pairs"}, + "second document": {"space: ENG\n...\nspace: OPS\n", "a project file must be a single document"}, + "duplicate key": {"space: ENG\nspace: OPS\n", "already defined"}, + "tab indent": {"labels:\n\t- a\n", "cannot start any token"}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + _, err := testDialect.ReadMapping(tc.text) + if err == nil { + t.Fatalf("ReadMapping(%q) succeeded, want an error", tc.text) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to contain %q", err, tc.want) + } + }) + } +} + +// A parse error must stay one line wherever it comes from: these strings land +// verbatim in check --json and in a project-file load failure. +func TestReadMappingErrorsAreOneLine(t *testing.T) { + _, err := testDialect.ReadMapping("space: ENG\nspace: OPS\n") + if err == nil { + t.Fatal("want an error") + } + if strings.Contains(err.Error(), "\n") { + t.Errorf("error spans lines:\n%s", err) + } +} + +// The whole point of sharing the reader: a document read as a bare mapping and +// the same text read as a fenced block must agree about every value. A second +// copy of the dialect is how they start to differ. +func TestReadMappingAgreesWithFencedBlock(t *testing.T) { + const body = "title: \"Deploy Runbook: Part 2\"\npage_id: 123\nparent: ~\nlabels: [runbook, ci/cd]\n" + + items, err := testDialect.ReadMapping(body) + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + scalars := map[string]string{} + lists := map[string][]string{} + for _, it := range items { + if it.List != nil { + lists[it.Key] = it.List + continue + } + scalars[it.Key] = it.Value + } + + mf, err := Parse("page.md", "---\n"+body+"---\nbody\n") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if !reflect.DeepEqual(scalars, mf.Frontmatter) { + t.Errorf("scalars = %#v, frontmatter = %#v", scalars, mf.Frontmatter) + } + if !reflect.DeepEqual(lists, mf.Lists) { + t.Errorf("lists = %#v, frontmatter lists = %#v", lists, mf.Lists) + } +} + +// scalarOnly is frontmatter's own rule, not the dialect's: a project file has +// no "parent" field to protect, and #139's pages: entries will want a mapping +// where frontmatter allows none. +func TestReadMappingDoesNotApplyFrontmattersScalarOnlyKeys(t *testing.T) { + items, err := testDialect.ReadMapping("parent: [a, b]\n") + if err != nil { + t.Fatalf("ReadMapping: %v", err) + } + if len(items) != 1 || items[0].List == nil { + t.Fatalf("items = %#v, want parent as a list", items) + } + if _, err := Parse("page.md", "---\nparent: [a, b]\n---\n"); err == nil { + t.Error("frontmatter accepted a list-valued parent; scalarFields must still apply there") + } +} diff --git a/internal/frontmatter/frontmatter.go b/internal/frontmatter/frontmatter.go index 043e554..6aedc93 100644 --- a/internal/frontmatter/frontmatter.go +++ b/internal/frontmatter/frontmatter.go @@ -5,11 +5,18 @@ // flat -- no nesting -- but a value may be a scalar or a sequence of scalars, // in either YAML spelling: the flow form "[a, b]" or the block form of "- a" // lines. Every scalar, including a sequence's elements, must occupy a single -// line; that is enforced by scalarValue and elementValue rather than assumed by -// a line-splitting parser that could not see a violation. Scalars land in +// line; that is enforced at read time rather than assumed by a line-splitting +// parser that could not see a violation. Scalars land in // MarkdownFile.Frontmatter and sequences in MarkdownFile.Lists, so a key // appears in exactly one map and nothing here knows which keys are lists. // +// What this package owns is markfluence's YAML *dialect* -- the node-kind +// whitelist, the single-line rule, every null spelling reading as "" -- and the +// fenced block is one use of it rather than the only one. dialect.go holds the +// reader, and ReadMapping exposes it for a whole document: internal/project +// reads markfluence.yaml through it (#100), so the two files cannot come to +// disagree about what YAML markfluence understands. +// // Writes go through valueNodeFor and elementNodeFor, which verify their own // output: they emit with goccy's chosen style, re-read the result, and fall // back to a double-quoted scalar when the two disagree. goccy's default is @@ -123,198 +130,6 @@ func emptyMapping() *ast.MappingNode { return ast.Mapping(token.New("", "", pos()), false) } -// parseBlock parses a frontmatter block's inner text. An empty block, and one -// holding only comments, are empty mappings rather than errors -- neither is -// invalid YAML. Any other shape (a bare scalar, a top-level list) is an error. -func parseBlock(fmText string) (*block, error) { - f, err := parser.ParseBytes([]byte(fmText), parser.ParseComments) - if err != nil { - return nil, formatParseError(err) - } - // A "..." line inside the block starts a second document, and reading only - // the first would drop every key after it without a word: `update` would - // then report "no page id" about a file that visibly has one. - if len(f.Docs) > 1 { - return nil, errors.New(`frontmatter must be a single document: remove the "..." line`) - } - if len(f.Docs) == 0 || f.Docs[0].Body == nil { - return &block{mapping: emptyMapping()}, nil - } - switch b := f.Docs[0].Body.(type) { - case *ast.MappingNode: - return &block{mapping: b}, nil - case *ast.MappingValueNode: - m := emptyMapping() - m.Values = append(m.Values, b) - return &block{mapping: m}, nil - case *ast.CommentGroupNode: - return &block{mapping: emptyMapping(), orphan: b}, nil - default: - return nil, fmt.Errorf("frontmatter must be a flat mapping of key: value pairs, found %s", - b.Type()) - } -} - -// formatParseError reduces a goccy error to a single line and corrects its -// position for the "---" opener, which the block text does not include. -// -// goccy's default Error() renders a multi-line source excerpt with ASCII -// pointer art, which would land verbatim in check --json's error string. -// Known limit: a duplicate-key message embeds a second position ("already -// defined at [1:1]") that stays block-relative. -func formatParseError(err error) error { - msg := yaml.FormatError(err, false, false) - return errors.New(shiftLeadingPosition(msg)) -} - -// positionRE matches a leading "[line:col] " position stamp. -var positionRE = regexp.MustCompile(`^\[(\d+):(\d+)\] `) - -// shiftLeadingPosition rewrites a leading [line:col] to account for the "---" -// line that opens the block. -func shiftLeadingPosition(msg string) string { - m := positionRE.FindStringSubmatch(msg) - if m == nil { - return msg - } - var line, col int - if _, err := fmt.Sscanf(m[1]+" "+m[2], "%d %d", &line, &col); err != nil { - return msg - } - return fmt.Sprintf("[%d:%d] %s", line+1, col, msg[len(m[0]):]) -} - -// scalarValue reads a mapping value as a string, rejecting anything that spans -// more than one line. -// -// Every spelling of null -- an absent value, "null", "~", "Null" -- reads as -// "", so a null is unset whatever the author wrote. The old parser mapped only -// the literal "null", which meant "parent: ~" read as though it were a page id. -func scalarValue(key string, n ast.Node) (string, error) { - if spansLines(n.GetToken().Origin) { - return "", fmt.Errorf("frontmatter %q must be a single-line scalar; "+ - "a value split over several lines is not supported", key) - } - return plainScalar(key, n) -} - -// plainScalar is the node-kind whitelist shared by scalarValue and -// elementValue. It is a whitelist because every other node kind -- an anchor, -// an alias, a tag, a "|" literal block -- reports GetToken().Value as its -// indicator character rather than its content, so a blacklist of sequences and -// mappings would silently read "&" or "|". A "- |-" sequence element is the -// case that makes this load-bearing twice over: its token does not span lines, -// so the whitelist is the only thing that catches it. -func plainScalar(key string, n ast.Node) (string, error) { - switch v := n.(type) { - case *ast.NullNode: - return "", nil - case *ast.StringNode, *ast.IntegerNode, *ast.FloatNode, *ast.BoolNode, - *ast.InfinityNode, *ast.NanNode: - return v.GetToken().Value, nil - default: - return "", fmt.Errorf("frontmatter %q must be a single scalar value, found %s", - key, n.Type()) - } -} - -// elementValue reads one sequence element. It shares scalarValue's whitelist -// but applies the line rule to the origin trimmed at *both* ends, because a -// leading newline in an element's origin is structure rather than content: it -// means the element began on a new line, which is true of every block item and -// of a flow sequence wrapped across lines. Trimming only the right, as -// scalarValue does, would refuse "[a,\n b]" for no reason. -// -// What it still refuses is an element whose own value runs past its line -- a -// plain scalar continued on the next line, or a multi-line quoted one -- for -// the same reason scalarValue does. -func elementValue(key string, n ast.Node) (string, error) { - if strings.Contains(strings.TrimSpace(n.GetToken().Origin), "\n") { - return "", fmt.Errorf("frontmatter %q must be a single-line scalar; "+ - "a list element split over several lines is not supported", key) - } - return plainScalar(key, n) -} - -// sequenceValue reads a mapping value as a list of strings. Both YAML spellings -// are accepted -- the flow form "[a, b]" and the block form of "- a" lines -- -// since goccy parses both correctly and a block list survives Normalize intact, -// so refusing one would mean rejecting a file that was understood perfectly. -// What the style does decide is how a rewrite is emitted; see setField. -// -// The element index is carried into the key so a message points at the item -// that is wrong rather than at the field. -func sequenceValue(key string, n *ast.SequenceNode) ([]string, error) { - out := make([]string, 0, len(n.Values)) - for i, e := range n.Values { - s, err := elementValue(fmt.Sprintf("%s[%d]", key, i), e) - if err != nil { - return nil, err - } - out = append(out, s) - } - return out, nil -} - -// spansLines reports whether a token's source text runs past its own line. -// -// This is what enforces the single-line half of the contract, and it has to be -// enforced at read time rather than trusted: an untouched key is re-emitted -// from the node the parser produced, and goccy's re-emission of a parsed node -// is not identity. -// -// What that costs differs by shape, measured against the pinned goccy rather -// than assumed. A "|" or ">" block is the one that breaks outright -- it -// re-emits as a block, which plainScalar's whitelist then refuses, so a write -// would produce a file markfluence cannot read, in create only after the page -// had been made. A plain scalar continued on the next line and a multi-line -// quoted one both re-emit folded onto one line, which parses but silently -// rewrites the author's file. Neither is something to do on the way past while -// setting some unrelated field, so both are refused up front. -// -// Trailing newlines and spaces are stripped first because a token's origin runs -// up to the next one, so even `title: T` carries the line break that follows it. -// A value markfluence wrote is never affected: a newline inside one is emitted -// as a two-character \n escape inside a double-quoted scalar, which occupies a -// single physical line. -func spansLines(origin string) bool { - return strings.Contains(strings.TrimRight(origin, "\n\t "), "\n") -} - -// toMaps reads a mapping into the two maps every caller uses: scalars by key, -// and sequences by key. -// -// A sequence-valued key is absent from the scalar map rather than present in -// some flattened spelling. Leaving it there would be worse than absent -- -// MarkdownFile.field would hand update a title of "[a, b]" -- and re-typing the -// scalar map to hold both would touch every caller for no gain. The parser -// learns a kind, not a name: nothing here knows which keys are lists. -func toMaps(m *ast.MappingNode) (map[string]string, map[string][]string, error) { - fm := make(map[string]string, len(m.Values)) - lists := map[string][]string{} - for _, v := range m.Values { - key := v.Key.GetToken().Value - if seq, ok := v.Value.(*ast.SequenceNode); ok { - if scalarFields[key] { - return nil, nil, fmt.Errorf( - "frontmatter %q must be a single value, not a list", key) - } - l, err := sequenceValue(key, seq) - if err != nil { - return nil, nil, err - } - lists[key] = l - continue - } - s, err := scalarValue(key, v.Value) - if err != nil { - return nil, nil, err - } - fm[key] = s - } - return fm, lists, nil -} - // --- writing ------------------------------------------------------------------ // isDigits reports whether s is one or more ASCII digits. Local rather than @@ -391,7 +206,7 @@ func readsBackAs(n ast.Node, want string) bool { if _, ok := parsed.Values[0].Value.(*ast.StringNode); !ok { return false } - got, err := scalarValue("v", parsed.Values[0].Value) + got, err := blockReader.scalar("v", parsed.Values[0].Value) return err == nil && got == want } @@ -472,7 +287,7 @@ func readsBackInSeqAs(n ast.Node, want string, flow bool) bool { if _, ok := parsed.Values[0].(*ast.StringNode); !ok { return false } - got, err := elementValue("v[0]", parsed.Values[0]) + got, err := blockReader.element("v[0]", parsed.Values[0]) return err == nil && got == want } @@ -606,7 +421,7 @@ func updateField(content string, f Field) (string, error) { if loc == nil { return Render([]Field{f}) + content, nil } - b, err := parseBlock(content[loc[2]:loc[3]]) + b, err := blockReader.parse(content[loc[2]:loc[3]]) if err != nil { return "", err } @@ -669,7 +484,7 @@ func Normalize(content string) (string, bool, error) { if loc == nil { return content, false, nil } - b, err := parseBlock(content[loc[2]:loc[3]]) + b, err := blockReader.parse(content[loc[2]:loc[3]]) if err != nil { return "", false, err } @@ -749,11 +564,11 @@ func Parse(filename, content string) (*MarkdownFile, error) { Frontmatter: map[string]string{}, Lists: map[string][]string{}, Body: content, }, nil } - b, err := parseBlock(content[loc[2]:loc[3]]) + b, err := blockReader.parse(content[loc[2]:loc[3]]) if err != nil { return nil, fmt.Errorf("%s: %w", filename, err) } - fm, lists, err := toMaps(b.mapping) + fm, lists, err := blockReader.maps(b.mapping) if err != nil { return nil, fmt.Errorf("%s: %w", filename, err) } From 3bdd6b4913468e75616ac1e16814e3e187705bbf Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 16:59:55 -0400 Subject: [PATCH 03/17] feat(project): read markfluence.yaml, refusing a file it cannot understand The project file has been a bare marker whose existence was its whole meaning. It now declares project-wide settings -- space and page_width -- and a file that cannot be understood is not a valid marker: discovery fails rather than walking on to an ancestor that happens to have a good one, and never falls back to the markdown file's own directory. The root decides every attachment name, bounds every read, and anchors the link index, so a file that cannot be understood means the project's boundary is unknown, and guessing is worse than stopping (#100). An unrecognized key is fatal, and that is the point rather than a cost. A silently ignored `spce: ENG` is wrong for every file in the project at once, and a project file written for a newer markfluence holds keys this binary would ignore -- so the error says an older binary is the likely cause. The file carries no schema version, deliberately. Loading happens in open(), the single place a Root is built from a marker hit: Discover, Cache.walkAndCache and FromPath all reach it, so the rule exists in one copy. It runs before os.OpenRoot so a refusal leaks no handle. --root loads the file too -- it overrides discovery, not the root's own settings. Structure is checked here; a vocabulary is not. internal/pagewidth imports internal/client and internal/client holds a *project.Cache, so this package cannot ask whether "huge" is a width -- pagewidth.Declared catches that where it already runs, and check gains an offline lint for it. Config holds no url or token, and the reason is sharper than "credentials are a different chain": basic auth goes to whatever host the resolved URL names, so a committed, walked-up file naming one would decide where the token is sent. That is a worse version of the .env hole #136 documents. An empty file and a comment-only one stay valid -- that is what ships, what the README documents, and what export plants. A test fixture that wrote the literal "bytes" into markfluence.yaml is now a real marker; the refusal found it. --- cmd/attachmentupload/attachmentupload_test.go | 15 +- internal/project/config.go | 159 ++++++++++ internal/project/config_test.go | 285 ++++++++++++++++++ internal/project/project.go | 42 ++- 4 files changed, 494 insertions(+), 7 deletions(-) create mode 100644 internal/project/config.go create mode 100644 internal/project/config_test.go diff --git a/cmd/attachmentupload/attachmentupload_test.go b/cmd/attachmentupload/attachmentupload_test.go index 5a2711e..cee3017 100644 --- a/cmd/attachmentupload/attachmentupload_test.go +++ b/cmd/attachmentupload/attachmentupload_test.go @@ -14,6 +14,17 @@ import ( "github.com/mozilla/markfluence/internal/project" ) +// writeMarker plants a valid markfluence.yaml -- a comment and nothing else, +// which is what ships and what export plants. +func writeMarker(t *testing.T, dir string) string { + t.Helper() + path := filepath.Join(dir, project.Filename) + if err := os.WriteFile(path, []byte("# Marks the root of a markfluence project.\n"), 0o644); err != nil { + t.Fatal(err) + } + return path +} + func writeFile(t *testing.T, dir, name string) string { t.Helper() path := filepath.Join(dir, name) @@ -132,7 +143,9 @@ func TestLocalAttachmentsRefusesABatchCollision(t *testing.T) { b := writeFile(t, root, "deploy/diagram.png") cache := project.NewCache("") if declareRoot { - writeFile(t, root, "markfluence.yaml") + // A real marker, not writeFile's placeholder bytes: the project + // file is parsed now, and a bare scalar in it is refused. + writeMarker(t, root) cache = project.NewCache(root) } diff --git a/internal/project/config.go b/internal/project/config.go new file mode 100644 index 0000000..e08eb6a --- /dev/null +++ b/internal/project/config.go @@ -0,0 +1,159 @@ +package project + +// The settings a markfluence.yaml declares, and why a file that cannot be +// understood stops the command rather than being treated as a bare marker. + +import ( + "errors" + "fmt" + "os" + "sort" + "strings" + + "github.com/mozilla/markfluence/internal/frontmatter" +) + +// Config is what a project file declares. Every field is project-*wide*, and +// every one is a default the file being published overrides: the chain is +// flag > frontmatter > project file, which is not the credentials chain +// (flag > environment > .env) and must not be conflated with it. A setting +// here answers "what is this content", not "who are you" (#100). +// +// Values are raw strings. A vocabulary is validated where it is consumed -- +// internal/pagewidth cannot be imported here, since it imports internal/client +// and internal/client holds a *Cache -- so this package checks that a file is +// structurally sound and says nothing about whether "huge" is a page width. +type Config struct { + // Space is the default space key, used by create when neither --space nor + // a frontmatter space: is given. + Space string + // PageWidth is the default page_width. Declaring it means update asserts + // a width on a file that declares none, the same way frontmatter does. + PageWidth string +} + +// kind is the shape a setting's value must have. Only scalars exist today. +type kind int + +const kindScalar kind = iota + 1 + +// settings is the whitelist of recognized top-level keys, mapped to the shape +// each one's value must have. +// +// A table of shapes rather than a set of names, because what a later setting +// needs checked is not "is this key known" but "is its value the right shape": +// #139's pages: is a mapping where both of today's settings are scalars, and it +// should be able to join this table instead of reworking the reader. +var settings = map[string]kind{ + "page_width": kindScalar, + "space": kindScalar, +} + +// dialect is markfluence's YAML dialect, worded for this file. The rules -- the +// scalar node-kind whitelist, the single-line rule, every null spelling reading +// as "" -- are internal/frontmatter's, deliberately: they were established by +// probing goccy, and a project file read by a second copy of them would be a +// second set of the same bugs (#100 called this "a third minimal parser" and +// declined it). +var dialect = frontmatter.Dialect{Doc: "a project file", Item: "setting"} + +// ConfigError is a markfluence.yaml that could not be understood. Typed so a +// caller can report it as itself rather than under the "resolving the +// documentation root" heading every other discovery failure earns -- the root +// was found; it is the file in it that is wrong. +type ConfigError struct { + // File is the project file's absolute path. + File string + // Line is a 1-based line within it, or 0 when the problem is the document + // as a whole rather than one setting. + Line int + Err error +} + +func (e *ConfigError) Error() string { + if e.Line > 0 { + return fmt.Sprintf("%s:%d: %s", e.File, e.Line, e.Err) + } + return fmt.Sprintf("%s: %s", e.File, e.Err) +} + +func (e *ConfigError) Unwrap() error { return e.Err } + +// loadConfig reads and validates a project file. +// +// An unrecognized key is fatal, and that is the point of validating at all +// rather than a cost of it (#100). A silently ignored `spce: ENG` is wrong for +// every file in the project at once, and a project file written for a newer +// markfluence holds keys this binary would ignore -- so publishing with the +// wrong space or the wrong width, silently and everywhere, is exactly what +// refusing prevents. The file therefore carries no schema version and this +// must not be loosened later; what should improve is the message, which says +// that an older binary is the likely cause. +// +// An empty file and a comment-only one are not errors: a comment and nothing +// else is what ships today, what the README documents, and what export plants. +func loadConfig(path string) (Config, error) { + data, err := os.ReadFile(path) + if err != nil { + // A file found and not readable is different from an ancestor that + // could not be stat'd (probeMarker treats that as "not here" and keeps + // walking). This one is a boundary that exists and cannot be + // established, which is the case that must not be guessed at. + return Config{}, &ConfigError{File: path, Err: errors.New(readFailure(err))} + } + items, err := dialect.ReadMapping(string(data)) + if err != nil { + return Config{}, &ConfigError{File: path, Err: err} + } + + cfg := Config{} + for _, it := range items { + want, ok := settings[it.Key] + if !ok { + return Config{}, &ConfigError{File: path, Line: it.Line, Err: unknownSetting(it.Key)} + } + if want == kindScalar && it.List != nil { + return Config{}, &ConfigError{File: path, Line: it.Line, Err: fmt.Errorf( + "setting %q must be a single value, not a list", it.Key)} + } + // A declared-but-empty setting is unset rather than an error, matching + // how frontmatter reads its own fields: `space:` with nothing after it + // says no more than an absent key does. + value := strings.TrimSpace(it.Value) + switch it.Key { + case "space": + cfg.Space = value + case "page_width": + cfg.PageWidth = value + } + } + return cfg, nil +} + +// unknownSetting is the message #100 exists for. One line, because it lands +// verbatim in check --json. +func unknownSetting(key string) error { + return fmt.Errorf( + "unknown setting %q (known: %s) -- an unrecognized setting may mean "+ + "this project needs a newer markfluence", key, strings.Join(knownSettings(), ", ")) +} + +// knownSettings lists the recognized keys, sorted so the message is stable. +func knownSettings() []string { + out := make([]string, 0, len(settings)) + for key := range settings { + out = append(out, key) + } + sort.Strings(out) + return out +} + +// readFailure strips the os.ReadFile message's own copy of the path, which +// ConfigError already supplies. +func readFailure(err error) string { + var pathErr *os.PathError + if errors.As(err, &pathErr) { + return pathErr.Err.Error() + } + return err.Error() +} diff --git a/internal/project/config_test.go b/internal/project/config_test.go new file mode 100644 index 0000000..b6f6275 --- /dev/null +++ b/internal/project/config_test.go @@ -0,0 +1,285 @@ +package project + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// write puts a project file in a fresh directory and returns the directory. +func write(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, Filename), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return dir +} + +func TestDiscoverReadsSettings(t *testing.T) { + dir := write(t, "space: ENG\npage_width: wide\n") + root, err := Discover(dir) + if err != nil { + t.Fatalf("Discover: %v", err) + } + defer root.FS.Close() + if root.Config.Space != "ENG" { + t.Errorf("Space = %q, want ENG", root.Config.Space) + } + if root.Config.PageWidth != "wide" { + t.Errorf("PageWidth = %q, want wide", root.Config.PageWidth) + } +} + +// The marker that ships today is a comment and nothing else, and export plants +// exactly that. Both it and an empty file must stay valid markers declaring no +// settings -- a project file gaining keys must not invalidate every existing one. +func TestDiscoverAcceptsAMarkerWithNoSettings(t *testing.T) { + for name, body := range map[string]string{ + "empty": "", + "comment only": "# Marks the root of a markfluence project. Image and link paths are recorded\n" + + "# relative to this directory. https://github.com/mozilla/markfluence\n", + } { + t.Run(name, func(t *testing.T) { + dir := write(t, body) + root, err := Discover(dir) + if err != nil { + t.Fatalf("Discover: %v", err) + } + defer root.FS.Close() + if root.Config != (Config{}) { + t.Errorf("Config = %#v, want zero", root.Config) + } + if root.Dir != dir { + t.Errorf("Dir = %q, want %q", root.Dir, dir) + } + }) + } +} + +func TestDiscoverRefusesAnUnknownSetting(t *testing.T) { + dir := write(t, "space: ENG\nspce: OPS\n") + _, err := Discover(dir) + if err == nil { + t.Fatal("Discover succeeded on an unknown setting, want an error") + } + msg := err.Error() + for _, want := range []string{ + filepath.Join(dir, Filename) + ":2", + `unknown setting "spce"`, + "known: page_width, space", + "newer markfluence", + } { + if !strings.Contains(msg, want) { + t.Errorf("error = %q, want it to contain %q", msg, want) + } + } + if strings.Contains(msg, "\n") { + t.Errorf("error spans lines:\n%s", msg) + } +} + +func TestDiscoverRefusesAMalformedFile(t *testing.T) { + tests := map[string]struct{ body, want string }{ + // A goccy parse error carries its own [line:col] and no noun of ours; + // ConfigError is what names the file. + "parse error": {"space: [ENG\n", "sequence end token"}, + "top-level list": {"- ENG\n", "must be a flat mapping of setting: value pairs"}, + "list where scalar": {"space: [ENG, OPS]\n", `setting "space" must be a single value, not a list`}, + "nested mapping": {"space:\n key: ENG\n", `setting "space" must be a single scalar value`}, + "duplicate key": {"space: ENG\nspace: OPS\n", "already defined"}, + "second document": {"space: ENG\n...\nspace: OPS\n", "must be a single document"}, + } + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + dir := write(t, tc.body) + _, err := Discover(dir) + if err == nil { + t.Fatalf("Discover succeeded on %q, want an error", tc.body) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to contain %q", err, tc.want) + } + var cfgErr *ConfigError + if !errors.As(err, &cfgErr) { + t.Errorf("error is %T, want it to unwrap to *ConfigError", err) + } + }) + } +} + +// The abort has to be immediate. Walking on to an ancestor that happens to +// have a good project file would publish under a root the author did not +// declare, which is the guess #100 exists to prevent. +func TestDiscoverDoesNotWalkPastAMalformedFile(t *testing.T) { + outer := write(t, "space: ENG\n") + inner := filepath.Join(outer, "docs") + if err := os.Mkdir(inner, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(inner, Filename), []byte("spce: OPS\n"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := Discover(inner); err == nil { + t.Fatal("Discover fell back to the outer project; a malformed file must stop the walk") + } +} + +// A malformed file is not a marker, so it must not be read as one and then +// have the *starting directory* used as the root either. +func TestDiscoverDoesNotFallBackToStartDirOnAMalformedFile(t *testing.T) { + dir := write(t, "spce: OPS\n") + root, err := Discover(dir) + if err == nil { + defer root.FS.Close() + t.Fatalf("Discover returned root %q, want an error", root.Dir) + } +} + +func TestCacheRefusesAMalformedFile(t *testing.T) { + dir := write(t, "spce: OPS\n") + c := NewCache("") + defer c.Close() + if _, err := c.Resolve(dir); err == nil { + t.Fatal("Cache.Resolve succeeded, want an error") + } +} + +// The cache exists so a batch pays for discovery once. A second file under the +// same root must not re-read the project file. +func TestCacheReadsTheProjectFileOnce(t *testing.T) { + dir := write(t, "space: ENG\n") + sub := filepath.Join(dir, "docs") + if err := os.Mkdir(sub, 0o755); err != nil { + t.Fatal(err) + } + c := NewCache("") + defer c.Close() + + first, err := c.Resolve(sub) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + // Make the file unreadable as YAML. A second Resolve that re-read it would + // now fail; one served from the cache cannot notice. + if err := os.WriteFile(filepath.Join(dir, Filename), []byte("- broken\n"), 0o644); err != nil { + t.Fatal(err) + } + second, err := c.Resolve(filepath.Join(dir, "other")) + if err != nil { + t.Fatalf("second Resolve re-read the project file: %v", err) + } + if second != first { + t.Error("second Resolve built a new Root, want the cached one") + } + if second.Config.Space != "ENG" { + t.Errorf("Space = %q, want ENG", second.Config.Space) + } +} + +// --root overrides discovery, not the file's contents: it would be strange for +// the flag that declares the root to discard the root's own settings. +func TestFromPathReadsSettings(t *testing.T) { + dir := write(t, "space: ENG\n") + root, err := FromPath(dir) + if err != nil { + t.Fatalf("FromPath: %v", err) + } + defer root.FS.Close() + if root.Config.Space != "ENG" { + t.Errorf("Space = %q, want ENG", root.Config.Space) + } +} + +func TestFromPathRefusesAMalformedFile(t *testing.T) { + dir := write(t, "spce: OPS\n") + if _, err := FromPath(dir); err == nil { + t.Fatal("FromPath succeeded, want an error") + } +} + +// --root at a directory with no project file has no settings and is not an +// error: that is the whole point of the flag for a tree that will never have one. +func TestFromPathWithNoProjectFile(t *testing.T) { + dir := t.TempDir() + root, err := FromPath(dir) + if err != nil { + t.Fatalf("FromPath: %v", err) + } + defer root.FS.Close() + if root.File != "" || root.Config != (Config{}) { + t.Errorf("File = %q, Config = %#v, want empty", root.File, root.Config) + } +} + +// An ancestor that cannot be stat'd is "not here" and the walk continues +// (probeMarker). A project file that was found and cannot be read is different: +// it is a boundary that exists and cannot be established. +func TestDiscoverRefusesAnUnreadableProjectFile(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can read a 0000 file") + } + dir := write(t, "space: ENG\n") + path := filepath.Join(dir, Filename) + if err := os.Chmod(path, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(path, 0o644) }) + + _, err := Discover(dir) + if err == nil { + t.Fatal("Discover succeeded on an unreadable project file, want an error") + } + if strings.Contains(err.Error(), path+": "+path) { + t.Errorf("error names the path twice: %s", err) + } + if !strings.Contains(err.Error(), path) { + t.Errorf("error = %q, want it to name %q", err, path) + } +} + +// A declared-but-empty setting says no more than an absent key, and must not +// be an error -- someone typing a key and stopping is a normal half-edit. +func TestLoadConfigTreatsAnEmptySettingAsUnset(t *testing.T) { + dir := write(t, "space:\npage_width: ~\n") + root, err := Discover(dir) + if err != nil { + t.Fatalf("Discover: %v", err) + } + defer root.FS.Close() + if root.Config != (Config{}) { + t.Errorf("Config = %#v, want zero", root.Config) + } +} + +// Settings are per-root, so one invocation spanning two projects gets each +// project's own defaults with no special case. +func TestSettingsArePerRoot(t *testing.T) { + base := t.TempDir() + one := filepath.Join(base, "one") + two := filepath.Join(base, "two") + for dir, body := range map[string]string{one: "space: ENG\n", two: "space: OPS\n"} { + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, Filename), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + c := NewCache("") + defer c.Close() + + got := map[string]string{} + for _, dir := range []string{one, two} { + root, err := c.Resolve(dir) + if err != nil { + t.Fatalf("Resolve(%s): %v", dir, err) + } + got[filepath.Base(dir)] = root.Config.Space + } + if got["one"] != "ENG" || got["two"] != "OPS" { + t.Errorf("spaces = %#v, want one=ENG two=OPS", got) + } +} diff --git a/internal/project/project.go b/internal/project/project.go index 6e291d9..279fe43 100644 --- a/internal/project/project.go +++ b/internal/project/project.go @@ -1,10 +1,18 @@ // Package project discovers the root of a markfluence project: the directory // holding markfluence.yaml, found by walking up from a starting directory. // -// The marker file's existence is its whole meaning -- nothing in it is parsed -// or executed, and discovery decides only where the root is, never authorizes -// anything the file might someday declare. See _plans/026's security review -// for why that separation is deliberate. +// The marker file declares project-wide settings (Config, #100), and a file +// that cannot be understood is not a valid marker: discovery fails rather than +// walking on to an ancestor or falling back to the starting directory. +// +// What the file still never does is authorize anything. It is read, never +// executed, and discovery decides only where the root is -- it does not grant +// a project the ability to redirect credentials, which is why Config holds no +// url or token and why a setting there answers "what is this content" rather +// than "who are you". See _plans/026's security review for why that separation +// is deliberate, and _plans/038 for the sharper form of it: basic auth goes to +// whatever host the resolved URL names, so a committed, walked-up file naming +// one would decide where the token is sent. // // Discover is called from two different starting points for two different // reasons, which is why this package returns a Root rather than a bare string: @@ -35,6 +43,9 @@ type Root struct { // File is the absolute path to markfluence.yaml, or "" when none was // found and Dir fell back to the starting directory. File string + // Config is what that file declares, zero-valued when there is no file + // (or when the file declares nothing, which is the shape that ships). + Config Config // FS scopes every read to Dir: a path cannot escape it, even via a // symlink partway down its traversal, which a lexical containment check // cannot see but os.Root refuses outright. Callers close it when done. @@ -104,13 +115,32 @@ func probeMarker(dir string) (hit bool, file string, err error) { } } -// open builds a Root for dir, opening an os.Root scoped to it. +// open builds a Root for dir, opening an os.Root scoped to it and loading the +// project file's settings when there is one. +// +// The load happens here because this is the single place a Root is built from a +// marker: Discover, Cache.walkAndCache and FromPath all reach it, so the rule +// that a project file which cannot be understood is not a valid marker exists +// in one copy and the three cannot come to disagree about it. Nothing keeps +// walking upward for a better marker and nothing falls back to the starting +// directory: the root decides every attachment name, bounds every read, and +// anchors the link index, so a file that cannot be understood means the +// project's boundary is unknown, and guessing is worse than stopping (#100). +// +// Loading precedes OpenRoot so a refusal leaks no handle. func open(dir, file string) (*Root, error) { + cfg := Config{} + if file != "" { + var err error + if cfg, err = loadConfig(file); err != nil { + return nil, err + } + } root, err := os.OpenRoot(dir) if err != nil { return nil, err } - return &Root{Dir: dir, File: file, FS: root}, nil + return &Root{Dir: dir, File: file, Config: cfg, FS: root}, nil } // FromPath builds a Root directly from an explicit directory, bypassing From 2c715f9de8a79d2d467f075f1a359198a63c101a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:00:52 -0400 Subject: [PATCH 04/17] fix(client): stop swallowing a root-discovery failure when locating .env loadEnvFile discovered the project root inside `if err == nil` and degraded to the working directory on any failure. That was harmless while the project file was never read. Now that it is parsed (#100), a file that cannot be understood makes discovery fail -- and degrading there would read a different .env than the project's, silently, and would leave a command with no per-file root of its own (read, search, info) never reporting the malformed file at all. #100 settles that as: abort immediately. --env-file still overrides discovery absolutely, which is how someone works around a project file they cannot fix; a test pins it. A missing .env, wherever it lands, is still fine. --- internal/client/config.go | 38 +++++++++++++++++++++----- internal/client/config_test.go | 49 ++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/internal/client/config.go b/internal/client/config.go index fc7abc7..76638c2 100644 --- a/internal/client/config.go +++ b/internal/client/config.go @@ -133,6 +133,13 @@ func resolveValue(flagVal, envKey string, dotenv map[string]string) string { // runs once before any file is touched, and doesn't bound anything -- it // only answers "where is .env." A missing .env, wherever it lands, is fine // and yields an empty map, matching prior behavior. +// +// A discovery *failure* is not fine, and used to be swallowed. Since the +// project file is parsed (#100), one that cannot be understood makes discovery +// fail -- and degrading to the working directory there would read a different +// .env than the project's, silently, and would leave a command with no +// per-file root of its own (read, search, info) never reporting the malformed +// file at all. #100 settles that as: abort immediately. func loadEnvFile(envFile string, roots *project.Cache) (map[string]string, error) { if envFile != "" { env, err := loadDotenv(envFile) @@ -144,14 +151,11 @@ func loadEnvFile(envFile string, roots *project.Cache) (map[string]string, error dir := "." if cwd, err := os.Getwd(); err == nil { - if roots != nil { - if root, err := roots.Resolve(cwd); err == nil { - dir = root.Dir - } - } else if root, err := project.Discover(cwd); err == nil { - dir = root.Dir - _ = root.FS.Close() + found, err := dotenvDir(cwd, roots) + if err != nil { + return nil, err } + dir = found } env, err := loadDotenv(filepath.Join(dir, dotenvPath)) @@ -161,6 +165,26 @@ func loadEnvFile(envFile string, roots *project.Cache) (map[string]string, error return env, nil } +// dotenvDir reports the directory .env is read from: the caller's own cache +// when it has one, so a --root override and the walk it already paid for both +// apply here too, and a fresh walk otherwise. The cache owns closing its +// handle; the fresh walk's is ours. +func dotenvDir(cwd string, roots *project.Cache) (string, error) { + if roots != nil { + root, err := roots.Resolve(cwd) + if err != nil { + return "", err + } + return root.Dir, nil + } + root, err := project.Discover(cwd) + if err != nil { + return "", err + } + defer root.FS.Close() + return root.Dir, nil +} + // securityWarner receives a credential-hygiene warning. Package-level and set // once from the command layer for the same reason SetRetryLogger is // (retrylog.go): twelve commands build a client through Resolve with an diff --git a/internal/client/config_test.go b/internal/client/config_test.go index 755160b..e2e4951 100644 --- a/internal/client/config_test.go +++ b/internal/client/config_test.go @@ -419,3 +419,52 @@ func TestResolveWarnsThroughTheDiscoveredEnvFile(t *testing.T) { t.Errorf("warnings = %v, want exactly one", *got) } } + +// A malformed markfluence.yaml used to be swallowed here, silently reading +// .env from the working directory instead of the project root. It matters most +// for a command with no per-file root of its own -- read, search, info -- which +// would otherwise never report the malformed file at all. +func TestResolveFailsOnAMalformedProjectFile(t *testing.T) { + clearConfluenceEnv(t) + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "markfluence.yaml"), + []byte("spce: ENG\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, ".env"), + []byte("CONFLUENCE_URL=https://wiki\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(root) + + if _, err := Resolve(ResolveOptions{}); err == nil { + t.Fatal("Resolve succeeded with a malformed project file, want an error") + } else if !strings.Contains(err.Error(), "unknown setting") { + t.Errorf("error = %q, want it to name the unknown setting", err) + } +} + +// --env-file overrides discovery absolutely, which has to keep holding: an +// explicit path is how someone works around a project file they cannot fix. +func TestResolveEnvFileOverridesAMalformedProjectFile(t *testing.T) { + clearConfluenceEnv(t) + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "markfluence.yaml"), + []byte("spce: ENG\n"), 0o644); err != nil { + t.Fatal(err) + } + explicit := filepath.Join(root, "creds.env") + if err := os.WriteFile(explicit, + []byte("CONFLUENCE_URL=https://wiki\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Chdir(root) + + c, err := Resolve(ResolveOptions{EnvFile: explicit}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if c.SiteURL() != "https://wiki" { + t.Errorf("SiteURL = %q, want https://wiki", c.SiteURL()) + } +} From 774d0899de3a1722009bb52aef837b4b3a0d04d5 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:02:48 -0400 Subject: [PATCH 05/17] feat(create): default space and page_width from markfluence.yaml #100's use case: a hundred files in one space, each repeating `space: ENG`. A project file declaring it once removes the duplication, and the chain is flag > frontmatter > project file -- the answer closer to the content wins. The project default is consulted only when both levels above it are silent, which is what keeps it from needing any new disagreement rule. --space and a frontmatter space both set and differing stays an error, unchanged: those are two answers about where a page goes with no reason to prefer one. A project default cannot be a third answer of that kind. The space chain moves into resolveSpace, beside resolveTitle and resolveWidth, because it was six inline lines with no way to test the precedence directly. A blank frontmatter page_width now falls through to the project default instead of short-circuiting to max, matching how every other field reads a blank value as unset. resolveWidth names the project file when the bad value came from there. internal/project cannot validate a width itself -- it would have to import internal/pagewidth, which imports internal/client, which holds a *project.Cache -- so this is the first place a project-wide one is checked, and "invalid page_width" pointing at a markdown file that never mentions one is the wrong file to send someone to. --- cmd/create/create.go | 76 +++++++++++++++++----- cmd/create/create_test.go | 128 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 184 insertions(+), 20 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 8482ec5..6b9efc0 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -693,7 +693,7 @@ func resolveFile( if title == "" { return record{}, errors.New("no title given (pass --title or add a 'title:' frontmatter field)") } - width, err := resolveWidth(pageWidthOpt, mf.Frontmatter) + width, err := resolveWidth(pageWidthOpt, mf.Frontmatter, root) if err != nil { return record{}, err } @@ -719,17 +719,9 @@ func resolveFile( return record{}, err } - // Space: --space or frontmatter 'space'; both set and differing is an error. - fmSpace := mf.Frontmatter["space"] - if spaceOpt != "" && fmSpace != "" && spaceOpt != fmSpace { - return record{}, fmt.Errorf("--space %q conflicts with frontmatter space %q", spaceOpt, fmSpace) - } - spaceKey := spaceOpt - if spaceKey == "" { - spaceKey = fmSpace - } - if spaceKey == "" { - return record{}, errors.New("no space given (pass --space or add a 'space:' frontmatter field)") + spaceKey, err := resolveSpace(spaceOpt, mf.Frontmatter, root) + if err != nil { + return record{}, err } spaceID, ok := spaceCache[spaceKey] if !ok { @@ -1002,11 +994,63 @@ func resolveTitle(cliTitle string, mf *frontmatter.MarkdownFile) string { return mf.Title() } -// resolveWidth returns the effective page width: --page-width overrides the -// frontmatter page_width, which defaults to max when unset. -func resolveWidth(cliPageWidth string, fm map[string]string) (pagewidth.Width, error) { +// resolveSpace returns the space key to publish into: --space, then the +// frontmatter space, then the project file's default (#100's chain, flag > +// frontmatter > project file). +// +// --space and a frontmatter space both set and differing stays an error, as it +// has been: those are two answers about where a page goes, with no reason to +// prefer one. The project default is not a third answer of that kind -- it is +// read only when neither of the two above it said anything, so it can never +// conflict with either, which is what keeps the chain from needing any new +// disagreement rule. +func resolveSpace(cliSpace string, fm map[string]string, root *project.Root) (string, error) { + fmSpace := fm["space"] + if cliSpace != "" && fmSpace != "" && cliSpace != fmSpace { + return "", fmt.Errorf("--space %q conflicts with frontmatter space %q", cliSpace, fmSpace) + } + if cliSpace != "" { + return cliSpace, nil + } + if fmSpace != "" { + return fmSpace, nil + } + if root != nil && root.Config.Space != "" { + return root.Config.Space, nil + } + return "", fmt.Errorf("no space given (pass --space, add a 'space:' frontmatter "+ + "field, or set 'space:' in %s)", project.Filename) +} + +// resolveWidth returns the effective page width: --page-width, then the +// frontmatter page_width, then the project file's default, then max. That is +// #100's chain -- flag > frontmatter > project file -- and the project file is +// only ever consulted when the two above it are silent, so it never +// participates in a conflict. +// +// A blank frontmatter page_width falls through to the project default rather +// than defaulting to max, matching how every other field reads a blank value +// as unset. +// +// The error names the project file when the bad value came from there. +// internal/project cannot check this itself -- it would have to import +// internal/pagewidth, which imports internal/client, which holds a +// *project.Cache -- so this is where a project-wide width is first validated, +// and "invalid page_width" pointing at a markdown file that never mentions one +// is the wrong file to send someone to. +func resolveWidth(cliPageWidth string, fm map[string]string, root *project.Root) (pagewidth.Width, error) { if cliPageWidth != "" { return pagewidth.Declared(map[string]string{"page_width": cliPageWidth}) } - return pagewidth.Declared(fm) + if strings.TrimSpace(fm["page_width"]) != "" { + return pagewidth.Declared(fm) + } + if root != nil && root.Config.PageWidth != "" { + w, err := pagewidth.Declared(map[string]string{"page_width": root.Config.PageWidth}) + if err != nil { + return "", fmt.Errorf("%s: %w", root.File, err) + } + return w, nil + } + return pagewidth.DefaultWidth, nil } diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index 7148c32..cb0f61a 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -113,28 +113,148 @@ func TestResolveTitle(t *testing.T) { } } +func TestResolveSpace(t *testing.T) { + withFM := map[string]string{"space": "FM"} + none := map[string]string{} + declared := &project.Root{File: "/repo/markfluence.yaml", Config: project.Config{Space: "PROJ"}} + bare := &project.Root{File: "/repo/markfluence.yaml"} + + t.Run("flag", func(t *testing.T) { + if got, err := resolveSpace("CLI", none, bare); err != nil || got != "CLI" { + t.Fatalf("= %q/%v, want CLI/nil", got, err) + } + }) + t.Run("frontmatter", func(t *testing.T) { + if got, err := resolveSpace("", withFM, bare); err != nil || got != "FM" { + t.Fatalf("= %q/%v, want FM/nil", got, err) + } + }) + t.Run("project file when the two above are silent", func(t *testing.T) { + if got, err := resolveSpace("", none, declared); err != nil || got != "PROJ" { + t.Fatalf("= %q/%v, want PROJ/nil", got, err) + } + }) + t.Run("flag beats the project file", func(t *testing.T) { + if got, err := resolveSpace("CLI", none, declared); err != nil || got != "CLI" { + t.Fatalf("= %q/%v, want CLI/nil", got, err) + } + }) + t.Run("frontmatter beats the project file", func(t *testing.T) { + if got, err := resolveSpace("", withFM, declared); err != nil || got != "FM" { + t.Fatalf("= %q/%v, want FM/nil", got, err) + } + }) + // The project default is consulted only when both levels above it are + // silent, so it never becomes a third party to this disagreement. + t.Run("flag conflicting with frontmatter is still an error", func(t *testing.T) { + _, err := resolveSpace("CLI", withFM, declared) + if err == nil { + t.Fatal("want an error when --space and frontmatter disagree") + } + if !strings.Contains(err.Error(), "conflicts with frontmatter") { + t.Errorf("error = %q, want the conflict wording", err) + } + }) + // Agreement is not a conflict, which the old inline form also held. + t.Run("flag agreeing with frontmatter is fine", func(t *testing.T) { + if got, err := resolveSpace("FM", withFM, declared); err != nil || got != "FM" { + t.Fatalf("= %q/%v, want FM/nil", got, err) + } + }) + t.Run("nothing anywhere names all three places", func(t *testing.T) { + _, err := resolveSpace("", none, bare) + if err == nil { + t.Fatal("want an error when no space is given") + } + for _, want := range []string{"--space", "frontmatter", "markfluence.yaml"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to mention %q", err, want) + } + } + }) + t.Run("nil root is not a panic", func(t *testing.T) { + if _, err := resolveSpace("", none, nil); err == nil { + t.Fatal("want an error when no space is given") + } + }) +} + func TestResolveWidth(t *testing.T) { withFM := map[string]string{"page_width": "wide"} + none := map[string]string{} + // A project file declaring a width, and one declaring nothing. + declared := &project.Root{File: "/repo/markfluence.yaml", Config: project.Config{PageWidth: "narrow"}} + bare := &project.Root{File: "/repo/markfluence.yaml"} + t.Run("flag overrides frontmatter", func(t *testing.T) { - if w, err := resolveWidth("narrow", withFM); err != nil || w != pagewidth.Narrow { + if w, err := resolveWidth("narrow", withFM, bare); err != nil || w != pagewidth.Narrow { t.Fatalf("= %q/%v, want narrow/nil", w, err) } }) t.Run("frontmatter when no flag", func(t *testing.T) { - if w, err := resolveWidth("", withFM); err != nil || w != pagewidth.Wide { + if w, err := resolveWidth("", withFM, bare); err != nil || w != pagewidth.Wide { t.Fatalf("= %q/%v, want wide/nil", w, err) } }) t.Run("defaults to max when unset", func(t *testing.T) { - if w, err := resolveWidth("", map[string]string{}); err != nil || w != pagewidth.Max { + if w, err := resolveWidth("", none, bare); err != nil || w != pagewidth.Max { t.Fatalf("= %q/%v, want max/nil", w, err) } }) t.Run("invalid flag errors", func(t *testing.T) { - if _, err := resolveWidth("huge", map[string]string{}); err == nil { + if _, err := resolveWidth("huge", none, bare); err == nil { t.Fatal("want error for invalid --page-width") } }) + + t.Run("project file when neither flag nor frontmatter", func(t *testing.T) { + if w, err := resolveWidth("", none, declared); err != nil || w != pagewidth.Narrow { + t.Fatalf("= %q/%v, want narrow/nil", w, err) + } + }) + t.Run("flag beats the project file", func(t *testing.T) { + if w, err := resolveWidth("wide", none, declared); err != nil || w != pagewidth.Wide { + t.Fatalf("= %q/%v, want wide/nil", w, err) + } + }) + t.Run("frontmatter beats the project file", func(t *testing.T) { + if w, err := resolveWidth("", withFM, declared); err != nil || w != pagewidth.Wide { + t.Fatalf("= %q/%v, want wide/nil", w, err) + } + }) + // A blank value is unset everywhere else in markfluence, so it must fall + // through rather than short-circuiting to max and hiding the project default. + t.Run("blank frontmatter falls through to the project file", func(t *testing.T) { + blank := map[string]string{"page_width": " "} + if w, err := resolveWidth("", blank, declared); err != nil || w != pagewidth.Narrow { + t.Fatalf("= %q/%v, want narrow/nil", w, err) + } + }) + // internal/project cannot validate its own width, so this is the first + // place a bad one is caught -- and the message has to send the reader to + // the file that actually holds it, not to a markdown file with no + // page_width in it at all. + t.Run("invalid project width names the project file", func(t *testing.T) { + bad := &project.Root{File: "/repo/markfluence.yaml", Config: project.Config{PageWidth: "huge"}} + _, err := resolveWidth("", none, bad) + if err == nil { + t.Fatal("want an error for an invalid project page_width") + } + if !strings.Contains(err.Error(), "/repo/markfluence.yaml") { + t.Errorf("error = %q, want it to name the project file", err) + } + if !strings.Contains(err.Error(), `invalid page_width "huge"`) { + t.Errorf("error = %q, want it to name the bad value", err) + } + }) + // A declared width is a project-wide default, not a reason to fail a file + // that overrides it: an unparseable one must still be caught above, but a + // good one must not become a per-file requirement. + t.Run("nil root is not a panic", func(t *testing.T) { + if w, err := resolveWidth("", none, nil); err != nil || w != pagewidth.Max { + t.Fatalf("= %q/%v, want max/nil", w, err) + } + }) } func TestWantPersist(t *testing.T) { From bf145e7feacd3c294bd255ae881304651cbfd545 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:04:45 -0400 Subject: [PATCH 06/17] feat(update): default page_width from markfluence.yaml Completes the width chain: --page-width, then the frontmatter page_width, then the project file's default. This is the one level that changes update's behavior. A project declaring page_width makes update assert a width on a file that declares none, where before the live width was left alone. That is deliberate -- it is what "declared means asserted" (L9) means one level up -- and a project that wants the live width untouched omits the key, which keeps "absent means no width request at all" intact. The root is now resolved above resolveWidth rather than beside the link index, because every local check has to stay ahead of the first request and the width chain now ends at a file on disk. The walk is cached, so asking early costs nothing. Building the *index* did not move, so a file the mtime check skips still never pays for one. A malformed project file is reported as VALIDATION rather than IO, via project.IsConfigError, and project.RootError leaves it without the "resolving the documentation root" heading: the root was found, and it is the file in it that is wrong. Reporting a file the author can open and fix as I/O sends the reader looking for a disk fault. --- cmd/update/update.go | 53 ++++++++++++++++++----- cmd/update/update_test.go | 87 +++++++++++++++++++++++++++++++++++--- internal/project/config.go | 22 ++++++++++ 3 files changed, 146 insertions(+), 16 deletions(-) diff --git a/cmd/update/update.go b/cmd/update/update.go index 5ccd893..880cfff 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -195,7 +195,20 @@ func processFile( if !pageref.IsDigits(pageID) { return r.fail(errors.New(pageref.NotNumericMessage(pageID)), jsonout.CodeValidation) } - width, applyWidth, err := resolveWidth(pageWidthFlag, mf) + // The root is resolved here rather than beside the link index below, + // because the width chain now ends at the project file and every local + // check has to stay ahead of the first request. The walk is cached, so + // asking early costs nothing; building the *index* is not moved, so a file + // the mtime check skips still never pays for one. + abs, err := filepath.Abs(filename) + if err != nil { + return r.fail(err, jsonout.CodeIO) + } + root, err := roots.Resolve(filepath.Dir(abs)) + if err != nil { + return r.fail(project.RootError(err), rootErrorCode(err)) + } + width, applyWidth, err := resolveWidth(pageWidthFlag, mf, root) if err != nil { return r.fail(err, jsonout.CodeValidation) } @@ -240,14 +253,6 @@ func processFile( } } - abs, err := filepath.Abs(filename) - if err != nil { - return r.fail(err, jsonout.CodeIO) - } - root, err := roots.Resolve(filepath.Dir(abs)) - if err != nil { - return r.fail(fmt.Errorf("resolving the documentation root: %w", err), jsonout.CodeIO) - } index, err := indexes.Get(root) if err != nil { return r.fail(fmt.Errorf("building the link index: %w", err), jsonout.CodeIO) @@ -383,10 +388,13 @@ func resolveTitlePageID(cliTitle, cliPageID string, mf *frontmatter.MarkdownFile return title, titlePresent, pageID } -// resolveWidth resolves the page width to assert. It returns apply=false when +// resolveWidth resolves the page width to assert: --page-width, then the +// frontmatter page_width, then the project file's default (#100's chain, flag > +// frontmatter > project file). It returns apply=false when // neither --page-width nor a frontmatter page_width is set, meaning the live // page's width should be left untouched. -func resolveWidth(cliPageWidth string, mf *frontmatter.MarkdownFile) (pagewidth.Width, bool, error) { +func resolveWidth(cliPageWidth string, mf *frontmatter.MarkdownFile, + root *project.Root) (pagewidth.Width, bool, error) { if cliPageWidth != "" { w, err := pagewidth.Declared(map[string]string{"page_width": cliPageWidth}) return w, err == nil, err @@ -395,9 +403,32 @@ func resolveWidth(cliPageWidth string, mf *frontmatter.MarkdownFile) (pagewidth. w, err := pagewidth.Declared(mf.Frontmatter) return w, err == nil, err } + // The project file's default, and the one level of the chain that changes + // update's behavior: declaring page_width there makes update assert a width + // on a file that declares none, where before it left the live width alone. + // That is deliberate -- it is what "declared means asserted" (L9) means one + // level up -- and a project that wants the live width untouched omits the + // key. Absent still means no width request at all. + if root != nil && root.Config.PageWidth != "" { + w, err := pagewidth.Declared(map[string]string{"page_width": root.Config.PageWidth}) + if err != nil { + return "", false, fmt.Errorf("%s: %w", root.File, err) + } + return w, true, nil + } return "", false, nil } +// rootErrorCode classifies a root-resolution failure. A malformed project file +// is a local defect in a file the author can open and fix, so reporting it as +// I/O would send the reader looking for a disk fault. +func rootErrorCode(err error) jsonout.Code { + if project.IsConfigError(err) { + return jsonout.CodeValidation + } + return jsonout.CodeIO +} + // applyLabels asserts the declared label set, recording the per-label actions. // // A file that declares no labels key makes no request at all -- not merely no diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index e5011b8..ea0d188 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -76,31 +76,108 @@ func TestResolveWidth(t *testing.T) { t.Fatal(err) } + // A project file declaring a width, and one declaring nothing. + declared := &project.Root{File: "/repo/markfluence.yaml", Config: project.Config{PageWidth: "narrow"}} + bare := &project.Root{File: "/repo/markfluence.yaml"} + t.Run("flag overrides frontmatter", func(t *testing.T) { - w, apply, err := resolveWidth("narrow", withFM) + w, apply, err := resolveWidth("narrow", withFM, bare) if err != nil || !apply || w != pagewidth.Narrow { t.Fatalf("= %q/%v/%v, want narrow/true/nil", w, apply, err) } }) t.Run("frontmatter when no flag", func(t *testing.T) { - w, apply, err := resolveWidth("", withFM) + w, apply, err := resolveWidth("", withFM, bare) if err != nil || !apply || w != pagewidth.Wide { t.Fatalf("= %q/%v/%v, want wide/true/nil", w, apply, err) } }) t.Run("no flag and no frontmatter width -> skip", func(t *testing.T) { - if _, apply, err := resolveWidth("", noWidth); err != nil || apply { + if _, apply, err := resolveWidth("", noWidth, bare); err != nil || apply { t.Fatalf("= apply %v err %v, want false/nil", apply, err) } - if _, apply, err := resolveWidth("", noFM); err != nil || apply { + if _, apply, err := resolveWidth("", noFM, bare); err != nil || apply { t.Fatalf("(no frontmatter) = apply %v err %v, want false/nil", apply, err) } }) t.Run("invalid flag errors", func(t *testing.T) { - if _, apply, err := resolveWidth("huge", noFM); err == nil || apply { + if _, apply, err := resolveWidth("huge", noFM, bare); err == nil || apply { t.Fatalf("= apply %v err %v, want false/error", apply, err) } }) + + // The behavior change: a project-wide page_width makes update assert a + // width on a file that declares none. Before, that file's live width was + // left alone. It is what "declared means asserted" (L9) means one level up. + t.Run("project file makes update assert a width", func(t *testing.T) { + w, apply, err := resolveWidth("", noWidth, declared) + if err != nil || !apply || w != pagewidth.Narrow { + t.Fatalf("= %q/%v/%v, want narrow/true/nil", w, apply, err) + } + }) + t.Run("flag beats the project file", func(t *testing.T) { + w, apply, err := resolveWidth("wide", noWidth, declared) + if err != nil || !apply || w != pagewidth.Wide { + t.Fatalf("= %q/%v/%v, want wide/true/nil", w, apply, err) + } + }) + t.Run("frontmatter beats the project file", func(t *testing.T) { + w, apply, err := resolveWidth("", withFM, declared) + if err != nil || !apply || w != pagewidth.Wide { + t.Fatalf("= %q/%v/%v, want wide/true/nil", w, apply, err) + } + }) + // The escape hatch has to keep working: a project that omits the key gets + // no width request at all, which is the pre-#100 behavior. + t.Run("no project width means no width request", func(t *testing.T) { + if _, apply, err := resolveWidth("", noWidth, declaredNothing()); err != nil || apply { + t.Fatalf("= apply %v err %v, want false/nil", apply, err) + } + }) + t.Run("invalid project width names the project file", func(t *testing.T) { + bad := &project.Root{File: "/repo/markfluence.yaml", Config: project.Config{PageWidth: "huge"}} + _, apply, err := resolveWidth("", noWidth, bad) + if err == nil || apply { + t.Fatalf("= apply %v err %v, want false/error", apply, err) + } + if !strings.Contains(err.Error(), "/repo/markfluence.yaml") { + t.Errorf("error = %q, want it to name the project file", err) + } + }) + t.Run("nil root is not a panic", func(t *testing.T) { + if _, apply, err := resolveWidth("", noWidth, nil); err != nil || apply { + t.Fatalf("= apply %v err %v, want false/nil", apply, err) + } + }) +} + +// declaredNothing is a project file with no settings -- the marker that ships. +func declaredNothing() *project.Root { + return &project.Root{File: "/repo/markfluence.yaml"} +} + +func TestRootErrorCode(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, project.Filename) + if err := os.WriteFile(path, []byte("spce: ENG\n"), 0o644); err != nil { + t.Fatal(err) + } + _, err := project.Discover(dir) + if err == nil { + t.Fatal("want an error") + } + if got := rootErrorCode(err); got != jsonout.CodeValidation { + t.Errorf("code = %q, want VALIDATION: a malformed project file is a local defect, not I/O", got) + } + // Anything else really is a failure to resolve the root. + if got := rootErrorCode(os.ErrPermission); got != jsonout.CodeIO { + t.Errorf("code = %q, want IO", got) + } + // RootError leaves a ConfigError as itself -- the root was found, and it is + // the file in it that is wrong. + if msg := project.RootError(err).Error(); strings.Contains(msg, "resolving the documentation root") { + t.Errorf("error = %q, want no root-resolution heading", msg) + } } func TestOverrideNeedsSingleFile(t *testing.T) { diff --git a/internal/project/config.go b/internal/project/config.go index e08eb6a..5081d3d 100644 --- a/internal/project/config.go +++ b/internal/project/config.go @@ -157,3 +157,25 @@ func readFailure(err error) string { } return err.Error() } + +// IsConfigError reports whether err is a markfluence.yaml that could not be +// understood, as opposed to any other way resolving a root can fail. +// +// It exists because the two deserve different reporting: a malformed project +// file is a local defect in a file the author can open and fix, where a failed +// walk or a refused os.OpenRoot is an I/O problem. A command reporting the +// first as I/O sends the reader looking for a disk fault. +func IsConfigError(err error) bool { + var cfgErr *ConfigError + return errors.As(err, &cfgErr) +} + +// RootError frames a root-resolution failure for a reader. A malformed project +// file is reported as itself -- the root was found, and it is the file in it +// that is wrong -- where anything else really is a failure to resolve the root. +func RootError(err error) error { + if IsConfigError(err) { + return err + } + return fmt.Errorf("resolving the documentation root: %w", err) +} From 05864c809dc65ef884c352767d1c6a4181eab40d Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:06:34 -0400 Subject: [PATCH 07/17] feat(check): validate markfluence.yaml offline check is the one verb that can find a project-wide page_width Confluence does not accept without publishing. internal/project cannot validate its own value -- it would have to import internal/pagewidth, which imports internal/client, which holds a *project.Cache -- so the loader checks structure and this checks the vocabulary. Reported as Broken rather than a warning, because it fails every publish under that root, which is the severity an invalid frontmatter page_width already gets. And reported on each file under the root rather than once for the run, which is what keeps every diagnostic scoped to the files actually named: checking a file under a different project says nothing about this one. frontmatterBroken becomes localBroken -- it now holds defects found without the converter generally, not only frontmatter ones. A malformed markfluence.yaml fails a file as VALIDATION rather than IO, and without the "resolving the documentation root" heading, here and in create: the root was found, and it is the file in it that is wrong. A file the author can open and fix reported as I/O sends the reader looking for a disk fault. --- cmd/check/check.go | 42 ++++++++++++++--- cmd/check/check_test.go | 102 ++++++++++++++++++++++++++++++++++++++++ cmd/create/create.go | 4 +- 3 files changed, 140 insertions(+), 8 deletions(-) diff --git a/cmd/check/check.go b/cmd/check/check.go index 6fc29e7..726ce6f 100644 --- a/cmd/check/check.go +++ b/cmd/check/check.go @@ -148,15 +148,24 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache } root, err := roots.Resolve(filepath.Dir(abs)) if err != nil { - return r.fail(fmt.Errorf("resolving the documentation root: %w", err), jsonout.CodeIO) + code := jsonout.CodeIO + if project.IsConfigError(err) { + // A markfluence.yaml that cannot be understood is a local defect in + // a file the author can open and fix, which is check's whole + // subject -- reporting it as I/O would send the reader looking for + // a disk fault. + code = jsonout.CodeValidation + } + return r.fail(project.RootError(err), code) } index, err := indexes.Get(root) if err != nil { 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 + // Collected before the conversion, which can bail out: a defect found + // without the converter -- in the frontmatter or in the project file -- 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 @@ -165,9 +174,28 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache // 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 + var localBroken []string + // A project-wide page_width Confluence does not accept fails every publish + // under this root, so it is Broken rather than a warning -- the same + // severity an invalid frontmatter page_width gets, for the same reason. + // + // This is where a project-wide width is validated offline at all: + // internal/project cannot check its own value, since it would have to + // import internal/pagewidth, which imports internal/client, which holds a + // *project.Cache. check is the one verb that can find it without + // publishing. + // + // Reported on each file under that root rather than once for the run, which + // is what keeps every diagnostic scoped to the files actually named: a file + // under a different project hears nothing about this one. + if root.Config.PageWidth != "" { + if _, err := pagewidth.Declared( + map[string]string{"page_width": root.Config.PageWidth}); err != nil { + localBroken = append(localBroken, fmt.Sprintf("%s: %s", root.File, err)) + } + } if title, present := mf.TitleField(); present && title == "" { - frontmatterBroken = append(frontmatterBroken, + localBroken = append(localBroken, "frontmatter has an empty 'title:'; give it a value or remove it") } @@ -187,13 +215,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 = append(frontmatterBroken, collision.Error()) + r.broken = append(localBroken, collision.Error()) r.status = statusBroken return r } return r.fail(err, jsonout.CodeConvert) } - r.broken = append(frontmatterBroken, page.Broken...) + r.broken = append(localBroken, page.Broken...) // Label warnings lead: they are a property of the frontmatter, so they hold // whatever the converter went on to find in the body. r.warnings = append(labelSet.Warnings, page.Warnings...) diff --git a/cmd/check/check_test.go b/cmd/check/check_test.go index be039b2..792e431 100644 --- a/cmd/check/check_test.go +++ b/cmd/check/check_test.go @@ -494,3 +494,105 @@ func TestRunScalarLabelsIsFailed(t *testing.T) { t.Errorf("output = %q, want it to name the list form", out) } } + +// check is the one verb that can find a project-wide page_width Confluence +// does not accept without publishing: internal/project cannot validate its own +// value, since it would have to import internal/pagewidth, which imports +// internal/client, which holds a *project.Cache. +func TestRunInvalidProjectPageWidthIsBroken(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "markfluence.yaml"), "page_width: huge\n") + write(t, filepath.Join(dir, "main.md"), "---\ntitle: Main\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, "invalid page_width") { + t.Errorf("output = %q, want the invalid-width message", out) + } + // The message has to name the project file, not the markdown file, which + // has no page_width in it at all. + if !strings.Contains(out, "markfluence.yaml") { + t.Errorf("output = %q, want it to name markfluence.yaml", out) + } +} + +// A valid project-wide width is a default, not a per-file requirement. +func TestRunValidProjectPageWidthIsClean(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "markfluence.yaml"), "page_width: wide\nspace: ENG\n") + write(t, filepath.Join(dir, "main.md"), "---\ntitle: Main\npage_id: 1\n---\n# Main\n") + + if _, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) + }); err != nil { + t.Fatalf("run = %v, want success", err) + } +} + +// A markfluence.yaml that cannot be understood is a local defect in a file the +// author can open and fix, which is exactly check's subject -- so it fails the +// file as VALIDATION rather than as I/O, and without the "resolving the +// documentation root" heading, since the root was found. +func TestRunMalformedProjectFileIsAValidationFailure(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "markfluence.yaml"), "spce: ENG\n") + write(t, filepath.Join(dir, "main.md"), "---\ntitle: Main\npage_id: 1\n---\n# Main\n") + + var env struct { + Results []struct { + Status string `json:"status"` + Code *string `json:"code"` + Error *string `json:"error"` + } `json:"results"` + } + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + 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 err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if len(env.Results) != 1 { + t.Fatalf("results = %#v, want one", env.Results) + } + got := env.Results[0] + if got.Status != "failed" { + t.Errorf("status = %q, want failed", got.Status) + } + if got.Code == nil || *got.Code != "VALIDATION" { + t.Errorf("code = %v, want VALIDATION", got.Code) + } + if got.Error == nil { + t.Fatal("error = nil, want a message") + } + if !strings.Contains(*got.Error, `unknown setting "spce"`) { + t.Errorf("error = %q, want the unknown-setting message", *got.Error) + } + if strings.Contains(*got.Error, "resolving the documentation root") { + t.Errorf("error = %q, want no root-resolution heading", *got.Error) + } +} + +// A project file under a *different* root says nothing about a file checked +// elsewhere: diagnostics stay scoped to the files actually named. +func TestRunProjectDefectIsScopedToItsOwnRoot(t *testing.T) { + base := t.TempDir() + bad := filepath.Join(base, "bad") + good := filepath.Join(base, "good") + write(t, filepath.Join(bad, "markfluence.yaml"), "page_width: huge\n") + write(t, filepath.Join(bad, "main.md"), "---\ntitle: Bad\npage_id: 1\n---\n# Bad\n") + write(t, filepath.Join(good, "markfluence.yaml"), "page_width: wide\n") + write(t, filepath.Join(good, "main.md"), "---\ntitle: Good\npage_id: 2\n---\n# Good\n") + + if _, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(good, "main.md")}) + }); err != nil { + t.Fatalf("checking the good project = %v, want success", err) + } +} diff --git a/cmd/create/create.go b/cmd/create/create.go index 6b9efc0..e15d08a 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -682,7 +682,9 @@ func resolveFile( } root, err := roots.Resolve(filepath.Dir(abs)) if err != nil { - return record{}, fmt.Errorf("resolving the documentation root: %w", err) + // A malformed markfluence.yaml is reported as itself: the root was + // found, and it is the file in it that is wrong. + return record{}, project.RootError(err) } index, err := indexes.Get(root) if err != nil { From 1250f2f0d148be64f43eabd24fba15d5ee8d3ea9 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:07:25 -0400 Subject: [PATCH 08/17] feat(project): report a project file's settings under --debug A project-wide default is invisible by construction: it takes effect for a file that says nothing about it, so "why did this publish to ENG?" has no answer in the file the reader is looking at. --debug is where that answer goes. Only a file that declares something earns a line. The marker that ships declares nothing, and a line per root in a batch would be noise -- the root itself is already reported unconditionally. No --json or schema change: these are inputs, not results. --- internal/project/config.go | 22 +++++++++++ internal/project/config_test.go | 67 +++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/internal/project/config.go b/internal/project/config.go index 5081d3d..0699bc2 100644 --- a/internal/project/config.go +++ b/internal/project/config.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/ui" ) // Config is what a project file declares. Every field is project-*wide*, and @@ -127,9 +128,30 @@ func loadConfig(path string) (Config, error) { cfg.PageWidth = value } } + // A project-wide default is invisible by construction: it takes effect for + // a file that says nothing about it, so "why did this publish to ENG?" has + // no answer in the file the reader is looking at. --debug is where that + // answer goes. Only a file that declares something is worth a line; the + // marker that ships declares nothing, and a line for every root in a batch + // would be noise. The root itself is already reported unconditionally. + if settings := cfg.declared(); len(settings) > 0 { + ui.Debug(fmt.Sprintf("project file %s: %s", path, strings.Join(settings, ", "))) + } return cfg, nil } +// declared lists the settings this Config actually carries, for reporting. +func (c Config) declared() []string { + var out []string + if c.Space != "" { + out = append(out, "space="+c.Space) + } + if c.PageWidth != "" { + out = append(out, "page_width="+c.PageWidth) + } + return out +} + // unknownSetting is the message #100 exists for. One line, because it lands // verbatim in check --json. func unknownSetting(key string) error { diff --git a/internal/project/config_test.go b/internal/project/config_test.go index b6f6275..4be4593 100644 --- a/internal/project/config_test.go +++ b/internal/project/config_test.go @@ -2,10 +2,13 @@ package project import ( "errors" + "io" "os" "path/filepath" "strings" "testing" + + "github.com/mozilla/markfluence/internal/ui" ) // write puts a project file in a fresh directory and returns the directory. @@ -283,3 +286,67 @@ func TestSettingsArePerRoot(t *testing.T) { t.Errorf("spaces = %#v, want one=ENG two=OPS", got) } } + +// A project-wide default takes effect for a file that says nothing about it, +// so "why did this publish to ENG?" has no answer in the file the reader is +// looking at. --debug is where that answer goes. +func TestLoadConfigReportsDeclaredSettingsUnderDebug(t *testing.T) { + ui.SetDebug(true) + t.Cleanup(func() { ui.SetDebug(false) }) + + dir := write(t, "space: ENG\npage_width: wide\n") + out := captureStderr(t, func() { + root, err := Discover(dir) + if err != nil { + t.Fatalf("Discover: %v", err) + } + root.FS.Close() + }) + for _, want := range []string{filepath.Join(dir, Filename), "space=ENG", "page_width=wide"} { + if !strings.Contains(out, want) { + t.Errorf("debug output = %q, want it to contain %q", out, want) + } + } +} + +// The marker that ships declares nothing, and a line for every root in a batch +// would be noise. The root itself is already reported unconditionally. +func TestLoadConfigSaysNothingForAMarkerWithNoSettings(t *testing.T) { + ui.SetDebug(true) + t.Cleanup(func() { ui.SetDebug(false) }) + + dir := write(t, "# Marks the root of a markfluence project.\n") + out := captureStderr(t, func() { + root, err := Discover(dir) + if err != nil { + t.Fatalf("Discover: %v", err) + } + root.FS.Close() + }) + if strings.Contains(out, "project file") { + t.Errorf("debug output = %q, want nothing about the project file", out) + } +} + +// captureStderr runs fn with os.Stderr redirected, returning what it printed. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + saved := os.Stderr + os.Stderr = w + done := make(chan string, 1) + go func() { + var b strings.Builder + _, _ = io.Copy(&b, r) + done <- b.String() + }() + fn() + os.Stderr = saved + _ = w.Close() + out := <-done + _ = r.Close() + return out +} From db7944724812ca31c042f3005fc440b2adf50ad1 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:07:40 -0400 Subject: [PATCH 09/17] docs(export): the marker file is read now, not merely present Two comments said nothing in markfluence.yaml is parsed, which #100 made false. The body export writes is unchanged: it declares no settings, and deliberately -- every file export writes already carries its own space: and page_width: in frontmatter, and frontmatter beats the project file, so a space: in the planted marker would be overridden by every file under it. It would matter only for a file somebody adds later, and even then inconsistently, since an existing marker is never overwritten (S3). --- cmd/export/projectfile.go | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/cmd/export/projectfile.go b/cmd/export/projectfile.go index c0e3833..67e3798 100644 --- a/cmd/export/projectfile.go +++ b/cmd/export/projectfile.go @@ -10,9 +10,17 @@ import ( "github.com/mozilla/markfluence/internal/project" ) -// projectFileBody is what markfluence.yaml holds. Its existence is its whole -// meaning -- nothing in it is parsed -- so it carries a comment saying so, for -// whoever finds it and wonders (_plans/025). +// projectFileBody is what markfluence.yaml holds: a comment and no settings. +// +// A project file can declare project-wide defaults now (#100), and this one +// declares none deliberately. An exported tree needs no space: -- every file +// export writes already carries its own in frontmatter (pagedoc.Frontmatter), +// and frontmatter beats the project file, so the setting would be overridden by +// every file under it. It would matter only for a file somebody adds later, and +// even then inconsistently, since an existing marker is never overwritten. +// +// So the comment stays accurate, and it is here for whoever finds the file and +// wonders what it is for (_plans/025). const projectFileBody = `# Marks the root of a markfluence project. Image and link paths are recorded # relative to this directory. https://github.com/mozilla/markfluence ` @@ -27,7 +35,8 @@ const ( // writeProjectFile plants markfluence.yaml at the destination of a multi-page // export, and reports what it did. // -// It is load-bearing rather than tidy. Without a project file the documentation +// It is load-bearing rather than tidy, and for the root it marks rather than +// for anything it says. Without a project file the documentation // root falls back to a markdown file's own directory, so dest/home/child.md // would take dest/home/ as its root -- and a shared asset reconstructed at // dest/assets/brand.png then sits above that root and republishes as From 103fc1b3ec59df0e0f8b1ff7db796d8adbefdb31 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:10:57 -0400 Subject: [PATCH 10/17] docs: record the project-file settings and the precedence chain docs/root-model.md said the project file's existence was its whole meaning and that nothing in it is parsed. Both are now false. Its section is rewritten around the settings, the flag > frontmatter > project file chain, why an unrecognized key is fatal, and what the file deliberately does not hold -- and the CVE-2022-24765 framing below it is tightened rather than dropped: the file is read now, but still never executed, and nothing in it can redirect a credential. CLAUDE.md had no internal/project bullet at all, which is the same gap the last refresh found: a package doing real work with nothing written down about it. It now carries the two-starting-points reason the package returns a type, the cache's walk, and the loader -- including why loading lives in open(), why an unknown key is fatal, why the vocabulary check is somewhere else, and why Config holds no url. docs/markdown_file.md gains the chain beside the field table, and the page_width row is corrected while it is being touched: it claimed create and update both assert on every publish, where update makes no width request at all for a file that declares none. --help had two stale claims -- create's "the space comes from --space or frontmatter" and update's width sentence -- so docs/commands/ regenerates. docs/guarantees.md notes that a project-wide default sits inside L2's scope in the direction L2 wants: declared on disk, found by the same working-directory-independent walk, and strictly better than the --space flag it replaces. Status unchanged. Also silences errcheck on a few root.FS.Close() calls. --- CLAUDE.md | 3 +- README.md | 19 ++++++- cmd/create/create.go | 9 ++-- cmd/update/update.go | 5 +- docs/commands/markfluence_create.md | 9 ++-- docs/commands/markfluence_update.md | 5 +- docs/guarantees.md | 7 +++ docs/markdown_file.md | 21 +++++++- docs/root-model.md | 80 +++++++++++++++++++++++++---- internal/client/config.go | 2 +- internal/project/config_test.go | 16 +++--- 11 files changed, 141 insertions(+), 35 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6e937d8..c1f8b07 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,7 +25,7 @@ The README is a 50,000-foot view — what markfluence does, which command to rea **`docs/commands/` is generated, not written.** `tools/gendocs` renders every command's help to markdown via cobra's doc generator (using `cmd.Root()`, exported for that one caller — a `main()` inside `cmd` would land in the binary, and a `_test.go` that writes into the repo is not a test). It is a second copy of the help text, and only safe because it cannot drift: `make docs-check` regenerates into a temp dir and diffs, inside `make check`. `DisableAutoGenTag` is set, or a timestamp would make every regeneration a diff and the guard noise. **Never hand-edit a file under `docs/commands/`.** -The rest of `docs/` is written: `markdown_file.md` (the page format — every frontmatter field and every body construct, the two halves that used to be split between the README and a separate file), `json-output.md` (per-command `--json` detail the schema cannot express as reasoning), `github-actions.md` (CI setup), `root-model.md`, `guarantees.md`, and `confluence/`. +The rest of `docs/` is written: `markdown_file.md` (the page format — every frontmatter field and every body construct, the two halves that used to be split between the README and a separate file), `json-output.md` (per-command `--json` detail the schema cannot express as reasoning), `github-actions.md` (CI setup), `root-model.md` (the documentation root *and* `markfluence.yaml`'s settings), `guarantees.md`, and `confluence/`. A change that adds a package, exports a function, adds a make target, or introduces a generated artifact belongs in **this file** as part of the same change. The only time it has drifted was a docs-only branch that felt like it needed no architecture note while adding all four. @@ -70,6 +70,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `cmd/attachment{list,upload,download}/` — the flat `attachment-list`/`attachment-upload`/`attachment-download` commands (noun-first so cobra's alphabetized help keeps them together and `attachment-` completes as a group). `upload` reuses the checksum skip/update logic, with `--force` (`client.ForceUploadAttachments`) and `--dry-run` (`PlanAttachments`); its `--name` takes a *path* whose base name becomes the stored name, and the recorded `path=` is the path as given, so a later publish can't create a duplicate under a different name; a batch whose base names collide is refused, since `planAttachments` reads the page's attachments once before its loop and would otherwise plan two creates for one name. `download` restores an attachment to its recorded `path=`, using the stored name verbatim when there is none (never interpreting it — a file really called `a%2Fb.png` must not be scattered into `a/b.png`, and `convert.sourceFor` answers the same way on the markdown side, which is what keeps a downloaded file where the markdown says it is), with `--flat` to opt out; `destPath` is the only place server data becomes a filesystem path and clamps to `--dest`, refusing rather than clipping an escape, since `..` is legitimate in a source path. - `cmd/schema/` — `schema`: print the embedded `--json` schema to stdout verbatim (no args, no credentials, no Confluence call). `--json` is deliberately a no-op — the output is already the schema document, not an envelope — which is also why `schema` is absent from the schema's own `command` enum. - `schema/` — the published JSON Schema (`json-output/v1.json`) *and* the `schema` Go package that embeds it (`V1`). The Go file lives beside the schema because `go:embed` cannot reach outside its own directory, and the schema stays at a top-level path a non-Go consumer can browse, mirroring its own `$id`. `internal/schematest` validates against the embed rather than reading the file, which is what makes "what ships" and "what the tests checked" the same bytes — do not reintroduce a disk read or a second copy. The version number is **not** restated here: `jsonout.SchemaVersion` and the document's own `schema_version` const are the two copies, tied together by a test in `cmd/schema`. The envelope's and the error object's top-level **`warnings`** are the one field no command fills: `jsonout.NewEnvelope`/`EmitError` drain a package-level collector (`AddWarning`), because the only thing in it is raised during credential resolution — below any command, before either document exists. +- `internal/project` — the documentation root: `Discover` (walk up from a directory looking for `markfluence.yaml`), `FromPath` (`--root`), `Resolve`, and `Cache`, which consults itself at every level of the walk so a batch spanning a subtree pays for the walk — and `os.OpenRoot` — once rather than per directory (the quadratic cost `_plans/025` measured). A `Root` carries `Dir`, `File`, `Config` and an `os.Root` that refuses an escape even through a symlink partway down. `Discover` is called from **two starting points for two reasons** — once per invocation from the working directory to locate `.env`, and once per markdown file from its own directory to bound its reads and name its attachments — which is why it returns a type rather than a string; the two diverge legitimately, so a multi-root batch is allowed and nothing refuses it ([docs/root-model.md](docs/root-model.md)). `config.go` reads the project file's **settings** (#100): `space` and `page_width`, resolving **flag > frontmatter > project file**, which is *not* the credentials chain and must never be conflated with it. Three things about it are load-bearing. It is read through `frontmatter.Dialect.ReadMapping` rather than a second parser, since every rule there was found by probing goccy and a second copy would be a second set of the same bugs. An **unknown top-level key is fatal**, and that is the point rather than a cost — a silently ignored `spce: ENG` is wrong for every file at once, and a file written for a newer markfluence holds keys this binary would ignore, so there is no schema version and this must not be loosened; an empty or comment-only file stays valid, being what ships and what `export` plants. And loading happens in `open()`, the single place a `Root` is built from a marker hit, so `Discover`/`Cache`/`FromPath` cannot disagree that a file which cannot be understood **is not a valid marker**: the walk does not continue upward and does not fall back to the starting directory, because the root decides every attachment name and guessing at it is worse than stopping. `ConfigError`/`IsConfigError`/`RootError` exist so a caller reports that as a local defect (`VALIDATION`) rather than under `resolving the documentation root` as I/O. What it validates is **structure only**: `internal/pagewidth` cannot be imported here (`pagewidth` → `client` → `project`), so a width's vocabulary is checked by `pagewidth.Declared` where it already runs and by `check`'s offline lint. `Config` deliberately holds **no `url` or token**, and the reason is sharper than "those are credentials": basic auth goes to whatever host the resolved URL names, so a committed, walked-up file naming one would decide where `CONFLUENCE_TOKEN` is sent — a worse version of the `.env` hole #136 records. - `internal/pageslug` — `Slug`/`For`/`Filename`: a title to a filename-safe slug. A package rather than a helper because `export`, `read` and `attachment-download` all place attachments under a page's own directory and must agree. It lowercases (so case-variant titles collide and can be caught) and drops `/` (so no title can inject a path separator); it is lossy, and no readable slug can avoid being, so the caller decides what a collision means. Known limit: NFD and NFC spellings of one title are different Go strings but one filename on APFS, so that pair is not disambiguated. - `internal/pagedoc` — a fetched page as a markdown document: `Render` (frontmatter + converted body), `Frontmatter`, and the lookups the converter can't do for itself — `Sources`/`SourcesFrom` (attachment name → recorded source path) and `PageLinks` (the page an `` points at → its URL). **One conversion, parameterized by a `Placement`**: where the page's file sits, where its unrecorded attachments go, what `parent:` says, and the attachment listing the caller already has. `read`, `export` and `attachment-download` all go through `Options`/`AttachmentDirFor` rather than assembling their own, so they cannot drift by accident — only by argument. For a page at the top level of what is being written, which is what `read` prints and what a single-page export writes, `read` and `export` are byte-identical; deeper in a tree they differ in exactly the position-dependent parts (a sourced attachment's `../` prefix, a `-` suffix a sibling forced, and `parent:`), because `read` has no tree to be positioned in. It needs a client (page width, attachment list, title lookups), which is why it isn't in `internal/convert` — that package is deliberately client-free, and it's why `StorageToMarkdown` takes those maps rather than fetching them. Every one of them is best-effort in the same shape: no references in the body means no request at all, and a lookup that fails is omitted rather than fatal (an omitted page link renders as raw storage, not as a link with no destination). It also owns **`UserCache`**, the user-name lookup both mention directions share (#91): a per-run, **cross-page** cache, threaded in from the caller the way `project.Cache`/`linkindex.Cache` are, because the obvious structure is wrong — `PageLinks` builds its space-id map per page and `Options` is built per page, so a user map written that way would re-resolve the same twelve people on every page of a 200-page export. It **remembers misses**, or a page mentioning deactivated people costs a request each, every page, to learn the same failures. Not persisted to disk, and the reason is **L2**: output must depend only on the files on disk, not on what a cache happens to hold. `MentionWarnings` is the forward direction's use of it, and sharing the cache is what makes that warning affordable — publishing needs no display names at all, so that lookup exists purely to report an id that names nobody. `PageLinks` resolves a space id **once per space key**, not once per link, and refuses to search site-wide when it can't scope a title to a space — a same-titled page in the wrong space is a wrong answer, which is worse than the passthrough a miss produces. - `internal/attachfile` — `Resolve` (where an attachment goes under a destination root, **including the traversal clamp**) and `Write` (download it there, honoring force/dry-run). Shared by `attachment-download` and `export`; the clamp must never exist in two copies. diff --git a/README.md b/README.md index aa03d2c..1b2f57c 100644 --- a/README.md +++ b/README.md @@ -483,6 +483,21 @@ storage markup. # relative to this directory. https://github.com/mozilla/markfluence ``` +It can also carry **project-wide defaults**, which is what saves a hundred +files from each repeating `space: ENG`: + +```yaml +space: ENG +page_width: max +``` + +Each is a default a file overrides: the chain is **flag > frontmatter > +project file**, so the answer closest to the content wins. A key markfluence +does not recognise is an error rather than something ignored — a typo in a +project-wide default is wrong for every file at once. Credentials are +deliberately not settings here; see +[docs/root-model.md](docs/root-model.md#what-it-deliberately-does-not-hold). + The rest of this section is the precise version of the same idea. Every markdown file has a **documentation root**: the directory holding `markfluence.yaml`, found by walking up from the file's own directory, or — @@ -494,8 +509,8 @@ overrides discovery for the whole invocation — and, for `create`, `update`, and `attachment-upload`, also redirects where `.env` is read from (see [Configure](#configure)). -For the reasoning behind this model — why a bare marker file, what it fixes, -what it costs — see [docs/root-model.md](docs/root-model.md) and +For the reasoning behind this model — what it fixes, what it costs, and every +project-wide setting — see [docs/root-model.md](docs/root-model.md) and [_plans/025_file-organization.md](_plans/025_file-organization.md). ### Moving files and assets diff --git a/cmd/create/create.go b/cmd/create/create.go index e15d08a..0355796 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -57,10 +57,11 @@ var Cmd = &cobra.Command{ Short: "Create new Confluence pages from markdown files", Long: "Create new Confluence pages from markdown FILEs.\n\n" + "The title comes from frontmatter, or from --title, which overrides it and\n" + - "requires a single FILE. The space comes from --space or frontmatter. The\n" + - "parent comes from --parent or frontmatter and may be a page or a Cloud\n" + - "folder -- give a folder's id the same way you would a page's. Page width\n" + - "defaults to max.\n\n" + + "requires a single FILE. The space comes from --space, then frontmatter,\n" + + "then a space: in markfluence.yaml -- the answer closest to the content\n" + + "wins. The parent comes from --parent or frontmatter and may be a page or\n" + + "a Cloud folder -- give a folder's id the same way you would a page's.\n" + + "Page width follows the same chain and defaults to max.\n\n" + "Every file is checked first -- including converting it -- and if any would\n" + "fail, nothing is created. A page_id that resolves to nothing is a failure\n" + "too, not a fresh page: create will not publish a second copy and overwrite\n" + diff --git a/cmd/update/update.go b/cmd/update/update.go index 880cfff..7c732ec 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -43,8 +43,9 @@ var Cmd = &cobra.Command{ "Title and page id are read from each file's YAML frontmatter; --title and\n" + "--page-id override the frontmatter (and require a single FILE). A page id is\n" + "required (from --page-id or frontmatter); update errors if none is set.\n\n" + - "Page width is asserted only when set via --page-width or a page_width\n" + - "frontmatter line -- otherwise the live page's width is left untouched.\n" + + "Page width is asserted only when set via --page-width, a page_width\n" + + "frontmatter line, or a page_width: in markfluence.yaml -- otherwise the\n" + + "live page's width is left untouched.\n" + "Labels work the same way: a labels: line is asserted exactly (anything on\n" + "the page the file does not list is removed), and no labels: line means the\n" + "page's labels are left alone, not even read.\n\n" + diff --git a/docs/commands/markfluence_create.md b/docs/commands/markfluence_create.md index 63db7be..dd37389 100644 --- a/docs/commands/markfluence_create.md +++ b/docs/commands/markfluence_create.md @@ -7,10 +7,11 @@ Create new Confluence pages from markdown files Create new Confluence pages from markdown FILEs. The title comes from frontmatter, or from --title, which overrides it and -requires a single FILE. The space comes from --space or frontmatter. The -parent comes from --parent or frontmatter and may be a page or a Cloud -folder -- give a folder's id the same way you would a page's. Page width -defaults to max. +requires a single FILE. The space comes from --space, then frontmatter, +then a space: in markfluence.yaml -- the answer closest to the content +wins. The parent comes from --parent or frontmatter and may be a page or +a Cloud folder -- give a folder's id the same way you would a page's. +Page width follows the same chain and defaults to max. Every file is checked first -- including converting it -- and if any would fail, nothing is created. A page_id that resolves to nothing is a failure diff --git a/docs/commands/markfluence_update.md b/docs/commands/markfluence_update.md index 038a5a1..3765666 100644 --- a/docs/commands/markfluence_update.md +++ b/docs/commands/markfluence_update.md @@ -10,8 +10,9 @@ Title and page id are read from each file's YAML frontmatter; --title and --page-id override the frontmatter (and require a single FILE). A page id is required (from --page-id or frontmatter); update errors if none is set. -Page width is asserted only when set via --page-width or a page_width -frontmatter line -- otherwise the live page's width is left untouched. +Page width is asserted only when set via --page-width, a page_width +frontmatter line, or a page_width: in markfluence.yaml -- otherwise the +live page's width is left untouched. Labels work the same way: a labels: line is asserted exactly (anything on the page the file does not list is removed), and no labels: line means the page's labels are left alone, not even read. diff --git a/docs/guarantees.md b/docs/guarantees.md index f7366fa..a89ca33 100644 --- a/docs/guarantees.md +++ b/docs/guarantees.md @@ -194,6 +194,13 @@ finds the root by walking up from each file's own directory, independent of the working directory and of what else is in the same command (`_plans/026` commits 1–4). +A project-wide `space:` or `page_width:` in `markfluence.yaml` (#100) sits +inside that scope rather than straining it, and in the direction L2 wants: the +value is declared in a committed file on disk, found by the same +working-directory-independent walk, so two people in different directories +resolve it identically. It is strictly better for L2 than the `--space` flag it +replaces, which is invocation state by definition. Status unchanged. + **L3** is what makes moving a page free. `images.go` records an attachment's `Source` relative to the root rather than to the referencing page, so identity follows the asset alone (`_plans/026` commit 4). diff --git a/docs/markdown_file.md b/docs/markdown_file.md index ec79409..737b998 100644 --- a/docs/markdown_file.md +++ b/docs/markdown_file.md @@ -42,15 +42,32 @@ A page genuinely titled `null` is written `title: "null"`. | Field | Value domain | Notes | | --- | --- | --- | -| `space` | a space key (e.g. `ENG`, or a personal space like `~1234abcd`) | Target space for `create` (or pass `--space`); written back by `create`. Always a key, never a numeric space id. | +| `space` | a space key (e.g. `ENG`, or a personal space like `~1234abcd`) | Target space for `create` (or pass `--space`, or set a project-wide `space:` — see below); written back by `create`. Always a key, never a numeric space id. | | `parent` | `null`, a numeric page **or folder** id, or a relative `.md` path | `null` = top-level page; an id = an existing parent, which may be a page or a Cloud folder (the value is just an id either way — nothing records which kind it is); a `.md` path = a parent authored in the same run (`create` resolves it in dependency order, then rewrites the value to ` # `). Used by `create` (or `--parent`). | | `page_id` | a numeric page id, or `null` | The target page. `update` looks it up by `title` and writes it back when missing; `create` writes it after creating the page. `null`/absent means "no page yet." | | `title` | text (**required**) | The Confluence page title. | | `labels` | a list of label names, e.g. `[ci/cd, howto]` | The page's labels. **Present means asserted exactly** — a label on the page that the file does not list is removed — and `labels: []` removes them all. **Absent means untouched**, so a page labeled by hand is safe from a run that never mentioned labels. Only `global:` labels are managed; a `my:`/`team:` label is shown by `info` and never written or removed — and if an unmanaged label shares a name with a surplus managed one, the removal is skipped with a warning, because Confluence's removal takes a name with no prefix and would delete the personal label instead. Names are lowercased (with a warning) since Confluence does that anyway; anything else invalid is an error before any write. `fix` writes back the live page's labels, which is how you adopt a page labeled in the UI. | -| `page_width` | `narrow`, `wide`, or `max` | The published page width (the UI's "Adjust width" options; `narrow`/`wide`/`max` map to the `default`/`full-width`/`max` appearance properties). Absent or blank defaults to `max`. `create`/`update` assert it on every publish (so a width set in the Confluence UI is overwritten unless the frontmatter matches); `fix` writes back the live page's width. | +| `page_width` | `narrow`, `wide`, or `max` | The published page width (the UI's "Adjust width" options; `narrow`/`wide`/`max` map to the `default`/`full-width`/`max` appearance properties). A declared width is **asserted on every publish**, so a width set in the Confluence UI is overwritten unless the file matches. The two verbs differ on what *absent* means: `create` defaults to `max`, while `update` leaves the live width alone and makes no width request at all. `--page-width` and a project-wide `page_width:` both count as declared (see below). `fix` writes back the live page's width. | To create a page, you only need to specify the `title` in the frontmatter. +### Project-wide defaults + +`space` and `page_width` can be declared once for a whole project, in the +`markfluence.yaml` that marks the [documentation +root](root-model.md#markfluenceyaml-the-project-file): + +```yaml +space: ENG +page_width: max +``` + +The chain is **flag > frontmatter > project file** — the answer closest to the +content wins — and the project file is only consulted when both levels above it +are silent, so it never conflicts with either. No other frontmatter field has a +project-wide form: `title` and `page_id` are per page by definition, and +`parent` varies per file. + ## Body The rest of this file is the body reference, construct by construct: what each diff --git a/docs/root-model.md b/docs/root-model.md index ed8adb1..73e64c2 100644 --- a/docs/root-model.md +++ b/docs/root-model.md @@ -49,25 +49,87 @@ the walk itself is paid for once, not twice. ## `markfluence.yaml`: the project file -Its existence is its whole meaning. Nothing in it is parsed or read; the -`.yaml` extension fixes the intended format for when a key is eventually -added (see [#100](https://github.com/mozilla/markfluence/issues/100)) without -that being a migration. It should carry a one-line comment saying what it -does, since a reader who finds it should be able to tell without already -knowing: +It marks the root, and it declares project-wide settings +([#100](https://github.com/mozilla/markfluence/issues/100)). A file with no +settings in it is perfectly normal — it is what `export` plants, and marking +the root was the file's only job until settings arrived: ```yaml # Marks the root of a markfluence project. Image and link paths are recorded # relative to this directory. https://github.com/mozilla/markfluence ``` +### The settings + +| Setting | What it defaults | Overridden by | +|---|---|---| +| `space` | the space `create` publishes into | `--space`, then a frontmatter `space:` | +| `page_width` | the width `create` and `update` assert | `--page-width`, then a frontmatter `page_width:` | + +```yaml +space: ENG +page_width: max +``` + +The chain is **flag > frontmatter > project file**: the answer closest to the +content wins. This is *not* the credentials chain, and conflating the two is +the mistake the file's design forecloses — credentials resolve **flag > +environment > `.env`** and answer *who you are*, where a setting here answers +*what the content is*. + +A project-wide setting is only ever read when both levels above it are silent, +so it never participates in a disagreement: `create` still refuses a `--space` +that contradicts a frontmatter `space:`, and a project default cannot become a +third party to that. + +One consequence worth knowing before you add `page_width:`: `update` asserts a +width only when one is declared, and a project-wide declaration counts. A +project that wants each page's live width left alone as it is should leave the +key out. + +Settings are per-root, so an invocation spanning two projects gets each +project's own defaults — see [Multi-root batches](#multi-root-batches-are-allowed). + +### A file that cannot be understood stops the command + +An unparseable file, or one holding a key markfluence does not recognise, is an +error naming what is wrong. It is specifically **not** treated as a valid root +marker: discovery does not walk on to an ancestor that happens to have a better +one, and does not fall back to the markdown file's own directory. The root +decides every attachment name and bounds every read, so a project file that +cannot be understood means the project's boundary is unknown, and guessing is +worse than stopping. + +**Refusing an unrecognised key is the point, not a limitation.** A +`markfluence.yaml` written for a newer markfluence holds keys an older binary +would ignore, and ignoring a project-wide default means publishing with the +wrong space or the wrong width — silently, everywhere at once. It is also what +catches `spce: ENG`. So the file carries no schema version, and the error says +that an older binary is the likely cause. + +`markfluence check` validates a project file offline, alongside the markdown +files you give it. + +### What it deliberately does not hold + +No `url`, `username`, or token. The reason is sharper than "those are +credentials": markfluence sends basic auth to whatever host the resolved URL +names, so a `url:` here would decide where `CONFLUENCE_TOKEN` is sent — and +this file is committed, shared, and walked up to from a subdirectory. One line +in a pull request would redirect a CI run's token to a host of the author's +choosing. + +The asymmetry against the settings it does hold is the whole argument: a wrong +`space` publishes to the wrong place in your own instance, which is visible and +`fix` recovers it. A wrong `url` hands out the token, which is neither. + Committed and shared, unlike `.env`, which stays gitignored and personal. A stray `.env` in an ancestor directory can hand a project credentials that aren't its own — which is exactly why the root (and, by extension, where `.env` was read from) is reported: visibility is the mitigation, not a -permission check. `markfluence` reads nothing from inside a project file and -executes nothing on account of its presence — walking up and trusting what's -found there is the shape of +permission check. `markfluence` *reads* a project file but **executes** nothing +on account of its presence, and nothing in it can redirect a credential — +walking up and trusting what's found there is the shape of [CVE-2022-24765](https://github.blog/2022-04-12-git-security-vulnerability-announced/) (pre-fix git walking up for `.git` with no ownership check) and of the `.git`-directory hook-execution CVEs that followed it (e.g. CVE-2024-32002), diff --git a/internal/client/config.go b/internal/client/config.go index 76638c2..90998ff 100644 --- a/internal/client/config.go +++ b/internal/client/config.go @@ -181,7 +181,7 @@ func dotenvDir(cwd string, roots *project.Cache) (string, error) { if err != nil { return "", err } - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() return root.Dir, nil } diff --git a/internal/project/config_test.go b/internal/project/config_test.go index 4be4593..5e3b24c 100644 --- a/internal/project/config_test.go +++ b/internal/project/config_test.go @@ -27,7 +27,7 @@ func TestDiscoverReadsSettings(t *testing.T) { if err != nil { t.Fatalf("Discover: %v", err) } - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() if root.Config.Space != "ENG" { t.Errorf("Space = %q, want ENG", root.Config.Space) } @@ -51,7 +51,7 @@ func TestDiscoverAcceptsAMarkerWithNoSettings(t *testing.T) { if err != nil { t.Fatalf("Discover: %v", err) } - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() if root.Config != (Config{}) { t.Errorf("Config = %#v, want zero", root.Config) } @@ -136,7 +136,7 @@ func TestDiscoverDoesNotFallBackToStartDirOnAMalformedFile(t *testing.T) { dir := write(t, "spce: OPS\n") root, err := Discover(dir) if err == nil { - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() t.Fatalf("Discover returned root %q, want an error", root.Dir) } } @@ -190,7 +190,7 @@ func TestFromPathReadsSettings(t *testing.T) { if err != nil { t.Fatalf("FromPath: %v", err) } - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() if root.Config.Space != "ENG" { t.Errorf("Space = %q, want ENG", root.Config.Space) } @@ -211,7 +211,7 @@ func TestFromPathWithNoProjectFile(t *testing.T) { if err != nil { t.Fatalf("FromPath: %v", err) } - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() if root.File != "" || root.Config != (Config{}) { t.Errorf("File = %q, Config = %#v, want empty", root.File, root.Config) } @@ -251,7 +251,7 @@ func TestLoadConfigTreatsAnEmptySettingAsUnset(t *testing.T) { if err != nil { t.Fatalf("Discover: %v", err) } - defer root.FS.Close() + defer func() { _ = root.FS.Close() }() if root.Config != (Config{}) { t.Errorf("Config = %#v, want zero", root.Config) } @@ -300,7 +300,7 @@ func TestLoadConfigReportsDeclaredSettingsUnderDebug(t *testing.T) { if err != nil { t.Fatalf("Discover: %v", err) } - root.FS.Close() + _ = root.FS.Close() }) for _, want := range []string{filepath.Join(dir, Filename), "space=ENG", "page_width=wide"} { if !strings.Contains(out, want) { @@ -321,7 +321,7 @@ func TestLoadConfigSaysNothingForAMarkerWithNoSettings(t *testing.T) { if err != nil { t.Fatalf("Discover: %v", err) } - root.FS.Close() + _ = root.FS.Close() }) if strings.Contains(out, "project file") { t.Errorf("debug output = %q, want nothing about the project file", out) From 0c8e064e25a1ec3b9e2c5b06b83a3b0330e9da0a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:15:35 -0400 Subject: [PATCH 11/17] docs: record why create still persists a project-wide space Found while testing #100 live: create writes the space it resolved back into the file, so a file create makes carries space: ENG even when the project file already said it, and the dedup pays off only for hand-written files that are only ever updated. Left alone. The target workflow is CI updating existing pages, not creating them -- a person creates the page and wires the repo up so a workflow can update it, which is the division #139 already records as "CI updates; humans create". create is the verb a human runs once with their own flags, and the write-back recording what it did is a feature there. Not persisting a project-supplied value would also make a file's frontmatter silently depend on where it sits, and #139 supersedes the problem by writing a pages: entry instead. --- _plans/038_project-file-settings.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/_plans/038_project-file-settings.md b/_plans/038_project-file-settings.md index c57327f..9a84153 100644 --- a/_plans/038_project-file-settings.md +++ b/_plans/038_project-file-settings.md @@ -333,6 +333,26 @@ even there inconsistently, since `writeProjectFile` never overwrites an existing marker (S3), so the setting would be present or absent depending on whether one was already there. +### `create --persist` still writes `space:` into the file — settled 2026-09-12 + +Found while testing live: `create` persists the space it resolved, so a file +`create` makes carries `space: ENG` even when the project file already said it. +The dedup this issue exists for therefore pays off only for files that are +hand-written and only ever `update`d. + +Left alone, deliberately. **The target workflow is CI updating existing pages, +not creating them** — a person creates the page and wires the repo up so a GHA +workflow can `update` it afterwards, which is the same division #139 records as +"CI updates; humans create" (creating in CI would mean a workflow committing a +new `page_id` back to the repo). So `create` is the verb a human runs once, with +their own flags, and the write-back recording what it actually did is a feature +there rather than duplication. + +The alternative — not persisting a field whose value came from the project file +— would make a file's frontmatter silently depend on where it sits, and #139 +supersedes the problem properly by having `create` write a `pages:` entry +instead. + ## Out of scope - **`pages:`** — #139, blocked on this. From 5516a31bf088bdf46b2ec83ba82bf5d6e6b5cc0f Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:32:31 -0400 Subject: [PATCH 12/17] fix(frontmatter): restore the flat-mapping refusal's wording The refactor claimed to be behavior-preserving and was not. Threading Item into the flat-mapping message turned frontmatter must be a flat mapping of key: value pairs, found String into frontmatter must be a flat mapping of frontmatter: value pairs, found String a user-facing string that lands verbatim in check --json. Item names a *named* key -- `setting "space" must be ...` -- and reads as nonsense in a sentence about keys in general, so that word is literal again for both dialects. It survived review and the test suite because the existing assertion matched the substring "flat mapping", which both spellings contain. TestParseErrorWordingIsExact now pins every message frontmatter composes as a whole sentence. goccy's own parse errors stay out of it: their wording belongs to the dependency, and pinning it would make a goccy bump look like a markfluence regression. --- internal/frontmatter/dialect.go | 12 ++- internal/frontmatter/dialect_test.go | 4 +- internal/frontmatter/frontmatter_test.go | 64 +++++++++++++++ internal/project/config_test.go | 99 ++++++++++++++++++++---- 4 files changed, 162 insertions(+), 17 deletions(-) diff --git a/internal/frontmatter/dialect.go b/internal/frontmatter/dialect.go index 4378dd6..488de32 100644 --- a/internal/frontmatter/dialect.go +++ b/internal/frontmatter/dialect.go @@ -31,6 +31,10 @@ import ( // the two kinds of message read differently: `frontmatter must be a flat // mapping` and `setting "space" must be a single scalar value` are both right, // and neither noun works in the other's sentence. +// +// Item goes only where a *named* key is the subject. A sentence about keys in +// general says "key" literally, which is why the flat-mapping refusal takes +// neither field for that word. type Dialect struct { Doc string Item string @@ -116,8 +120,12 @@ func (r reader) parse(text string) (*block, error) { case *ast.CommentGroupNode: return &block{mapping: emptyMapping(), orphan: b}, nil default: - return nil, fmt.Errorf("%s must be a flat mapping of %s: value pairs, found %s", - r.Doc, r.Item, b.Type()) + // "key: value pairs" is literal, not r.Item: Item names one key in a + // sentence about that key ("setting \"space\" must be ..."), and reads + // as nonsense here -- this said "a flat mapping of frontmatter: value + // pairs" before a test pinned the whole sentence. + return nil, fmt.Errorf("%s must be a flat mapping of key: value pairs, found %s", + r.Doc, b.Type()) } } diff --git a/internal/frontmatter/dialect_test.go b/internal/frontmatter/dialect_test.go index ca51d04..a848f95 100644 --- a/internal/frontmatter/dialect_test.go +++ b/internal/frontmatter/dialect_test.go @@ -98,8 +98,8 @@ func TestReadMappingRefusals(t *testing.T) { "tag": {"space: !!str ENG\n", `setting "space" must be a single scalar value`}, "block element block": {"labels:\n - |-\n a\n", `setting "labels[0]" must be a single scalar value`}, "continued scalar": {"space: ENG\n OPS\n", `must be a single-line scalar`}, - "top-level sequence": {"- ENG\n", "a project file must be a flat mapping of setting: value pairs"}, - "bare scalar": {"ENG\n", "a project file must be a flat mapping of setting: value pairs"}, + "top-level sequence": {"- ENG\n", "a project file must be a flat mapping of key: value pairs"}, + "bare scalar": {"ENG\n", "a project file must be a flat mapping of key: value pairs"}, "second document": {"space: ENG\n...\nspace: OPS\n", "a project file must be a single document"}, "duplicate key": {"space: ENG\nspace: OPS\n", "already defined"}, "tab indent": {"labels:\n\t- a\n", "cannot start any token"}, diff --git a/internal/frontmatter/frontmatter_test.go b/internal/frontmatter/frontmatter_test.go index 7bdf664..56ce9c6 100644 --- a/internal/frontmatter/frontmatter_test.go +++ b/internal/frontmatter/frontmatter_test.go @@ -798,3 +798,67 @@ func TestUnknownListKeyIsStillAllowed(t *testing.T) { t.Errorf("reviewers = %q, want [ana bo]", got) } } + +// TestParseErrorWordingIsExact pins the full sentence of every message +// frontmatter composes itself, not a substring of it. +// +// This exists because the substring assertions above did not notice a refactor +// that changed "a flat mapping of key: value pairs" into "a flat mapping of +// frontmatter: value pairs": both contain "flat mapping". These strings are +// user-facing and land verbatim in `check --json`, so the whole sentence is the +// contract. goccy's own parse errors are excluded -- their wording belongs to +// the dependency, and pinning it here would make a goccy bump look like a +// markfluence regression. +func TestParseErrorWordingIsExact(t *testing.T) { + tests := map[string]struct{ content, want string }{ + "top-level scalar": { + "---\njust text\n---\nx\n", + "doc.md: frontmatter must be a flat mapping of key: value pairs, found String", + }, + "top-level list": { + "---\n- a\n---\nx\n", + "doc.md: frontmatter must be a flat mapping of key: value pairs, found Sequence", + }, + "second document": { + "---\ntitle: A\n...\ntitle: B\n---\nx\n", + `doc.md: frontmatter must be a single document: remove the "..." line`, + }, + "nested value": { + "---\ntitle:\n a: b\n---\nx\n", + `doc.md: frontmatter "title" must be a single scalar value, found Mapping`, + }, + "literal block": { + "---\ntitle: |\n lit\n---\nx\n", + `doc.md: frontmatter "title" must be a single scalar value, found Literal`, + }, + "continued scalar": { + "---\ntitle: a plain\n continued\n---\nx\n", + `doc.md: frontmatter "title" must be a single-line scalar; ` + + "a value split over several lines is not supported", + }, + "list element block": { + "---\nlabels:\n - |\n lit\n---\nx\n", + `doc.md: frontmatter "labels[0]" must be a single scalar value, found Literal`, + }, + "list element continued": { + "---\nlabels:\n - a plain\n continued\n---\nx\n", + `doc.md: frontmatter "labels[0]" must be a single-line scalar; ` + + "a list element split over several lines is not supported", + }, + "list where a scalar is required": { + "---\nparent: [a, b]\n---\nx\n", + `doc.md: frontmatter "parent" must be a single value, not a list`, + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + _, err := frontmatter.Parse("doc.md", tt.content) + if err == nil { + t.Fatalf("Parse(%q) = nil error, want one", tt.content) + } + if err.Error() != tt.want { + t.Errorf("error =\n %q\nwant\n %q", err, tt.want) + } + }) + } +} diff --git a/internal/project/config_test.go b/internal/project/config_test.go index 5e3b24c..abc9e6f 100644 --- a/internal/project/config_test.go +++ b/internal/project/config_test.go @@ -89,7 +89,7 @@ func TestDiscoverRefusesAMalformedFile(t *testing.T) { // A goccy parse error carries its own [line:col] and no noun of ours; // ConfigError is what names the file. "parse error": {"space: [ENG\n", "sequence end token"}, - "top-level list": {"- ENG\n", "must be a flat mapping of setting: value pairs"}, + "top-level list": {"- ENG\n", "must be a flat mapping of key: value pairs"}, "list where scalar": {"space: [ENG, OPS]\n", `setting "space" must be a single value, not a list`}, "nested mapping": {"space:\n key: ENG\n", `setting "space" must be a single scalar value`}, "duplicate key": {"space: ENG\nspace: OPS\n", "already defined"}, @@ -289,19 +289,19 @@ func TestSettingsArePerRoot(t *testing.T) { // A project-wide default takes effect for a file that says nothing about it, // so "why did this publish to ENG?" has no answer in the file the reader is -// looking at. --debug is where that answer goes. -func TestLoadConfigReportsDeclaredSettingsUnderDebug(t *testing.T) { +// looking at. ReportSettings is where that answer goes. +func TestReportSettingsNamesEveryDeclaredSetting(t *testing.T) { ui.SetDebug(true) t.Cleanup(func() { ui.SetDebug(false) }) dir := write(t, "space: ENG\npage_width: wide\n") - out := captureStderr(t, func() { - root, err := Discover(dir) - if err != nil { - t.Fatalf("Discover: %v", err) - } - _ = root.FS.Close() - }) + c := NewCache("") + defer c.Close() + if _, err := c.Resolve(dir); err != nil { + t.Fatalf("Resolve: %v", err) + } + + out := captureStderr(t, func() { ReportSettings(c) }) for _, want := range []string{filepath.Join(dir, Filename), "space=ENG", "page_width=wide"} { if !strings.Contains(out, want) { t.Errorf("debug output = %q, want it to contain %q", out, want) @@ -311,11 +311,33 @@ func TestLoadConfigReportsDeclaredSettingsUnderDebug(t *testing.T) { // The marker that ships declares nothing, and a line for every root in a batch // would be noise. The root itself is already reported unconditionally. -func TestLoadConfigSaysNothingForAMarkerWithNoSettings(t *testing.T) { +func TestReportSettingsSaysNothingForAMarkerWithNoSettings(t *testing.T) { ui.SetDebug(true) t.Cleanup(func() { ui.SetDebug(false) }) dir := write(t, "# Marks the root of a markfluence project.\n") + c := NewCache("") + defer c.Close() + if _, err := c.Resolve(dir); err != nil { + t.Fatalf("Resolve: %v", err) + } + + out := captureStderr(t, func() { ReportSettings(c) }) + if strings.Contains(out, "project file") { + t.Errorf("debug output = %q, want nothing about the project file", out) + } +} + +// Loading must not print. It happens once per root for two unrelated reasons +// -- a markdown file's root, and the separate walk from the working directory +// that only locates .env -- so a line emitted during a load described +// whichever root came first and fired for commands (info, search) that read no +// settings at all. +func TestLoadingAProjectFilePrintsNothing(t *testing.T) { + ui.SetDebug(true) + t.Cleanup(func() { ui.SetDebug(false) }) + + dir := write(t, "space: ENG\n") out := captureStderr(t, func() { root, err := Discover(dir) if err != nil { @@ -323,8 +345,35 @@ func TestLoadConfigSaysNothingForAMarkerWithNoSettings(t *testing.T) { } _ = root.FS.Close() }) - if strings.Contains(out, "project file") { - t.Errorf("debug output = %q, want nothing about the project file", out) + if out != "" { + t.Errorf("Discover printed %q, want nothing", out) + } +} + +// Under --root every starting directory maps to one Root, and it must be built +// once: a second FromPath means a second os.OpenRoot and a second read of the +// project file for a root that cannot differ. +func TestCacheBuildsTheOverrideRootOnce(t *testing.T) { + dir := write(t, "space: ENG\n") + c := NewCache(dir) + defer c.Close() + + var first *Root + for _, start := range []string{dir, filepath.Join(dir, "a"), filepath.Join(dir, "b", "c")} { + root, err := c.Resolve(start) + if err != nil { + t.Fatalf("Resolve(%s): %v", start, err) + } + if first == nil { + first = root + continue + } + if root != first { + t.Errorf("Resolve(%s) built a second Root; want the one already built", start) + } + } + if got := c.resolved(); len(got) != 1 { + t.Errorf("resolved() = %d roots, want 1", len(got)) } } @@ -350,3 +399,27 @@ func captureStderr(t *testing.T, fn func()) string { _ = r.Close() return out } + +// A leading BOM is not a setting name. Without stripping it, a file a Windows +// editor wrote reports an unknown setting whose name starts with U+FEFF and +// advises upgrading markfluence -- the wrong remedy for the wrong problem. +func TestLoadConfigStripsALeadingBOM(t *testing.T) { + dir := write(t, "\ufeffspace: ENG\npage_width: wide\n") + root, err := Discover(dir) + if err != nil { + t.Fatalf("Discover: %v", err) + } + defer func() { _ = root.FS.Close() }() + if root.Config.Space != "ENG" || root.Config.PageWidth != "wide" { + t.Errorf("Config = %#v, want space=ENG page_width=wide", root.Config) + } +} + +// Only at the start, and only one: a BOM anywhere else is content markfluence +// must not silently discard. +func TestLoadConfigDoesNotStripABOMElsewhere(t *testing.T) { + dir := write(t, "space: ENG\n\ufeffpage_width: wide\n") + if _, err := Discover(dir); err == nil { + t.Fatal("Discover accepted a mid-file BOM, want an error") + } +} From 70bddca081d10443edaafa7d60fc6cce60619361 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:32:32 -0400 Subject: [PATCH 13/17] fix(check): do not fail a file that overrides the project's page_width The lint reported an invalid project-wide page_width as Broken for every file under the root -- including a file declaring its own width, which wins, so update publishes it perfectly well. The justifying comment ("fails every publish under this root") was false against the very resolvers the lint previews, and CLAUDE.md's rule for check is that a false positive is worse than a miss. Now reported only for a file whose own frontmatter declares no width, which is exactly the set that would use the project default. TestRunProjectDefectIsScopedToItsOwnRoot was also trivially true: it named only the good project, so nothing would touch the bad tree under any implementation. It now names both files in one run and asserts exactly one fails and that the good file is never blamed for the other project's width. --- cmd/check/check.go | 24 +++++++++++++++------ cmd/check/check_test.go | 47 +++++++++++++++++++++++++++++++++++------ 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/cmd/check/check.go b/cmd/check/check.go index 726ce6f..1bd5ab9 100644 --- a/cmd/check/check.go +++ b/cmd/check/check.go @@ -9,6 +9,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/mozilla/markfluence/internal/buildinfo" "github.com/mozilla/markfluence/internal/completion" @@ -90,6 +91,10 @@ func run(cmd *cobra.Command, args []string) error { for _, dir := range roots.Roots() { ui.Info("root: " + dir) } + // Under --debug only, and beside the root it belongs to: a project-wide + // default takes effect for a file that says nothing about it, so it has no + // answer anywhere in the file a reader would open. + project.ReportSettings(roots) if ui.IsJSON() { items := make([]any, len(results)) @@ -175,9 +180,12 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache // applying once both verbs agree. An absent title stays unreported: update // accepts it and keeps the live page's title. var localBroken []string - // A project-wide page_width Confluence does not accept fails every publish - // under this root, so it is Broken rather than a warning -- the same - // severity an invalid frontmatter page_width gets, for the same reason. + // A project-wide page_width Confluence does not accept, reported only for a + // file that would actually use it -- one whose own frontmatter declares no + // width. A file that declares its own wins over the project file (the + // chain is flag > frontmatter > project file), so reporting the project's + // bad value there would fail a file that publishes perfectly well, and + // check's rule is that a false positive is worse than a miss. // // This is where a project-wide width is validated offline at all: // internal/project cannot check its own value, since it would have to @@ -185,10 +193,12 @@ func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache // *project.Cache. check is the one verb that can find it without // publishing. // - // Reported on each file under that root rather than once for the run, which - // is what keeps every diagnostic scoped to the files actually named: a file - // under a different project hears nothing about this one. - if root.Config.PageWidth != "" { + // Broken rather than a warning, matching an invalid frontmatter page_width: + // for the files it is reported on, the publish really would fail. And + // reported per file rather than once for the run, which is what keeps every + // diagnostic scoped to the files actually named -- a file under a different + // project hears nothing about this one. + if root.Config.PageWidth != "" && strings.TrimSpace(mf.Frontmatter["page_width"]) == "" { if _, err := pagewidth.Declared( map[string]string{"page_width": root.Config.PageWidth}); err != nil { localBroken = append(localBroken, fmt.Sprintf("%s: %s", root.File, err)) diff --git a/cmd/check/check_test.go b/cmd/check/check_test.go index 792e431..5211d8e 100644 --- a/cmd/check/check_test.go +++ b/cmd/check/check_test.go @@ -580,19 +580,52 @@ func TestRunMalformedProjectFileIsAValidationFailure(t *testing.T) { } // A project file under a *different* root says nothing about a file checked -// elsewhere: diagnostics stay scoped to the files actually named. +// elsewhere: diagnostics stay scoped to the file they apply to. Both files are +// named in one run so the scoping is actually exercised -- naming only the good +// one would pass against any implementation, since nothing would touch the bad +// tree at all. func TestRunProjectDefectIsScopedToItsOwnRoot(t *testing.T) { base := t.TempDir() bad := filepath.Join(base, "bad") good := filepath.Join(base, "good") write(t, filepath.Join(bad, "markfluence.yaml"), "page_width: huge\n") - write(t, filepath.Join(bad, "main.md"), "---\ntitle: Bad\npage_id: 1\n---\n# Bad\n") + write(t, filepath.Join(bad, "bad.md"), "---\ntitle: Bad\npage_id: 1\n---\n# Bad\n") write(t, filepath.Join(good, "markfluence.yaml"), "page_width: wide\n") - write(t, filepath.Join(good, "main.md"), "---\ntitle: Good\npage_id: 2\n---\n# Good\n") + write(t, filepath.Join(good, "good.md"), "---\ntitle: Good\npage_id: 2\n---\n# Good\n") - if _, err := captureOutput(t, func() error { - return run(testCmd(t, ""), []string{filepath.Join(good, "main.md")}) - }); err != nil { - t.Fatalf("checking the good project = %v, want success", err) + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{ + filepath.Join(good, "good.md"), filepath.Join(bad, "bad.md")}) + }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error (bad.md is broken)", err) + } + if !strings.Contains(out, "1 of 2 file(s) failed") { + t.Errorf("output = %q, want exactly one of the two files to fail", out) + } + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "good.md") && strings.Contains(line, "invalid page_width") { + t.Errorf("good.md was blamed for the other project's width: %q", line) + } + } +} + +// A file declaring its own page_width wins over the project file, so the +// project's bad value must not fail it: check's rule is that a false positive +// is worse than a miss (CLAUDE.md), and update would publish this file fine. +func TestRunInvalidProjectPageWidthIsNotReportedForAFileThatOverridesIt(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "markfluence.yaml"), "page_width: huge\n") + write(t, filepath.Join(dir, "main.md"), + "---\ntitle: Main\npage_id: 1\npage_width: wide\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: the file declares its own width", err) + } + if strings.Contains(out, "invalid page_width") { + t.Errorf("output = %q, want no complaint: the file's own width wins", out) } } From f3ccd97b0a7ad0d7c2dc7e114a3447863d96ab5a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:32:47 -0400 Subject: [PATCH 14/17] refactor(project): report settings from the command layer, not from the load ui.Debug inside loadConfig described the wrong root and fired for commands that read no settings. Loading happens once per root for two unrelated reasons -- a markdown file's root, and the separate walk from the working directory that only locates .env -- so `info --debug` announced a project file it never consulted, and create described cwd's root rather than any file's. project.ReportSettings(cache) is now an explicit call beside where create, update and check already print `root:`, so the roots it describes are the ones the command actually used. Loading prints nothing, which a test pins. Two more things the review turned up in the same area: --root re-read the project file once per starting directory. Cache held the override Root under abs only, so each new directory re-ran FromPath -- a second os.OpenRoot, true before this branch, and now a second file read for a root that cannot differ. Cache.overrideRoot builds it once, and Close dedupes by *Root so a shared handle is not closed N times. A leading UTF-8 BOM was read as part of the first setting's name, so a file a Windows editor wrote reported an unknown setting and advised upgrading markfluence -- the wrong remedy for the wrong problem. Stripped, at the start of the file only: anywhere else it is content markfluence must not silently discard. --- cmd/create/create.go | 4 ++++ cmd/update/update.go | 4 ++++ internal/project/cache.go | 44 +++++++++++++++++++++++++++++++------- internal/project/config.go | 42 +++++++++++++++++++++++++++--------- 4 files changed, 76 insertions(+), 18 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 0355796..8c97c86 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -373,6 +373,10 @@ func run(cmd *cobra.Command, args []string) error { for _, dir := range roots.Roots() { ui.Info("root: " + dir) } + // Under --debug only, and beside the root it belongs to: a project-wide + // default takes effect for a file that says nothing about it, so it has no + // answer anywhere in the file a reader would open. + project.ReportSettings(roots) var ordered []record if len(errs) == 0 { diff --git a/cmd/update/update.go b/cmd/update/update.go index 7c732ec..c5f8fd0 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -140,6 +140,10 @@ func run(cmd *cobra.Command, args []string) error { for _, dir := range roots.Roots() { ui.Info("root: " + dir) } + // Under --debug only, and beside the root it belongs to: a project-wide + // default takes effect for a file that says nothing about it, so it has no + // answer anywhere in the file a reader would open. + project.ReportSettings(roots) if ui.IsJSON() { items := make([]any, len(results)) diff --git a/internal/project/cache.go b/internal/project/cache.go index 5b6b06f..bc36ef2 100644 --- a/internal/project/cache.go +++ b/internal/project/cache.go @@ -14,7 +14,12 @@ import ( // Not safe for concurrent use. type Cache struct { override string - byDir map[string]*Root + // overrideRoot is the one Root --root resolves to, built on first use. + // Without it, every distinct starting directory re-ran FromPath: a second + // os.OpenRoot, and -- since the project file is parsed -- a second read of + // it, for a root that cannot differ. + overrideRoot *Root + byDir map[string]*Root } // NewCache builds a Cache that applies override -- --root's value, or "" when @@ -25,7 +30,7 @@ func NewCache(override string) *Cache { // Resolve returns the root for startDir, discovering (or applying the // override) only the first time a given directory is seen. With an override, -// every startDir maps to the same *Root, opened once. With no override, the +// every startDir maps to the same *Root, built exactly once (overrideRoot). With no override, the // walk up from startDir consults the cache at every level (walkAndCache) so a // batch spanning many subdirectories of one project pays for Discover's walk // -- and os.OpenRoot -- once for the whole subtree, not once per distinct @@ -40,12 +45,15 @@ func (c *Cache) Resolve(startDir string) (*Root, error) { return root, nil } if c.override != "" { - root, err := FromPath(c.override) - if err != nil { - return nil, err + if c.overrideRoot == nil { + root, err := FromPath(c.override) + if err != nil { + return nil, err + } + c.overrideRoot = root } - c.byDir[abs] = root - return root, nil + c.byDir[abs] = c.overrideRoot + return c.overrideRoot, nil } return c.walkAndCache(abs) } @@ -120,8 +128,28 @@ func (c *Cache) Roots() []string { // Resolve call is done -- a root can be reused across many files, so nothing // closes it until the whole cache does. func (c *Cache) Close() { - for _, root := range c.byDir { + // Distinct Roots, not every byDir entry: under --root many directories map + // to one Root, and closing its handle once per entry would close an + // already-closed handle N-1 times. + for _, root := range c.resolved() { _ = root.FS.Close() } clear(c.byDir) + c.overrideRoot = nil +} + +// resolved returns every distinct *Root this cache holds, ordered by Dir, for +// reporting. Distinct by identity rather than by Dir: two Roots for one +// directory would be a bug, and collapsing them would hide it. +func (c *Cache) resolved() []*Root { + seen := map[*Root]bool{} + out := []*Root{} + for _, root := range c.byDir { + if !seen[root] { + seen[root] = true + out = append(out, root) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].Dir < out[j].Dir }) + return out } diff --git a/internal/project/config.go b/internal/project/config.go index 0699bc2..4a66511 100644 --- a/internal/project/config.go +++ b/internal/project/config.go @@ -102,7 +102,12 @@ func loadConfig(path string) (Config, error) { // established, which is the case that must not be guessed at. return Config{}, &ConfigError{File: path, Err: errors.New(readFailure(err))} } - items, err := dialect.ReadMapping(string(data)) + // A leading BOM is not a setting name. Without stripping it, a file a + // Windows editor wrote reports an unknown setting whose name begins U+FEFF, advising + // upgrading markfluence, which is the wrong remedy for the wrong problem. + // Only at the start of the file, and only one: anywhere else it really is + // content markfluence should not silently discard. + items, err := dialect.ReadMapping(strings.TrimPrefix(string(data), "\ufeff")) if err != nil { return Config{}, &ConfigError{File: path, Err: err} } @@ -128,15 +133,6 @@ func loadConfig(path string) (Config, error) { cfg.PageWidth = value } } - // A project-wide default is invisible by construction: it takes effect for - // a file that says nothing about it, so "why did this publish to ENG?" has - // no answer in the file the reader is looking at. --debug is where that - // answer goes. Only a file that declares something is worth a line; the - // marker that ships declares nothing, and a line for every root in a batch - // would be noise. The root itself is already reported unconditionally. - if settings := cfg.declared(); len(settings) > 0 { - ui.Debug(fmt.Sprintf("project file %s: %s", path, strings.Join(settings, ", "))) - } return cfg, nil } @@ -152,6 +148,32 @@ func (c Config) declared() []string { return out } +// ReportSettings logs, under --debug, the settings every project file this +// cache resolved declares. +// +// A project-wide default is invisible by construction: it takes effect for a +// file that says nothing about it, so "why did this publish to ENG?" has no +// answer in the file the reader is looking at. This is where that answer goes. +// +// It is an explicit call from the command layer rather than a side effect of +// loading, and that placement is the point. Loading happens once per root for +// two unrelated reasons -- a markdown file's root, and the separate walk from +// the working directory that only locates .env -- so printing during a load +// described whichever root came first and fired for commands that read no +// settings at all (`info`, `search`). A command calls this when it has a cache +// whose roots are the ones it actually used, beside where it already reports +// `root:`. +// +// Only a file declaring something earns a line: the marker that ships declares +// nothing, and a line per root in a batch would be noise. +func ReportSettings(c *Cache) { + for _, root := range c.resolved() { + if settings := root.Config.declared(); len(settings) > 0 { + ui.Debug(fmt.Sprintf("project file %s: %s", root.File, strings.Join(settings, ", "))) + } + } +} + // unknownSetting is the message #100 exists for. One line, because it lands // verbatim in check --json. func unknownSetting(key string) error { From 2ac266875c4d454ef1abc8f45ad0024f5720fbcd Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:32:47 -0400 Subject: [PATCH 15/17] fix(attachment-upload): report a malformed project file as a local defect It still wrapped the failure as "resolving the documentation root" and coded it IO, where create, update and check all use project.RootError and VALIDATION -- contradicting the CLAUDE.md bullet this branch added, which says those helpers exist so a caller reports a markfluence.yaml the author can open and fix as a local defect rather than sending them to look for a disk fault. --- cmd/attachmentupload/attachmentupload.go | 9 ++++++- cmd/attachmentupload/attachmentupload_test.go | 27 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/cmd/attachmentupload/attachmentupload.go b/cmd/attachmentupload/attachmentupload.go index bbec439..f8c131a 100644 --- a/cmd/attachmentupload/attachmentupload.go +++ b/cmd/attachmentupload/attachmentupload.go @@ -263,7 +263,14 @@ func rootRelativeSource(f string, roots *project.Cache) (string, error) { } root, err := roots.Resolve(filepath.Dir(abs)) if err != nil { - return "", fmt.Errorf("resolving the documentation root: %w", err) + if project.IsConfigError(err) { + // A markfluence.yaml that cannot be understood is a local defect in + // a file the author can open and fix, so it travels as badInput and + // is reported VALIDATION rather than IO -- matching create, update + // and check. + return "", badInput{err} + } + return "", project.RootError(err) } rel, err := filepath.Rel(root.Dir, abs) if err != nil { diff --git a/cmd/attachmentupload/attachmentupload_test.go b/cmd/attachmentupload/attachmentupload_test.go index cee3017..8510004 100644 --- a/cmd/attachmentupload/attachmentupload_test.go +++ b/cmd/attachmentupload/attachmentupload_test.go @@ -320,3 +320,30 @@ func TestPlanFailureCodeSeparatesServerFromLocal(t *testing.T) { }) } } + +// A markfluence.yaml that cannot be understood is a local defect in a file the +// author can open and fix, so it is reported VALIDATION rather than IO -- +// matching create, update and check, and the CLAUDE.md bullet that says those +// helpers exist for exactly that. +func TestLocalAttachmentsReportsAMalformedProjectFileAsValidation(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, project.Filename), + []byte("spce: ENG\n"), 0o644); err != nil { + t.Fatal(err) + } + file := writeFile(t, root, "assets/x.png") + + _, err := localAttachments([]string{file}, "", project.NewCache("")) + if err == nil { + t.Fatal("localAttachments succeeded with a malformed project file, want an error") + } + if got := localAttachmentsCode(err); got != jsonout.CodeValidation { + t.Errorf("code = %q, want VALIDATION", got) + } + if strings.Contains(err.Error(), "resolving the documentation root") { + t.Errorf("error = %q, want no root-resolution heading: the root was found", err) + } + if !strings.Contains(err.Error(), `unknown setting "spce"`) { + t.Errorf("error = %q, want the unknown-setting message", err) + } +} From 8eda72928b433bd89147f2fa1790980aefadbabc Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:33:02 -0400 Subject: [PATCH 16/17] test(update): pin the project-wide width on the wire Nothing asserted that a project-wide page_width makes update actually *request* the width -- only resolveWidth's apply bool, which is one layer above where the behavior change lives. Three tests now assert on the content- property traffic: two writes for a file declaring none (both the published and draft appearance properties, or the reader and the editor disagree), no requests at all when the project declares no width, and the file's own value on the wire when it declares one. Sabotage-checked: flipping apply to false in resolveWidth's project branch fails the first of them. --- cmd/update/update_test.go | 129 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index ea0d188..1945ffd 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -763,3 +763,132 @@ func TestProcessFileDoesNotWarnAboutAResolvableMention(t *testing.T) { t.Errorf("published body = %q, want a mention", published) } } + +// --- project-wide settings ---------------------------------------------------- + +// propertyServer answers a publish plus any content-property traffic, recording +// every property path so a test can assert on a request that was *not* made. +// Width lives in two content properties (docs/confluence/page-width.md). +func propertyServer(t *testing.T, paths *[]string) *client.ConfluenceClient { + t.Helper() + return clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "properties") { + *paths = append(*paths, r.Method+" "+r.URL.Path) + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"results":[]}`)) + return + } + _, _ = w.Write([]byte(`{"id":"p1","version":{"number":1}}`)) + return + } + switch r.Method { + case http.MethodGet: + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2020-01-01T00:00:00Z"))) + case http.MethodPut: + _, _ = w.Write([]byte(pageWithVersion("1", 4, "2026-01-01T00:00:00Z"))) + default: + t.Errorf("unexpected method: %s %s", r.Method, r.URL.Path) + } + }) +} + +// writeProject writes a markfluence.yaml and a markdown file under one root, +// returning the file's path. +func writeProject(t *testing.T, projectFile, md string) string { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, project.Filename), []byte(projectFile), 0o644); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, "f.md") + if err := os.WriteFile(path, []byte(md), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// The behavior change #100 makes to update, asserted on the wire rather than +// through resolveWidth's bool: a project-wide page_width makes update write the +// width for a file that declares none, where before it made no width request. +func TestProcessFileAppliesProjectWidth(t *testing.T) { + var paths []string + c := propertyServer(t, &paths) + path := writeProject(t, "page_width: narrow\n", "---\npage_id: 1\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache(), pagedoc.NewUserCache()) + if !r.ok { + t.Fatalf("result not ok: %+v", r) + } + if len(paths) == 0 { + t.Fatal("no content-property requests: the project width was not applied") + } + writes := 0 + for _, p := range paths { + if strings.HasPrefix(p, http.MethodPost) || strings.HasPrefix(p, http.MethodPut) { + writes++ + } + } + // Both the published and draft appearance properties, or the reader and the + // editor disagree about the width. + if writes != 2 { + t.Errorf("property writes = %d (%v), want 2", writes, paths) + } +} + +// The escape hatch, asserted the same way: a project that declares no width +// makes no width *request* at all, which is what keeps "absent means untouched" +// a property rather than an implementation detail. +func TestProcessFileNoProjectWidthMakesNoWidthRequest(t *testing.T) { + var paths []string + c := propertyServer(t, &paths) + path := writeProject(t, "space: ENG\n", "---\npage_id: 1\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache(), pagedoc.NewUserCache()) + if !r.ok { + t.Fatalf("result not ok: %+v", r) + } + if len(paths) != 0 { + t.Errorf("content-property requests = %v, want none", paths) + } +} + +// A file declaring its own width wins over the project file, and the value on +// the wire is the file's. +func TestProcessFileFrontmatterWidthBeatsProjectWidth(t *testing.T) { + var bodies []string + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "properties") { + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"results":[]}`)) + return + } + buf := make([]byte, 512) + n, _ := r.Body.Read(buf) + bodies = append(bodies, string(buf[:n])) + _, _ = w.Write([]byte(`{"id":"p1","version":{"number":1}}`)) + return + } + switch r.Method { + case http.MethodGet: + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2020-01-01T00:00:00Z"))) + default: + _, _ = w.Write([]byte(pageWithVersion("1", 4, "2026-01-01T00:00:00Z"))) + } + }) + path := writeProject(t, "page_width: narrow\n", + "---\npage_id: 1\npage_width: wide\n---\nHello.\n") + + if r := processFile(path, c, project.NewCache(""), linkindex.NewCache(), + pagedoc.NewUserCache()); !r.ok { + t.Fatalf("result not ok: %+v", r) + } + if len(bodies) == 0 { + t.Fatal("no width written") + } + for _, b := range bodies { + // wide -> "full-width"; narrow -> "default". + if !strings.Contains(b, "full-width") { + t.Errorf("property body = %q, want the file's own width (full-width)", b) + } + } +} From be9d66c96586a3c24c9824e0693b0907617fb936 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 12 Sep 2026 17:33:02 -0400 Subject: [PATCH 17/17] docs: correct two renamed identifiers and record the review CLAUDE.md and docs/guarantees.md still cited scalarValue and elementValue, which the dialect refactor renamed to reader.scalar and reader.element. CLAUDE.md's frontmatter bullet also gains the shared dialect: that Dialect.ReadMapping is how internal/project reads markfluence.yaml, and that a Dialect's nouns name a document and a *named* key only -- using Item for the word "key" in general is what silently rewrote a user-facing message, now pinned sentence-by-sentence. _plans/038 records all seven review findings with what each one broke, so the next change in this area starts from the evidence rather than from the claims. --- CLAUDE.md | 2 +- _plans/038_project-file-settings.md | 61 +++++++++++++++++++++++++++++ docs/guarantees.md | 2 +- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c1f8b07..b625fbb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -78,7 +78,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `internal/pageref` — `Resolve`, the single page-argument resolver: a numeric id, a Confluence page **or folder** URL (`pagePathRE` matches both `/pages/` and `/folder/`, since `children` takes a folder and a folder URL is what a browser hands you — the id is all it returns, so a command that can only use a page reports its own not-found), or a `.md` file whose frontmatter has a `page_id` (stat'd first, so `123.md` is a file). Every command taking a page uses it. `message.go` also owns the wording for the two ways a *frontmatter* `page_id` is wrong — `NotFoundMessage` (caller supplies the remedy, which differs per command) and `NotNumericMessage` — because `create`, `update`, and `fix` all report them and a reader should recognize the same problem across all three. They return strings, not errors: `create` wraps the text in its typed `pageIDFailure` (which also carries the `--json` fields), the others want a plain error. Anything checking a `page_id` before a request uses `IsDigits`, since the API answers a non-numeric id with a 400 whose body says nothing useful. - `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). A **folder** — the Cloud content type that can parent a page — has its own v2 route, `GetFolderOrNil` against `/wiki/api/v2/folders/{id}`, because every v2 *page* route answers a folder id with 404; enumerating children, if it is ever added, must be v1, since v2 cannot list inside a folder at all and its page-children route silently omits folders ([docs/confluence/folders.md](docs/confluence/folders.md)). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send`. `HTTPError.Error()` appends a **hint** for the three auth failures whose status misleads, matched on the response *body* rather than deduced from the status and always **appended** to it, never replacing it. The one that matters: **a rejected credential is a 404 on every v2 route**, so a revoked token used to make `read` answer `page ... not found` about a page that exists. `RejectedCredential` tells it apart by the fact that every genuine v2 404 *names* what it could not find and the auth one does not, `notFound` gates the three `…OrNil` helpers on it so they stop reading it as "absent", and `jsonout.CodeFor` checks it before the status switch so `--json` reports `AUTH` rather than `NOT_FOUND`. **Two error types on the request path, and one predicate for them**: an `*HTTPError` once a response has a status, an unexported `requestError` when there is none (a transport failure, a request that would not build, a body that would not decode), and `FromRequest` answers whether an error is either. That is what lets a caller tell a server failure from a local one — `jsonout.CodeOr(err, fallback)` is the whole point of it, since `CodeFor` alone reports every non-`HTTPError` as `NETWORK` and so turns `no title given` into a network problem (#133). The rule is deliberately scoped to the request: `DownloadAttachment` writing to the caller's writer, `uploadAttachment` opening the caller's file, and `Resolve` reading the environment stay untyped, because tagging them would misreport an unreadable file as a network failure. The wrapper carries no message of its own, so `Error()` is the inner text verbatim and nothing a reader sees changed. A 403 that is not one of the two measured credential phrasings gets no hint, because that is what a genuine permission denial looks like ([docs/confluence/api.md](docs/confluence/api.md#scopes)). **Retry rules**: 429 for any method; 502/503/504 for idempotent methods; **any other 5xx only when the response carries `Retry-After`** — that is how a 500 becomes retryable, and it is why `parseRetryAfter` reports the header's *presence* apart from its delay (`Retry-After: 0` means "retry now", not "no header"). The exponential delay is jittered, a server-supplied `Retry-After` never is. Decisions go to a package-level hook (`SetRetryLogger`, set once in `root.go` beside `ui.SetDebug`) and fire whichever way they went, because `internal/client` prints nothing and a silent twelve-minute retry storm is otherwise indistinguishable from a hang. **A versioned PUT is not as idempotent as its method**: `SetContentProperty` retry-once on top (recovers a lost create-POST response) and `UpdatePage`'s `updateLanded` both exist for the same reason — a write whose response was lost gets re-sent, and the re-sent version is refused. `updateLanded` requires version *and* title *and* body to match what was sent, since a concurrent edit could have produced the version alone and claiming success over someone else's content is worse than a false failure ([docs/confluence/api.md](docs/confluence/api.md)). `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; only the current comment form is parsed — an attachment stamped by a markfluence predating a comment-format change reads as unmanaged and is re-uploaded once, the same as any hand-uploaded file — except that a *recorded path disagreeing with the local source* is an update even when the checksum matches, so a mangled path repairs itself instead of surviving every later publish; a comment with no source recorded at all is not a disagreement. Every text part of the upload form must go through `writeTextField`, never `multipart.Writer.WriteField`, which emits no charset and gets decoded as Latin-1), `_links.next` pagination. **Three pagination schemes, and picking the wrong one truncates silently.** v1 *child/attachment* collections page through the generic `listV1` helper by `start`/`limit` offset, never `_links.next` (absent when the results fit one page, so it cannot terminate a loop); `ListAttachments`, `ListChildPages`, and `ListChildFolders` all go through it. v2 collections page through `listV2` by the cursor in `_links.next`, which is a `/wiki`-prefixed absolute path `resolveNext` handles unchanged; `ListContentProperties` and `SearchPagesByTitle` share it. **`/wiki/rest/api/search` is neither**: it ignores `start` outright, its `next` is context-relative so it needs the `/wiki` prefix `resolveNext` does not add, a short page does *not* mean the end, and `totalSize` can be nonzero against an empty `results` — so `searchCQL` terminates only on a missing `next` and nothing may branch on `totalSize` ([docs/confluence/search.md](docs/confluence/search.md)). `searchCQLBounded` adds a row bound under it (`SearchCQL` is that call with no bound, which is why `find` is unaffected): it asks for `max+1` and reports the surplus as `more`, since `totalSize` cannot supply a count. Full text goes through `SearchText`/`SearchRawCQL`, which return the cleaned `SearchMatch` the way `FindByTitle` returns `TitleMatch` — and **every field of a match comes from the row's `content` object**, because the row-level `title` is HTML-escaped *and* wrapped in `@@@hl@@@` markers where `content.title` is neither. The `excerpt` exists only at row level, so `cleanExcerpt` strips those markers, unescapes once, and collapses to one line — in the client, so the human and `--json` paths cannot disagree about it. `excerpt=highlight` is passed explicitly and **re-attached when following the cursor** (the `next` link carries `cql` and `limit` but not `excerpt`, and `doJSON` appends params with a bare `?`); an unrecognized value there yields an empty excerpt with a 200, so a rename by Atlassian degrades to no excerpts rather than an error. A row with no `content` object is skipped and **counted** — `type = space` answers with hundreds of them, and a silent skip would report a successful empty result. A bare v1 child row already carries `webui`, `status`, and `extensions.position`, so child listing needs no `expand`. `DownloadAttachment` goes through `send` (inheriting retry/backoff) against `_links.download`; **never** add a `CheckRedirect` that forwards headers — it would leak site credentials to Atlassian's media host, which neither needs nor wants them. `config.go` holds `Resolve` and the `.env` reader, plus the **`.env` permission warning** (#136): a `.env` reachable by anyone but its owner (`mode.Perm()&0o077`) *and* containing `CONFLUENCE_TOKEN` earns a warning naming the file, its mode, and the `chmod`. Both halves matter — a `.env` holding only the URL and username leaks nothing, and a warning that fires on a file with no secret in it is how one becomes something people scroll past. It stats rather than lstats (a link's own `0777` would cry wolf over a `0600` target), lives in `loadDotenv` because that is the one function both the discovered `.env` and `--env-file` pass through, and reaches the reader through `SetSecurityWarner` for the same reason `SetRetryLogger` exists — wired to `cmd/root.go`'s `reportSecurityWarning`, which prints it (human mode) *and* records it via `jsonout.AddWarning`, since stderr under `--json` is a schema-validated document with no room for a stray line. A group/world-*writable* `.env` with no token in it is knowingly **not** covered, though `CONFLUENCE_URL` resolves from there too and rewriting it would redirect the token: see #136. Why each of these is shaped this way, with the evidence: [docs/confluence/api.md](docs/confluence/api.md) and [attachments.md](docs/confluence/attachments.md). - `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, root *project.Root, index *linkindex.Index, baseURL, spaceKey, version string) (*ConfluencePage, error)`. `root` bounds which images and parent references may be read (S1/S2) and is what an image's recorded `Source` is relative to; `index` is the tree-wide link/anchor index for `root` (`internal/linkindex.Build`), built once and shared across every file converted under it rather than rebuilt per conversion — both are discovered/built by the caller (`internal/project`/`internal/linkindex`), which is why this package stays client-free. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `mention.go` owns the user-mention mapping in both directions (#91): a mention is 80% of all `` usage, and it converts to `[@Display Name](https://home.atlassian.com/people/{accountId})`. Three things decide its shape, each measured rather than reasoned. **The URL is Atlassian Home, not the site** — Confluence's own renderer still emits `{site}/wiki/people/{id}`, which no longer resolves usefully in a browser, so a mention in markdown names *no site*, needs nothing from configuration, and is therefore recognisable by `check` with no client at all. **Matching is on the path, ignoring host and query**, because several spellings of one target circulate (the Home URL, the modal's `?cloudId=` copy, the `/o/{orgId}` redirect, both Confluence forms, root-relative) and none of `cloudId`/`ref`/the org segment identifies the person — only the id does, and `ri:user` stores nothing else. **The `@` on the link text is the marker**, load-bearing rather than decoration: the URL cannot tell "mention this person" from "link to their profile", so without it anyone writing the second would silently get the first. The account id is **not** pattern-validated (two shapes are live on one instance, so a pattern tight enough for one rejects the other) and `ri:local-id` is never emitted (a mention carrying only the id resolves to the same person, verified via ADF). `ConfluencePage.Mentions` reports the ids the *forward* direction emitted so the caller can warn about one that names nobody — the `Attachments` arrangement, and necessary because Confluence accepts any id and renders `@Unlicensed user` rather than failing, and the profile URL 200s either way. An unresolvable mention still renders as a link, `[@Unlicensed user](…)` — that wording mirrors Confluence because the only ids reaching it are the ones the page labels that way: a **deactivated account resolves normally** and keeps its name (measured across every mention on a real page — 18 of them, six departed, all 200, returning e.g. `Mark Reid (Deactivated)`), so a departed colleague never takes that branch. Name resolution is `pagedoc.UserCache`, a per-run cross-page cache, and the tri-state is the part to preserve: `client.LookupUser` separates a name from `ErrNoSuchUser` from an unaskable question, `StorageOptions.UserNames` carries that as name / `""` / absent, and only a **confirmed** absence renders the placeholder. Flattening those would write a fabricated name over a real one the moment a VPN dropped mid-export, across a whole tree, into a file that then looks authoritative — which is also why the cache remembers a 404 but not a timeout (one is an answer, the other is not) and why `MentionWarnings` warns only about a confirmed absence; `aclink.go` is the *inverse* direction's one element with enough shape to need its own file — ``, which the editor writes for every internal link and `MdToConfluence` never emits, so nothing in the regression suite covers it. One rule decides its whole mapping: **convert when the markdown republishes to a link resolving to the same target, pass the storage through when it would not** — so a page link and a space link convert, while a mention and a space link convert, and an attachment link (only images are uploaded, so a relative href would be dead) and an unresolvable target stay raw, which the shield republishes byte-identical. A page target is a **title, never an id**, so `PageLinkTargets` reports what needs resolving and `StorageOptions.PageLinks` carries the answers back. An `ac:anchor` is **percent-encoded** where `confluenceSlug` output is not: decode it before matching a heading, leave it encoded inside a URL. A same-page anchor recovers its heading from the document rather than inverting the slug, which is impossible — `confluenceSlug` turns both a space and a hyphen into `-`. The survey the mapping rests on, and the `xml.HTMLAutoClose` trap that made `` crash the parser outright (#88), are in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); Plain text used as a markdown link's text goes through `escapeLinkText` (`\`, `[`, `]`), applied to the *raw* sources only — a page title, a space key, an anchor, a display name — and via `inlineTextForLink` to a body whose every descendant is a text node. Never to already-rendered output: an `ac:link-body` holding markup has been converted to markdown already, and escaping it yields a literal `\*\*bold\*\*`. Both directions are tested, because a fix at either extreme passes one and fails the other. `attachname.go` owns the source-path→attachment-name mapping, which is now the path's **base name** and nothing else (#59/`_plans/029`): the name is the attachment's identity, so an encoded path moved the name every time the file moved and orphaned the old attachment, and the path is recorded in the comment anyway. The mapping is therefore lossy, and what the bijection used to buy is an explicit refusal — two assets in one document whose base names agree return a typed `NameCollisionError` from `MdToConfluence`, which is a *failure* and not a `Broken` entry, since nothing blocks a publish on `Broken`. `check` catches that error and reports it as `Broken` anyway, because there it is a document defect like a dead link rather than a converter failure. A stored name is never interpreted in the other direction either: `sourceFor` reads the recorded path or uses the name verbatim. What names Confluence accepts is in [docs/confluence/attachments.md](docs/confluence/attachments.md); `destination.go` owns the **other** codec, destination↔path (`decodeDestination`/`encodeDestination`), shared by images *and* doc links — a markdown destination is a URL, so decode inbound (**before** `withinRoot`, or an encoded `..%2F` slips the clamp) and encode outbound in `storage_to_md.go` (or `export` emits markdown that no longer parses, and `sourceFor`'s absolute-path refusal is undone by the next read); an undecodable destination is a literal `%` in a filename, not an error; the reasoning is in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); `images.go` (resolution stays page-relative like GitHub; the documentation root — cwd — bounds what may be published, and an image above it is `IMAGE BROKEN`), `links.go` (GitHub/Confluence slugs, doc-link + anchor rewriting against `internal/linkindex`'s tree-wide index; `resolveDocKey` resolves a destination to the index's root-relative key and reports `escapes` — a purely lexical check on the *query* side, since the index itself needs no clamp: an escaping key can never be in it, built by walking downward from root). A doc-link target is one of four severities, #42: missing entirely or escaping root is **Broken** (`LINK BROKEN: … (not found|outside the documentation root)`) and replaces the whole `` element — tags and visible text alike — with that literal message, matching `images.go`'s precedent for a missing image (`renderLink` needs a small per-node flag, `linkBrokenText`, since goldmark still invokes a container node's renderer on the matching leaving call regardless of `WalkSkipChildren` on entering, and there is no `` to write in the broken case); existing on disk with no `page_id` yet is unchanged — a **warning**, the normal state of an unpublished tree; a `#fragment` matching no heading on an otherwise-resolving target also **warns**, gated on `linkindex.Index.FileExists` so a missing/escaping target isn't double-reported. `tables.go` (the `` tag, stamped with `data-layout="align-start"` so tables auto-size and left-align — this must stay if column widths are ever emitted, or a `` silently induces a layout; plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour`; `storage_to_md.go`'s `cellTexts` reverses this, reading `data-highlight-colour` back into a `bg:` marker (`cellBGNames`, the reverse of `tables.go`'s swatch map — a hex outside the 21 swatches round-trips as the literal hex, and where two names share a hex the British spelling wins, matching Confluence's own `-colour`), and a column's GFM alignment becomes a `

` wrapper **inside** the cell — never the `align` attribute the GFM renderer would emit, which is the one form Confluence discards. Only center and right are emitted: Confluence has no explicit left, so `:---` publishes bare and `read` recovers it as `---`. Since alignment is per-paragraph there and per-column in GFM, `columnSeparators` in `storage_to_md.go` takes each column's most common declared alignment (ties to the first seen) and drops the rest. Rows still fall through to the GFM renderer. A multi-line cell is one `

` per line — Enter in the editor starts a new `

`, it does not insert a `
` — so `renderCellLines` in `storage_to_md.go` joins sibling `

` children with a literal `
` rather than nothing: a GFM table row is exactly one physical line, so a real newline isn't an option, and the same substitution catches a bare mid-line `
` (Shift+Enter) that would otherwise render as the two-space hard break valid in ordinary block content but not inside a table row. A `