diff --git a/CLAUDE.md b/CLAUDE.md index e6cbdd5..78ce4bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,8 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd **Before changing anything that talks to Confluence, read [docs/confluence/](docs/confluence/)** — what we established by experiment, since Atlassian documents little of it. Two traps recorded there have each already produced a confident wrong conclusion: `body-format=view` is not what the browser renders, and `body.storage` proves only what was stored, never what takes effect. +**[docs/guarantees.md](docs/guarantees.md) holds the properties markfluence holds itself to** — safety (S1-S6), laws (L1-L8), conformance (C1), reporting (R1-R2). Each carries a status, because several are aspirational rather than true today: a spec or PR cites them by id to say what it changes. The ids are permanent and never reused, and a change that downgrades a status says so in the commit message and in that file rather than letting it be noticed later. + ### Layout - `cmd/root.go` — the cobra root: `--url`/`--username`/`--debug`/`--no-color` persistent flags, version from `internal/buildinfo`, and registration of every subcommand. `Execute()` prints cobra-generated errors (bad args/flags) but not `ui.ErrSilent`, which marks a failure a command already reported. @@ -60,7 +62,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `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. - `internal/pagetree` — `Walk`, the traversal of pages *and folders* under a node, plus `AllDepths`. It is a package rather than command-local because listing a subtree and exporting one (#59) need the identical walk, and its rules must not exist in two copies: siblings arrive from two requests (`/child/page`, `/child/folder`) and are **merged by `extensions.position`**, or the output loses the order Confluence displays; a folder **counts as a level** like a page, which is only reasonable because folders are reported rather than silently traversed; and the walk descends folders even when only pages matter, since a folder may hold the only pages in a subtree. `nodeURL` uses `SiteURL()` — a v1 child row carries `webui` but no `base`. A visited set guards the unbounded case. - `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`. 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; the legacy `mzcld:checksum:` form is still parsed, and the skip test compares the *parsed* checksum so a format change doesn't force a re-upload — 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; an absent path is a legacy comment, 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. 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/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`. 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. 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, baseURL, spaceKey, version string) (*ConfluencePage, error)`. 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; `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 (80% of all real usage), 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); `attachname.go` owns the source-path↔attachment-name mapping (percent-encoding `%`→`%25` then `/`→`%2F`, which is **bijective** — that is what makes the dedupe collision-free and lets `read` recover an image's original path; decode refuses an absolute result, which markfluence never produces; 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` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting; `docKey` is the single lookup key for both the page and anchor maps. Failure here is **silent** — an unresolved link publishes as a dead relative href with no warning), `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`, 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. Layout values, cell alignment, and colors: [docs/confluence/storage-format.md](docs/confluence/storage-format.md)), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. - `internal/frontmatter` — flat YAML frontmatter parse/quote/`UpdateField`, and the `MarkdownFile` type (`Parse`/`ParseFile`, exported `Filename`/`Content`/`Frontmatter`/`Body`, and `Title`/`PageID`/`Space`/`Parent` accessors that normalize missing/blank/`"null"`). - `internal/pagewidth` — the `page_width` `Width` enum (`narrow`/`wide`/`max`, default `max`), `Declared`, the vocab↔content-property maps, `WidthFromProperties`, and `Apply`/`Read` against the client. Width lives in two content properties and **both** must be written or the reader and the editor disagree: [docs/confluence/page-width.md](docs/confluence/page-width.md). diff --git a/README.md b/README.md index 0430c9b..1075da3 100644 --- a/README.md +++ b/README.md @@ -61,8 +61,13 @@ resolved with the precedence **flag > environment variable > `.env` file**: | API token | *(none — never a flag)* | `CONFLUENCE_TOKEN` | | Cloud ID *(optional)* | `--cloud-id` | `CONFLUENCE_CLOUD_ID` | -markfluence reads a `.env` file from the current directory automatically (no need -to `source` it), or from an explicit path via `--env-file PATH`. +markfluence reads a `.env` file automatically (no need to `source` it) from the +[documentation root](#the-documentation-root) — the directory holding +`markfluence.yaml`, found by walking up from the working directory, or the +working directory itself with no `markfluence.yaml` above it — or from an +explicit path via `--env-file PATH`. For `create`, `update`, and +`attachment-upload`, `--root PATH` redirects this too, the same as it does +the per-file root those commands otherwise resolve independently. Copy `.env.example` to `.env` and fill in: @@ -129,24 +134,22 @@ readonly:content.attachment:confluence read:confluence-content.summary ``` -> [!IMPORTANT] -> **The mixture of naming styles is correct, not a copy-paste error.** Atlassian -> has two scope vocabularies — *classic* (`read:confluence-user`) and *granular* -> (`read:page:confluence`) — and they are granted independently: holding one does -> **not** imply the other. markfluence talks to both API versions, and each -> version accepts only one vocabulary, so the list above is genuinely mixed. A -> token granted the classic names alone fails with -> `401 Unauthorized; scope does not match` on almost every command, which is what -> makes this an easy list to get wrong. The measurements behind that are in -> [docs/confluence/api.md](docs/confluence/api.md#scopes). - > [!NOTE] > Scopes are fixed when a token is issued. A missing one needs a **new** token, > not an edit to the existing one. -> [!NOTE] -> Currently, markfluence doesn't support deleting anything, so it doesn't need -> delete scopes. This might change in the future. +**The mixture of naming styles is correct, not a copy-paste error.** Atlassian +has two scope vocabularies — *classic* (`read:confluence-user`) and *granular* +(`read:page:confluence`) — and they are granted independently: holding one does +**not** imply the other. markfluence talks to both API versions, and each +version accepts only one vocabulary, so the list above is genuinely mixed. + +A token granted the classic names alone fails with +`401 Unauthorized; scope does not match` on almost every command, which is what +makes this an easy list to get wrong. The measurements behind that are in +[docs/confluence/api.md](docs/confluence/api.md#scopes). +If you get a `401 Unauthorized; scope does not match` error, you need additional +scopes. A **403** (rather than the 401 above) means the opposite problem: the token is scoped for the call, but the service account lacks Confluence permission on that @@ -252,7 +255,13 @@ written back (and the file won't record its new `page_id`). A whole tree can be created in one pass: give each child a `parent:` that points at its parent's `.md` file, and `create` orders creation parents-first and fills in the -real ids (see the `parent` field below). +real ids (see the `parent` field below). Creation is three-phase — every file is +validated (above), then a content-less stub is reserved for each, parents-first, +before any of them is converted — so a link from one file in the batch to another +resolves regardless of which direction it points, or whether the two link to each +other. A run interrupted after this point leaves a permanent, empty page version +behind rather than no page at all; every id is already persisted (unless +`--no-persist`), so a plain `update` finishes the job. `--dry-run` validates every file (the same checks a real run makes, so it exits non-zero on the same failures) and previews what would be created — pages, @@ -638,7 +647,8 @@ Usage: markfluence attachment-upload PAGE FILE... [flags] Upload or replace attachments on a page, complementing the automatic sync that `create` and `update` perform for a page's images. -Each file is attached under its base name. A file whose contents already match +Each file is attached under its path relative to the documentation root (its +base name, with no `markfluence.yaml` above it). A file whose contents already match what's on the page is skipped, using the same checksum bookkeeping `create`/`update` use, so uploading by hand and publishing agree on what's current. `--force` uploads anyway (bumping the attachment's version), which is @@ -1001,11 +1011,15 @@ view the file on GitHub, so a page in a subdirectory can share an asset directory above it: ``` -docs/ ← run markfluence from here +docs/ ← needs a markfluence.yaml here for this to work assets/logo.png guide/page.md → ![logo](../assets/logo.png) ``` +That layout needs a [documentation root](#the-documentation-root) declared at +`docs/` — without one, each page's root defaults to its own directory, and +`guide/page.md` reaching above itself for `assets/` is out of bounds. + > [!NOTE] > An image path is a URL, not a filename, so a space or other special character > has to be percent-encoded — `![shot](assets/my%20image.png)` for a file named @@ -1020,24 +1034,21 @@ docs/ ← run markfluence from here > `markfluence read` and `markfluence export` write the encoded form, so a page > round-trips back to Markdown that still renders. -**Run markfluence from the root of your documentation tree.** That root bounds -which images may be published: an image resolving outside it (`../../secrets/x.png`) -is reported as `IMAGE BROKEN: … (outside the documentation root)` rather than -uploaded. - -Confluence attachment names cannot contain `/`, so the path is percent-encoded -into the attachment name — `assets/logo.png` is attached as `assets%2Flogo.png`, -and `../assets/logo.png` as `..%2Fassets%2Flogo.png`. The encoding is reversible, -so `markfluence read` restores an image's original path instead of a flattened +Every image is bounded by the [documentation root](#the-documentation-root): +one resolving outside it (`../../secrets/x.png`) is reported as +`IMAGE BROKEN: … (outside the documentation root)` rather than uploaded, and a +symlink is refused even when it resolves inside the root. + +Confluence attachment names cannot contain `/`, so the path — relative to the +root, not to the page — is percent-encoded into the attachment name: +`assets/logo.png` referenced from a page at the root is attached as +`assets%2Flogo.png`; the same file referenced as `../assets/logo.png` from a +page one directory down is attached under the *same* name, since both +resolve to the same root-relative path. The encoding is reversible, so +`markfluence read` restores an image's original path instead of a flattened one. markfluence also records the source path in the attachment's comment, which it prefers over decoding the name. -> [!NOTE] -> Pages published before this encoding existed used `/` → `_`. Republishing such -> a page uploads the image under its new name and updates the page to match, but -> the old attachment stays behind, unreferenced — markfluence never deletes. -> Remove those manually if the clutter bothers you. - Extra properties ride in the title as JSON: ```markdown @@ -1071,8 +1082,9 @@ non-ASCII heading anchor may arrive as `#caf%C3%A9-section`. Both are decoded before markfluence matches them against files and headings on disk, so either spelling resolves. A link it cannot resolve — a target with no `page_id`, or a file that isn't there — is left exactly as written and published as-is, which on -Confluence is a dead relative link. There is no warning for this, so check the -targets when a link matters. +Confluence is a dead relative link. A `.md` link shaped like a same-tree +reference gets a warning when this happens; a mention, an attachment link, or +an external URL was never meant to resolve here and stays silent. **Comment directives:** - `` — replaced with Confluence table-of-contents macro. @@ -1113,6 +1125,66 @@ Right column. Storage markup shown inside a fenced code block stays literal (it isn't activated). +## The documentation root + +**Do you need a `markfluence.yaml`?** + +- If you export, edit, and publish files **one at a time**, no. Each file's + root defaults to its own directory, and that's already the directory you + want. +- If you're working on a **directory tree** of files — pages that link to + each other, or that share an `assets/` directory — put a + `markfluence.yaml` at the root of that tree. Without one, each file's root + still defaults to its own directory, which means a page can't reach an + image or another page sitting *above* itself; a shared-assets layout like + the one in [Images](#body) above needs a declared root to work at all. + +```yaml +# Marks the root of a markfluence project. Image and link paths are recorded +# relative to this directory. https://github.com/mozilla/markfluence +``` + +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 — +with no `markfluence.yaml` anywhere above it — the file's own directory. It +bounds which images and `parent:` references a file may read, and it's what +an image's recorded attachment name and source are relative to. The root +actually used is reported once per distinct value in a run. `--root PATH` +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 +[_plans/025_file-organization.md](_plans/025_file-organization.md). + +### Common tasks + +**Moving or renaming a markdown file.** Just move it. Links to it resolve by +where it actually is, via the root-relative link index — nothing elsewhere +needs editing, and nothing needs republishing except the moved file itself +(to pick up its own new links, if any changed). + +**Moving a page's own images along with it.** This churns: an attachment's +identity is relative to the *root*, not the page, so moving both together +changes the images' root-relative paths, and the next publish uploads them +under new names, leaving the originals behind unreferenced (markfluence never +deletes; [#99](https://github.com/mozilla/markfluence/issues/99) tracks a +future `attachment-prune`). Moving just the page and leaving its images in a +shared directory is the free move instead. + +**Renaming or moving a shared asset**, independent of any page, churns the +same way: every page referencing it records a new attachment name on its next +publish. Identity follows the asset's location, not any particular page's +(this is L3 in [docs/guarantees.md](docs/guarantees.md) — `identity-from-asset-location`). + +**Setting up a shared assets directory across many pages** needs a +`markfluence.yaml` at the directory that should be the shared root. Without +one, each page's root defaults to its own directory, and an asset above any +one of them is `IMAGE BROKEN` — the layout in [Images](#body) above needs +this to work at all. + ## Development Requires Go 1.25+. diff --git a/_plans/025_file-organization.md b/_plans/025_file-organization.md new file mode 100644 index 0000000..420a357 --- /dev/null +++ b/_plans/025_file-organization.md @@ -0,0 +1,1145 @@ +# Spec: file organization and how it round-trips + +How markdown on disk maps to pages and attachments in Confluence, and back. + +A spec rather than an implementation plan. It states the problem in terms of the +guarantees in [docs/guarantees.md](../docs/guarantees.md), lists the layouts +markfluence should support, then works through the model and what it costs, and +ends with the work it implies. + +Everything it raises is now decided. One thing is deliberately *not* fixed here +and says so where it comes up: **R1** (report-unresolved-references), which needs +a converter diagnostic rather than a path decision. + +## The problem + +**Eight guarantees are not true today.** Each is broken by a scenario a user +reaches without doing anything unusual. + +| | label | guarantee | status | scenario | +|---|---|---|---|---| +| **L1** | `resolve-what-was-named` | a reference resolves to the file it names, or nothing | Aspirational | A | +| **C1** | `preview-compatible-resolution` | references resolve the way a Markdown preview does | Partial | A | +| **L2** | `invocation-independent` | resolution and naming depend only on the files on disk | Aspirational | B | +| **L3** | `identity-from-asset-location` | an attachment's identity depends only on the asset's location | Aspirational | C | +| **L5** | `roundtrip-from-confluence` | export, then publish back unedited, changes nothing | Partial | D | +| **L6** | `roundtrip-from-disk` | publish, then export, republishes the same page | Partial | D | +| **R1** | `report-unresolved-references` | every unresolved reference is reported | Aspirational | E | +| **S2** | `no-read-outside-root` | no file is read outside the root | Aspirational | F | + +### Scenario A. A link to a file in a subdirectory + +Breaks **L1** and **C1**. Given this tree: + +``` +docs/ + index.md See the [setup overview](setup/overview.md) to get started. + guide.md Follow the [install steps](setup/install.md). + overview.md title: Product Overview page_id: 999 + setup/ + overview.md title: Setup Overview page_id: 777 + install.md title: Install Steps page_id: 888 +``` + +`index.md` links to `setup/overview.md`, which is page id **777**. Publishing it +produces a link to page id **999** instead: + +``` +index.md + +$ cd docs/team && markfluence update onboarding.md + → 0 attachments + →

IMAGE BROKEN: ../assets/logo.png (outside the documentation root)

+``` + +The second run publishes the sentence `IMAGE BROKEN: ../assets/logo.png (outside +the documentation root)` into the page body, where readers see it. `update` +prints it as a warning and publishes anyway. + +The cause is what bounds the publishable area. `MdToConfluence` calls +`os.Getwd()` and treats the working directory as the documentation root, then +refuses any image resolving above it. From `docs/`, the image is inside. From +`docs/team/`, the identical reference is outside. + +Two consequences follow from the root being the working directory rather than a +property of the tree. Attachment names shift with it — the first run's +`..%2Fassets%2Flogo.png` would have been `assets%2Flogo.png` had the root been +declared at `docs/` — which is Scenario C from a different angle. And the same +tree reached through a symlink is rejected outright, because `withinRoot` +compares lexically after `filepath.Abs` without resolving either side, so +`/docs` and `/private/docs` do not match. + +The rule is documented: the README says to run markfluence from the root of your +documentation tree. Nothing detects a violation, and the consequence is a +corrupted page rather than a refusal. + +### Scenario C. One shared image, referenced from two depths + +Breaks **L3**. This one happens at **publish** time, and nothing visibly breaks +when it does — it is the cause of Scenario D and of the churn in Use case 9, +which is why it is worth naming on its own. + +Given this tree, published from `docs/`: + +``` +docs/ + assets/brand.png + index.md ![brand](assets/brand.png) page_id: 100 + team/ + onboarding.md ![brand](../assets/brand.png) page_id: 200 +``` + +Both pages reference **the same file on disk**. Publishing them: + +``` +$ cd docs && markfluence update index.md team/onboarding.md +``` + +markfluence uploads the image once per page. A Confluence attachment belongs to +a page rather than to a space, so each page gets its own copy even though the two +share one file on disk. That duplication is inherent, and it is not the problem. + +The problem is what markfluence records on each copy. Two fields carry the path: + +- the **attachment name**, which is Confluence's `title` for the attachment. It + is what the page's attachment list shows, and what `ri:filename` in the page + body points at. +- the **recorded source**, which markfluence writes into the attachment's + *comment* field as `path=…` beside a checksum + (`attachmentComment` builds `markfluence: sha256= path=`). It is + the original markdown destination, and it is what `read` and `export` use to + put the image back where it came from. + +What the two pages end up with: + +``` +page 100 (from index.md) + attachment name assets%2Fbrand.png + attachment comment markfluence: sha256= path=assets/brand.png + body references + +page 200 (from team/onboarding.md) + attachment name ..%2Fassets%2Fbrand.png + attachment comment markfluence: sha256= path=../assets/brand.png + body references +``` + +Both are the reference **as the author wrote it** — percent-encoded for the name, +verbatim for the comment. The two authors wrote different relative paths to reach +the same file, so the file has two identities in Confluence. Both pages render +correctly, which is why this stays invisible until something depends on the +name. + +Two things depend on it. + +**Export.** `export --dest DIR` writes the page's markdown to `DIR/.md` +and each attachment to `DIR` joined with its **recorded source**. `--dest` +defaults to `.`, so `--dest out` run from `~/work` means the destination +directory is `~/work/out`. + +Joining the two recorded sources gives very different results: + +``` +page 100 path=assets/brand.png → out/assets/brand.png written +page 200 path=../assets/brand.png → assets/brand.png REFUSED +``` + +The second one is not inside `out` at all. `filepath.Join` cleans as it joins, so +`out` + `../assets/brand.png` collapses to `assets/brand.png` — a sibling of +`out`, in `~/work` itself. Writing there would drop a file outside the directory +the user named, so `attachfile.Resolve` refuses it: + +``` +attachment "..%2Fassets%2Fbrand.png" resolves to "assets/brand.png", +outside the destination directory +``` + +So the same image on disk is exportable from page 100 and not from page 200, +decided entirely by how the author of each page happened to spell the path. That +is Scenario D and we'll discuss it there. + +**Moving a file.** Move `index.md` from `docs/` into `docs/guides/` and its +reference has to become `../assets/brand.png` to still resolve. Republish it — +same page, `page_id` 100 is still in the frontmatter: + +``` +$ cd docs && markfluence update guides/index.md + → attachment name ..%2Fassets%2Fbrand.png (was assets%2Fbrand.png) + → attachment comment path=../assets/brand.png (was path=assets/brand.png) +``` + +That uploads a *second* attachment under the new name and leaves the original +behind unreferenced, because markfluence never deletes. Nothing about the page or +the image changed; only the markdown file's position did. + +The model fixes the *rename*: with the source recorded relative to the root, +moving a page does not change what its attachments are called, so there is +nothing to strand. It does not fix stranding in general, because an attachment's +identity does follow the **asset's** location by design (**L3**) — so moving or +renaming an asset still renames its attachment, as does removing an image from a +page. Cleaning those up is #99 (`attachment-prune`), which is a tool for the +residual rather than part of this model. + +Note that `team/onboarding.md` and `guides/index.md` agree, because they sit at +the same depth. Two pages only disagree when they are at different depths, which +is why this is easy to miss in a shallow tree and unavoidable in a deep one. + +`attachment-upload` diverges a third way: it records `filepath.Base(f)`, so +uploading `sub/img.png` by hand records `img.png`, while publishing a page that +references the same file records `sub/img.png`. + +### Scenario D. Exporting a page whose assets sit above it + +Breaks **L5** and **L6**. Publishing works; only the return trip fails. + +Start from the layout the README endorses for shared assets, and the one +`regression/images-shared-parent` pins with a golden — a page in a subdirectory +using an asset directory above it: + +``` +docs/ + assets/logo.png + team/ + onboarding.md ![company logo](../assets/logo.png) +``` + +Publish it, from the root of the tree so the asset is inside the documentation +root: + +``` +$ cd docs && markfluence create team/onboarding.md --space ENG +``` + +That succeeds. The page now carries one attachment, named +`..%2Fassets%2Flogo.png`, whose comment records `path=../assets/logo.png` — the +reference as written, per Scenario C. + +Now get it back. This is the direction where the original tree may not exist at +all: a colleague exporting a page they did not publish has only what Confluence +stores. + +``` +$ markfluence export https://.../pages/12345/Onboarding --dest out +``` + +`export` writes the markdown to `out/onboarding.md` and each attachment to `out` +joined with its recorded source. For this attachment that join is: + +``` +out + ../assets/logo.png → assets/logo.png +``` + +which is not inside `out` — `filepath.Join` cleans as it joins, so the result is +a *sibling* of `out`. `attachfile.Resolve` refuses to write outside the directory +the user named, so the export reports: + +``` + ✓ written out/onboarding.md + ✗ failed ..%2Fassets%2Flogo.png: attachment "..%2Fassets%2Flogo.png" resolves to + "assets/logo.png", outside the destination directory +``` + +and exits **1**, because `report` counts failed attachments and any failure is a +non-zero exit. The markdown is written; the image it references is not. The +exported page renders with a broken image, and there is no flag that changes +this — `--flat` on `attachment-download` opts out of recorded paths, but `export` +has no equivalent. + +So a layout markfluence documents, tests, and publishes correctly cannot be +round-tripped. That is why **L5** and **L6** are Partial rather than false: they +hold for every layout whose assets sit at or below the page, and fail entirely +for the one above it. + +The refusal itself is correct — it is **S1** (no-write-outside-root) doing its +job. The defect is +upstream: Scenario C recorded a path that cannot be honoured under `--dest`. +Fixing this belongs in naming, not in export. + +> **Verified in pieces, not end to end.** The converter recording +> `path=../assets/logo.png`, and `attachfile.Resolve` refusing exactly that +> string with exactly that message, are both measured. The export output above is +> assembled from `report` in `cmd/export/export.go` rather than observed on a +> live page, because the credential to hand lacks the scope to publish one. + +### Scenario E. Any unresolved reference + +Breaks **R1**. Images report what they could not resolve; links say nothing at +all. Same class of problem, two behaviours. + +``` +docs/ + guide.md (below) + draft.md title: Draft, and no page_id -- not published yet + notes.txt a text file, not an image +``` + +`guide.md` contains four references, none of which can resolve: + +```markdown +A [link to a missing file](nope.md). + +A [link to the draft](draft.md), which exists but has no page_id. + +An image that is missing: ![missing image](nope.png) + +An image of the wrong type: ![bad type](notes.txt) +``` + +Publishing it with `markfluence update guide.md` produces this body: + +``` +

A link to a missing file.

+

A link to the draft, which exists but has no page_id.

+

An image that is missing: IMAGE BROKEN: nope.png (not found)

+

An image of the wrong type: IMAGE BROKEN: notes.txt (unsupported type)

+``` + +and these diagnostics: + +``` +broken (2): IMAGE BROKEN: nope.png (not found) + IMAGE BROKEN: notes.txt (unsupported type) +warnings (0): +``` + +**The images behave well.** Both failures are named in `broken`, so `update` +prints them, and both are visible in the page as `IMAGE BROKEN: …` text, so a +reader can see something is wrong. Loud in both directions. + +**The links behave badly.** Both publish as `` and +`` — relative hrefs that are dead on Confluence, since nothing +resolves `nope.md` there. Neither appears in `broken` or in `warnings`. `update` +reports a successful publish, the page looks fine in a diff, and the links fail +only when somebody clicks one. + +The two link cases fail for different reasons, and both are silent. `nope.md` +does not exist. `draft.md` does exist, but has no `page_id`, so it is not in the +page index — which is the ordinary state of every page in a tree that has not +been published yet, and is why Scenario E and Use case 7 are related. + +The cause is one-sided reporting. `internal/convert/images.go` appends to +`r.broken` in three places and `r.warnings` in two. `internal/convert/links.go` +touches neither, ever. + +This is documented behaviour rather than an oversight. The README: + +> A link it cannot resolve — a target with no `page_id`, or a file that isn't +> there — is left exactly as written and published as-is, which on Confluence is +> a dead relative link. **There is no warning for this**, so check the targets +> when a link matters. + +R1 is the decision to stop saying that. Note what it does *not* require: leaving +the link as written is a reasonable thing to publish, and R1 does not ask for it +to be an error. It asks for it to be **said out loud** — which is why R1 is a +reporting requirement rather than a law. + +### Scenario F. Reading outside the root + +Breaks **S2**, and it is the one scenario where this spec's model makes things +worse before better. + +``` +/work/ + outside/ + secret.md title: Secret Outside The Root, and no page_id + linked.md title: Linked Outside, page_id: 555 + docs/ ← the documentation root + page.md parent: ../outside/secret.md + link.md A [link outside the root](../outside/linked.md). +``` + +`withinRoot` is applied to images and to nothing else, so the two references +above are treated very differently — and neither is treated the way S2 asks. + +#### A `parent:` path is read with no clamp + +`page.md` names a parent above the root. Publishing it: + +``` +$ cd docs && markfluence create page.md --space ENG +``` + +`resolveParent` joins the path onto the file's directory, stats it, and parses +its frontmatter, with no bound of any kind. The read happens before a client is +ever touched, so it is provable without a network call at all — calling +`resolveParent` with a `nil` client returns: + +``` +parent not yet published (no page_id): ../outside/secret.md +``` + +That error is only reachable by opening `/work/outside/secret.md` and finding no +`page_id` in it. The file outside the root was read. + +The exposure is genuinely small: only `page_id` and `title` are taken, and +nothing from that file is published. But S2 says no file is read outside the +root, and this reads one. + +#### A link cannot traverse, purely by accident + +`link.md` names a target above the root too, and nothing is read: + +``` +href: href="../outside/linked.md" +broken=[] warnings=[] +``` + +The reason is the lookup key, not a bound. `docKey("../outside/linked.md")` is +`"linked.md"`, which is looked up in an index of `docs/` alone. There is no +`linked.md` in `docs/`, so the link resolves to nothing and is published as +written. `page_id: 555` is never seen, because `/work/outside/` is never opened. + +Flattening every destination to its basename makes traversal impossible. That is +the same flattening that produces Scenario A's wrong-page links — the property +protecting S2 here is a side effect of the bug there. + +#### Which is why the model has to add a clamp + +Making resolution path-aware, so `../team/onboarding.md` finds the page it names, +necessarily also makes `../../../../etc/anything.md` a real path lookup. The +accident goes away with the bug. + +So S2 becomes work this spec creates rather than work it completes. What the model +does about it is in +[How S2 is enforced](#how-s2-no-read-outside-root-is-enforced): links need no +clamp at all once resolution is a lookup in an index built downward from the +root, and the two remaining reads — an image leaf and a `parent:` path — are +handled differently from each other. + +### What is actually going wrong + +Not one cause. Three kinds, and they are worth keeping apart, because choosing a +single convention fixes most of them and provably does not fix two. + +Letters throughout this section refer to the scenarios above. + +**Conventions that are wrong.** These work exactly as designed and still violate +a guarantee: + +| scenario | convention | violates | +|---|---|---| +| A | a link destination is keyed by its **basename**, looked up in one directory | L1, C1 | +| B | the root is the **working directory** | L2 | +| C | an attachment's identity is the reference **as written** | L3 | + +**Inconsistencies.** The same question answered differently in different places, +which is what happens when no single convention was chosen: + +| scenario | inconsistency | +|---|---| +| A | images resolve a destination as a *path*; links flatten it to a basename | +| C | `attachment-upload` records a basename; `update` records the path | +| D | publishing accepts an asset above the page; exporting refuses it | +| F | `withinRoot` guards images, and not links or `parent:` paths | + +**Bugs.** Behaviour contradicting its own stated contract: + +| scenario | bug | +|---|---| +| A | a non-sibling link is **rewritten to the wrong page**, where the README promises it is "left exactly as written" | +| B | `withinRoot` rejects a tree reached through a **symlink**, because it compares lexically — `filepath.Abs` then `filepath.Rel`, with no `EvalSymlinks` anywhere in the tree | + +The three groups are not independent. The inconsistencies exist because there was +no single answer to defer to, and two of the bugs are downstream of a convention: +basename keying is what produces the wrong page, and reference-as-written is what +produces a path export cannot honour. + +**Two things are not downstream of any convention**, and one root will not fix +them: + +- **The symlink comparison** (Scenario B). Choosing where the root comes from does not + change that the comparison is lexical. The fix is to stop following symlinks at + all — the index walk traverses none, a leaf read refuses one, and an `os.Root` + catches an escape through an intermediate directory that a leaf check cannot + see. See [docs/guarantees.md](../docs/guarantees.md#symlinks). +- **The reporting omission** (Scenario E). Links never say what they could not resolve. + That gap exists under any convention, and closing it is a diagnostic rather + than a path decision. + +## The use cases + +What markfluence should support. Mechanism comes later; this is the list to +measure a solution against. + +### Use case 1. One markdown file, no images + +``` +notes.md +``` + +Publish it. Nothing to resolve, no configuration. + +### Use case 2. Export a page, edit it, publish it back + +Take a page that already exists in Confluence, get it onto disk with its images, +edit it, publish it back. Publishing back an unedited export changes nothing. + +### Use case 3. A new page with its images in a subdirectory + +``` +project/ + page.md + images/diagram.png +``` + +The subdirectory is for tidiness and should need no configuration. + +### Use case 4. A directory of pages sharing one images subdirectory + +``` +project/ + a.md + b.md + c.md + images/x.png +``` + +The pages share a Confluence parent, so they sit in one directory. They may link +to each other. + +### Use case 5. A whole space: hierarchy, shared and page-specific images + +``` +docs/ + assets/brand.png ← shared by many pages + index.md + team/ + onboarding.md + images/flow.png ← specific to one page + ops/ + runbook.md + images/graph.png +``` + +Around 100 files. Pages link to each other across directories. Brand images are +shared; graphs are page-specific. The Confluence hierarchy comes from +frontmatter, not from the directory layout. + +### Use case 6. Links across the tree + +``` +docs/ + index.md [team onboarding](team/onboarding.md) + [the runbook](ops/runbook.md) + team/ + onboarding.md [escalation](../ops/runbook.md#escalation) + [back to the index](../index.md) + ops/ + runbook.md ## Escalation + [onboarding](../team/onboarding.md) +``` + +Four directions, all of which should resolve to the right Confluence page: +**down** into a subdirectory, **across** between two of them, **up** to the root, +and **into a heading** in a page in another directory. + +Implied by Use case 5, and stated separately because it is the requirement most +likely to be dropped. Note that every link here is spelled the way a Markdown +preview resolves it, so the tree renders correctly on GitHub before it is +published at all — which is the standard the links should be held to. + +### Use case 7. Publishing a whole tree for the first time + +Nothing has a `page_id` yet, and links point at pages that do not exist until +the run creates them — including pairs that link to each other. + +The requirement is that one command publishes the tree with every link resolved, +rather than leaving the reader to know that a second pass is needed. + +### Use case 8. Exporting a subtree or a whole space + +The inverse of Use case 5. Does not exist today: `export` is single-page. + +Three provenance variations have to work, because a real space contains all +three: pages markfluence published (their attachments carry recorded paths), +pages that originated in Confluence (their attachments carry none), and a subtree +mixing the two. + +### Use case 9. Moving or renaming a markdown file + +Reorganise the repository without churning Confluence. The page keeps its +identity, and its attachments keep theirs. + +Two variants, because a page with images can be moved two ways: + +``` +(a) the markdown and its images move together + docs/index.md + docs/images/x.png ![x](images/x.png) + → docs/guides/index.md + docs/guides/images/x.png ![x](images/x.png) + +(b) the markdown moves, the images stay, the links are updated + docs/index.md + docs/images/x.png ![x](images/x.png) + → docs/guides/index.md + docs/images/x.png ![x](../images/x.png) +``` + +Variant (a) is the tidy per-page layout of Use cases 3 and 4 moved as a unit. +Variant (b) is what happens when the images are shared and stay put. + +The requirement is that neither churns Confluence: no re-uploaded attachment +under a new name, no stranded original. **Only one of the two can be met** — see +below. + +### Use case 10. Cloning the repository, or publishing from CI + +Someone else checks out the repo, or CI does, and publishing produces the same +result — same attachment names, same links. + +### Use case 11. Smaller cases, so they are deliberate + +- **Non-image attachments** (PDF, CSV) via `attachment-upload`. +- **Attachments added in the Confluence UI** by someone else, which markfluence + must leave alone. +- **A page whose title changes**, since an exported filename is slug-derived. + +## The solution model + +One root. Every recorded path is relative to it. + +- **The root** is the directory holding `markfluence.yaml`, found by walking up + from the working directory. With no config file, the root is the markdown + file's own directory. `--root` overrides discovery. +- **The root is always reported**, in human output and in `--json`. It silently + determines every attachment name and bounds what may be published, so leaving + it invisible would make **L2** true but unauditable — and a stray config file in + an ancestor directory undetectable. +- **Assets must live at or below the root.** Above it is `IMAGE BROKEN`, as + today. +- **Markdown still writes page-relative paths**, so a Markdown preview renders + them. `![](images/flow.png)` from `team/onboarding.md` is unchanged. +- **The recorded attachment source is root-relative.** That same reference + records `team/images/flow.png`, named `team%2Fimages%2Fflow.png`. +- **Links resolve root-relative**, against an index of the tree below the root + rather than by basename against one directory. +- **Symlinks are not followed** ([non-goal](../docs/guarantees.md#symlinks)). The + index walk does not traverse them, a leaf read refuses one, and reads go through + an `os.Root` on the root as the backstop — which also makes *how the root was + reached* irrelevant. + +There is no flat mode and no nested mode. For a project whose files all sit in +one directory, the root *is* that directory, and root-relative and page-relative +produce byte-identical results. "Flat" is not a mode; it is what this model does +when the tree is one level deep. + +### The project file + +#### The no-config default is stricter than today + +Without `markfluence.yaml`, the root is the markdown file's own directory — so an +asset *above* the page is outside the root and becomes `IMAGE BROKEN`. That is +narrower than today, where the root is the working directory and running from the +tree root lets a page reach an asset above itself. + +Scenario B's own tree is the example. `docs/team/onboarding.md` referencing +`../assets/logo.png` publishes today when run from `docs/`; with no project file +its root is `docs/team/` and the asset is out of bounds. + +This is deliberate rather than an oversight — **assets above a page are exactly +the case that needs a declared root**, and Use case 5 is where they appear. But it +means the shared-parent layout the README endorses is repaired by this model +*only when a project file exists*, and it is why item 12 has to make the root +explicit in the regression suite: `regression/images-shared-parent/test.input` has +no root today, and its golden pins `source: "../assets/logo.png"`. + + + +`markfluence.yaml`, in the root directory. **Its existence is its whole meaning:** +it marks the root, and nothing in it is read yet. + +Visible rather than hidden, and named for the tool the way `go.mod`, `Cargo.toml` +and `pyproject.toml` are. The root silently decides every attachment name and +bounds what may be published, so being able to see where it is matters more here +than tidiness in a directory listing does. + +It carries a comment rather than being literally empty, so a reader who finds it +learns what it does: + +```yaml +# Marks the root of a markfluence project. Image and link paths are recorded +# relative to this directory. https://github.com/mozilla/markfluence +``` + +**YAML by name, with no parser yet.** Nothing in the file is read, so nothing +parses it and no dependency is needed today. The `.yaml` extension fixes the +intended format so adding keys later is not a migration. + +When keys do arrive the format costs something: **there is no YAML library in this +module.** `go.yaml.in/yaml/v3` appears in `go.sum` only as a `/go.mod` hash — a +module-graph entry with no zip hash — so `go mod why` reports "main module does +not need package" and importing it fails on a missing `go.sum` entry. Adding it is +a new direct dependency, not a promotion. The alternative is a third minimal +parser, after `internal/frontmatter` and the `.env` reader; `frontmatter` itself +cannot be reused, since it requires `---` fences and returns an empty map without +them. + +**Two precedence chains, and they must not be conflated.** Credentials resolve +**flag > environment > `.env`** and are about *who you are*. Anything the project +file grows later is about *what the content is*, and would resolve **flag > +frontmatter > project file** — a different chain, because a per-file answer +should beat a per-project one. A default `space` is the obvious first candidate, +since Use case 5 repeats `space: ENG` across a hundred files. + +**`.env` is read from the root.** One walk, one anchor: it is read from wherever +`markfluence.yaml` was found, so running from a subdirectory works for +credentials as well as for assets. With no project file, the working directory, as +today. `--env-file` still overrides absolutely. + +The two files stay separate — `markfluence.yaml` is committed and shared, `.env` +is gitignored and personal — and one footgun comes with the change: a stray `.env` +in an ancestor can hand a project credentials that are not its own. Reporting the +root is what makes that visible. + +Unresolved and deferred to when keys exist: what markfluence does with a key it +does not recognise, and whether a malformed file is an error or still a valid +marker. + +#### Hooks, and why discovery must not authorise them + +markfluence today executes nothing, which is why the discovery risk in *What +changes* is framed as narrow. That may not hold: a git-style hook system — run a +named program at a point in the pipeline — is a plausible direction, because it +lets users add variable expansion, mermaid rendering, a "maintained at" +banner, and other things without markfluence carrying the maintenance for each +of them. The hook system does not need designing now and is out of scope for +this spec. + +One constraint does need honouring now, because it is free today and a retrofit +later: **discovering a file by walking up must not, by itself, authorise anything +in it to run.** Git's hooks live inside the directory its discovery walk finds, +which is precisely what made CVE-2022-24765 reachable, and the answer was +`safe.directory` bolted on afterwards. Keeping the root marker separable from any +future execution declaration — distinct files, or an explicit consent step in the +shape of `direnv allow` — costs nothing while neither exists. + +A milder form of the same applies to `.env`. Reading it from a discovered root +means a stray one in an ancestor can hand a project credentials that are not its +own, which is a footgun in your own tree and worse in a shared one. + +### How S2 (no-read-outside-root) is enforced + +Mostly it is not enforced, because it stops being enforceable-by-check and +becomes true by construction. + +**Links and anchors need no clamp.** The index is built by walking *down* from the +root, so resolution is a map lookup rather than a file read. Nothing outside the +root can be in the index, and `filepath.WalkDir` does not descend a symlink to put +it there (verified — see the non-goal). So `[x](../../../../etc/passwd.md)` is not +refused; it is simply not found, falls through to "left exactly as written", and +is reported under **R1** (report-unresolved-references) like any other +unresolved link. It deserves its own +message — "target is outside the project root" beats "not found" — but the +behaviour is the same. + +**`parent:` is a real read**, and the only one left besides an image leaf. It goes +through the root handle, and an escaping path is a **hard error** rather than an +unresolved-and-reported: a parent is load-bearing, and publishing under the wrong +parent is worse than not publishing. `create` already errors on a bad parent, so +this joins an existing path. It does mean `cmd/create` needs the root handle, +which it has no reason to hold today. + +### The link index is built once + +Built once per run and shared, rather than rebuilt per conversion. Measured, and +doing so is faster than what happens today. + +| files | per-directory, per conversion (today) | tree-wide, per conversion | tree-wide, built once | +|---|---|---|---| +| 100 | 30ms | 162ms | 1.6ms | +| 200 | 115ms | 583ms | 3ms | +| 400 | 445ms | 2.24s | 5.5ms | + +Doubling the file count roughly quadruples the first two columns and doubles the +third, so **today's per-directory index is already O(n²)**: each conversion +re-reads its whole directory, and 40 files in a directory means 40 reads for each +of 400 conversions. Tree-wide indexing does not introduce a new complexity class, +it multiplies the existing one by about five. + +Built once, the index is O(n) and about **80× faster than today** at 400 files. +Extrapolated to 1000 files: today ≈ 2.8s, tree-wide per conversion ≈ 14s, shared +≈ 14ms. So the shared index is not a performance concession the model needs — it +repairs a cost that predates it. + +### Publishing a tree in three phases + +`create` reserves ids before converting anything. + +1. **Preflight.** What phase 1 already does — validate `page_id`, space, parent + and title for every file, and abort the whole run if any of them fails. +2. **Reserve.** Create a stub for every page: title, parent, no content. Capture + each `page_id` and persist it unless `--no-persist`. No conversion happens + here, and no attachments are uploaded. +3. **Publish.** Convert and update every page. Every `page_id` in the set now + exists on disk, so every link resolves. + +The reason this beats a second pass bolted onto the current design is +**determinism**. Today conversion happens per file inside the create loop, and +`buildPageMap` reads ids from disk, so a file converted later sees the ids of +files created earlier — links pointing "backwards" in parent-topological order +resolve and the rest do not, with link direction unrelated to parent order. A +cycle can never fully resolve. Reserving first removes the ordering question from +links entirely. + +Three costs, accepted: + +- **Every page's v1 is a stub, permanently.** Confluence assigns ids on creation, + so there is no way to learn an id without making a page, and the empty first + version stays in the history. +- **An interrupted run leaves stubs**, where today it leaves pages missing. More + recoverable — every id exists and is persisted, so `markfluence update *.md` + finishes the job — but uglier while it is broken. +- **Writes double**, even for a page nothing links to. + +Topological ordering stays necessary in phase 2, because creating a page needs +its parent's id at creation time. Only *link* resolution stops depending on +order. + +Under `--no-persist` phase 3 still works, since the ids are in memory for the +duration of the run; what is lost is the ability to re-run `update` afterwards, +which is already what that flag means. + +### What it costs + +**Longer attachment names.** A page-specific image becomes +`team%2Fsub%2Fimages%2Fgraph.png` rather than `images%2Fgraph.png`. The majority +case pays so the minority case — a shared image — works. Accepted: names are +identifiers, a reader sees the rendered image, and the name surfaces only in a +page's attachment list. + +**A path longer than 165 characters cannot be recorded.** Both the attachment +name and its comment cap at 255 characters +([attachments.md](../docs/confluence/attachments.md#how-long-a-name-and-a-comment-may-be), +verified 2026-08-28), and the comment binds first because it carries 90 +characters of fixed overhead — `markfluence: ` + `sha256=` + 64 hex + ` path=`. +Root-relative paths are longer than the page-relative ones recorded today, so +this ceiling gets closer rather than staying put; a 130-character path already +lands at 220. + +Over the limit is an **HTTP 400**, not a truncation, which is the outcome worth having: +a truncated comment would disagree with the local source on every publish and +re-upload the attachment forever trying to correct itself. Shortening the comment +format buys headroom — see *What changes*. + +**Which kind of move is free gets inverted.** Identity relative to the markdown +file makes Use case 9's variant (a) free and (b) churn; identity relative to the +root does the reverse. Measured, today: + +``` +(a) md + images/ moved together source=images/x.png unchanged +(b) md moved, asset left behind source=../images/x.png changed +``` + +Both cannot be free. The two variants differ in precisely which path moved, so a +path-based identity has to pick one, and only content-addressed names would make +both free — at the cost of readable names and of reconstructing a tree on export. + +Root-relative is still the right side of that trade, for two reasons. Variant (a) +churning produces an **orphan**, which is cleanable (#99) and leaves the page +correct meanwhile; the shared-asset case it buys is **broken** today, with export +refusing outright and no workaround. And L2 is unreachable from a page-relative +identity at all. + +### Multi-page export layout + +The Confluence hierarchy is mirrored into directories. A page becomes +`.md`, and gains a `/` beside it if it has children or attachments of +its own. A Confluence folder becomes a directory with no markdown in it. + +**Two placement rules for attachments, one per provenance.** This is the part the +markdown layout turns out to depend on: + +| the attachment | goes to | because | +|---|---|---| +| has a recorded `path=` (markfluence published it) | `dest/` | the recorded path is authoritative, and reconstructing it is what makes **L5**/**L6** hold | +| has none (it originated in Confluence) | `dest//` | attachment names are unique per *page*, so page-scoping cannot collide | + +Mixed provenance needs no special handling: each attachment follows its own +rule, and the two land in different parts of the tree. + +``` +dest/ + home.md + home/ + onboarding.md ← markfluence-published + onboarding/ + diagram.png ← native attachment for onboarding.md, page-scoped + escalation.md ← child page + assets/brand.png ← recorded path=assets/brand.png, shared + team/images/flow.png ← recorded path=team/images/flow.png +``` + +The export is self-describing as a result: an asset under a page's directory came +from Confluence, and an asset in the shared tree came from markfluence — until the +first republish, after which an adopted asset has a recorded path and moves into +the shared tree. + +**Recorded paths collide across pages, and benignly.** The model's success case is +one shared asset referenced from many pages, each carrying its own attachment +recording `path=assets/brand.png`. On a multi-page export all of them resolve to +`dest/assets/brand.png`. The bytes are identical, so the first write lands and the +rest skip under **S3** (no-overwrite-without-force) — the right outcome, reached +by accident. Worth stating rather than discovering: a *differing* checksum under +one recorded path means two pages disagree about what that path holds, and that +should be reported rather than skipped. + +**Why page-scoping rather than flat.** A Confluence attachment name is unique +within its page, not within the space, so fifty native pages can each carry a +`diagram.png`. Today `attachfile.Resolve` falls back to the attachment name when +there is no recorded path, so all fifty resolve to `dest/diagram.png` — which +under **S3** (no-overwrite-without-force) is a refusal, or forty-nine skips. +Page-scoping removes that class of collision by construction rather than by +detecting it. + +Two consequences to carry into implementation: + +- **Placement and the emitted markdown have to move together.** `sourceFor` + derives an unsourced attachment's markdown destination from its name, so a + page-scoped directory needs `StorageToMarkdown` to carry a per-page prefix. +- **Single-page export adopts the same rule.** Otherwise the same native page + exports as different markdown depending on how many pages were asked for. + Nothing depends on today's flat-in-the-root behaviour. + +**Slug collisions are refused, naming both pages.** `slugify` is lossy — +`Deploy: Prod`, `Deploy Prod` and `deploy-prod` all become `deploy-prod`, and +long titles truncate — so two pages can want one filename even though Confluence +enforces unique titles per space. Mirroring narrows this to siblings, and it does +not eliminate it. Refusing follows from the guarantees rather than from taste: +overwriting is out under **S3**, appending a page id would make the filename +depend on walk order (**L2** in the export direction), and skipping with a warning +is the quiet incompleteness this spec exists to remove. + +**The adopt-an-existing-page flow falls out of this for free.** Export a native +page, and its attachments land under its directory with the markdown referencing +them there. Republish, and markfluence records +`path=home/onboarding/diagram.png`. The page becomes markfluence-managed, in a +layout someone would plausibly have written by hand, without anyone deciding +anything. + +## How the guarantees are fixed + +| | before | after | +|---|---|---| +| **L1** | `sub/dup.md` reaches `./dup.md` | resolution is by path, so a basename cannot match | +| **C1** | true for images, false for links | both resolve page-relative, as a preview does | +| **L2** | the root is the working directory | the root is found by walking up, so invocation does not matter | +| **L3** | identity depends on the referencing page's depth | identity is the asset's root-relative path | +| **L5** | fails for an asset above the page | nothing can escape `--dest`, because nothing is recorded as escaping | +| **L6** | same cause | same fix | + +Two are **not** fixed here. + +**R1** needs the converter to emit a diagnostic where it emits nothing today. +Separate work, tracked with the `check` command. What this spec contributes is a +model in which "could not resolve" is precisely statable. + +**S2** (no-read-outside-root) is also fixed, though it gets more reachable before +it gets safer: path-aware links make traversal possible where basename flattening +made it impossible. The model settles the enforcement completely — links need no +clamp once resolution is an index lookup, and the two remaining reads are an +image leaf and a `parent:` path. Items 2 and 7 are the work. + +## How the use cases work + +| use case | today | under the model | +|---|---|---| +| 1. one file, no images | works | unchanged | +| 2. export, edit, publish back | works for assets at or below the page | works for any layout | +| 3. images in a subdirectory | works | byte-identical | +| 4. pages sharing an images directory | works | byte-identical; links resolve by path rather than by coincidence | +| 5. whole space | partly — see Scenarios A–D | works | +| 6. links across the tree | broken | works | +| 7. first publish of a tree | links resolve only if they point backwards in parent order; a cycle never resolves | every link resolves — `create` reserves ids in phase 2 before converting anything | +| 8. subtree export | does not exist | hierarchy mirrored into directories; attachments placed by provenance | +| 9. moving a file | (a) free, (b) renames and strands | **inverted**: (b) free, (a) renames and strands | +| 10. clone, or CI | depends on the working directory | same result anywhere | +| 11. `attachment-upload` | flattens to a basename | root-relative, like everything else | + +Use cases 1, 3 and 4 are **byte-identical** to today. That is what the root +defaulting to the markdown file's own directory buys: the model changes behaviour +only for trees, which is where the problems are. + +### Worked example: Use case 5 + +``` +docs/ + + assets/brand.png + index.md ![](assets/brand.png) + team/ + onboarding.md ![](images/flow.png) ![](../assets/brand.png) + images/flow.png +``` + +| | today | under the model | +|---|---|---| +| `flow.png` source | `images/flow.png` | `team/images/flow.png` | +| `brand.png` from `index.md` | `assets%2Fbrand.png` | `assets%2Fbrand.png` | +| `brand.png` from `team/onboarding.md` | `..%2Fassets%2Fbrand.png` | `assets%2Fbrand.png` | +| `brand.png` after moving a page | renamed, originals stranded | unchanged | +| exporting either page | fails | writes the tree under `--dest` | +| `[x](../team/onboarding.md)` from `ops/` | dead, or the wrong page | resolves | +| running from `docs/team/` | `IMAGE BROKEN` for brand | works | + +Page hierarchy is untouched throughout: `parent:` in frontmatter is the only +thing that decides it, per **L8** (no-layout-inference). Nothing infers a parent +from a directory. + +### Worked example: Use case 2 + +`export --dest out/` on a page published from a nested tree writes: + +``` +out/ + onboarding.md ← contains ![flow](team/images/flow.png) + team/images/flow.png ![brand](assets/brand.png) + assets/brand.png +``` + +The markdown lands at the dest root carrying the recorded sources verbatim +(`sourceFor`), which resolve from there and render in a preview. Re-publishing it +— no config, so the root is `out/` — records the same two sources. Same names, no +orphans, so **L5** holds. + +The page landed at `out/onboarding.md`, not at a path mirroring the disk layout it +was published from — `team/` is a source-tree directory, and export has no way to +know it. Multi-page export mirrors the *Confluence hierarchy* instead, which is a +different tree; for a single page neither matters. + +## What changes + +1. **Thread a root through the converter.** `MdToConfluence` takes it instead of + calling `os.Getwd()`. +2. **Scope reads to an `os.Root`** on the documentation root, replacing + `convert.withinRoot`'s lexical comparison. This is what makes S2 hold by + construction and retires the symlinked-checkout failure without special-casing + it. `os.Root` is the backstop for an escape through an intermediate symlinked + directory; item 7 covers the leaf. Its `path escapes from parent` message wants + wrapping the way `internal/attachfile` already wraps it. Reads through a root + handle also mean the converter takes the handle rather than a root string. +3. **Record `Source` root-relative** in `images.go`. +4. **Root-relative link resolution.** `docKey` keeps the path, and the page and + anchor indexes cover the tree below the root. The index is **built once and + passed in**, not rebuilt per conversion — which means `MdToConfluence` takes + it alongside the root. That is what keeps the walk O(n), and it happens to fix + the quadratic cost the per-directory version already has. +5. **Config file discovery**: walk up from the working directory, stat a known + filename at each level, stop at the first hit or at the filesystem root. + Discovery stats a filename rather than listing a directory, so it needs only + execute permission on each ancestor — which is guaranteed, since otherwise the + working directory would be unreachable. Treat `EACCES` as "not here, keep + walking" rather than fatal. Reaching the root without a hit is not an error. +6. **Review the security history of walk-up discovery before implementing it.** + Walking up out of your own tree and trusting what you find there is the shape + of [CVE-2022-24765](https://github.blog/2022-04-12-git-security-vulnerability-announced/), + which git answered with `safe.directory` ownership checks. markfluence + executes nothing from a config file, so the exposure is narrower — but the + discovered root decides every attachment name and bounds what may be + published. `.editorconfig` has the closest discovery model to what is proposed + here, so its issues are the ones worth reading before this is built. +7. **Refuse symlinks at the two remaining reads** — an image leaf (`os.Lstat`, + refuse anything not a regular file) and a frontmatter `parent:` path. Link and + anchor resolution needs nothing, since the index is a lookup built by a walk + that does not traverse symlinks. `cmd/create` gains the root handle so + `resolveParent` can open through it. +8. **`attachment-upload`**: root-relative source rather than `filepath.Base`. +9. **`attachfile.Resolve`**: nothing can escape any more, so keep the clamp as a + guard against server data rather than as a rule about layouts. +10. **Shorten the attachment comment format.** The comment caps at 255 characters + and its overhead is what bounds a recorded path, so the format is a budget + decision rather than a cosmetic one: + + | format | overhead | path budget | + |---|---|---| + | `markfluence: sha256=<64> path=` (today) | 90 | 165 | + | `mf: s=<64> p=` | 73 | 182 | + | `markfluence: sha256=<32> path=` | 58 | 197 | + | `mf: s=<32> p=` | 41 | 214 | + | `markfluence: s=<64> p=` (keys only) | 82 | 173 | + + **The checksum is the bigger lever**, at 64 of the 90 characters. It answers + "did these bytes change" rather than resisting an adversary, so 128 bits is + already more than the job needs — truncating it buys roughly twice what + renaming the keys does. + + Shortening the keys alone buys 8 characters and costs nothing at all, which the + table's last row shows. Against that, the prefix is not only overhead: it is + the **ownership marker** + `AttachmentMeta.Managed` tests, and therefore what **S5** (remove-only-ours) + rests on. Someone + browsing a page's attachments in Confluence can guess what `markfluence: ` + means and cannot guess `mf:`. Worth weighing 17 characters against a + self-describing marker on data other people will read. + `parseAttachmentComment` already tolerates a legacy form, so accepting both + spellings costs little. + +11. **Restructure `create` into three phases** — preflight, reserve, publish. + Phase 2 creates a stub per page (title and parent, no content) and captures + the ids; phase 3 converts and updates everything, so every link resolves + regardless of order. Phase 2 keeps the topological ordering, since a page + needs its parent's id at creation time. `--dry-run` must still create nothing. + +12. **Regression suite**: the root becomes explicit in `test.input`, and the + `images-shared-parent` golden changes — that case is the whole point. +13. **`--root`**, a persistent flag overriding discovery, with completion. +14. **Report the discovered root** in human output and in `--json`. The JSON half + is not free: `schema/json-output/v1.json` has no `root` field, and the + project's own rule is that every result field lives on a typed struct with + `additionalProperties:false`, so this means a schema change plus conformance + updates for every command. Nothing budgeted that until now. +15. **Read `.env` from the discovered root**, falling back to the working + directory when there is no project file. `internal/client.Resolve` reads + `dotenvPath` (`".env"`) against the working directory today, and it is the + single place this is resolved. +16. **Multi-page export.** The whole of *Multi-page export layout*: mirror the + hierarchy into directories, place attachments by provenance, teach + `StorageToMarkdown` a per-page prefix for unsourced attachments, adopt the + same rule in single-page export, and refuse slug collisions naming both + pages. This is the largest single piece of work in the list and depends on + `internal/pagetree` for the walk. +17. **Docs**: the README path rules, a `docs/` entry for the root model, and + status updates in [docs/guarantees.md](../docs/guarantees.md). diff --git a/_plans/026_file-organization-implementation.md b/_plans/026_file-organization-implementation.md new file mode 100644 index 0000000..74c3370 --- /dev/null +++ b/_plans/026_file-organization-implementation.md @@ -0,0 +1,389 @@ +# Plan: implementing the root model (025) + +[025](025_file-organization.md) is a spec: it establishes what "the root" means, +why one root fixes L1/C1/L2/L3/S2 and partially fixes L5/L6, and what it costs. +This plan sequences that model into landable commits. Everything here was +resolved by interview before writing any code; where this plan disagrees with +025's wording, it's because 025 left the point open (item 10's comment format) +or because implementing it surfaced a distinction 025's prose didn't need to +draw (two discovery passes, not one). + +All commits land on `file-org-fixing`, one PR at the end. + +**Status: all 9 commits landed.** Each section below is annotated with what +actually happened where it differs from what was planned going in. What's +still open, deliberately out of this plan's scope, is listed at the end of +commit 9. + +## Out of scope (deliberately) + +- **Multi-page export** (025 item 16, use case 8). Tracked separately as #59. + Nothing in this plan depends on it: once a commit here makes an image's + recorded `Source` root-relative, single-page export's Scenario D already + round-trips, because `attachfile.Resolve`'s `dest + source` join no longer + escapes. That's a side effect, not a reason to fold #59 in here. +- **A dedicated `check` command.** 025 gestures at a standalone diagnostic that + audits a whole tree without publishing and distinguishes *why* a reference + didn't resolve (missing file vs. no `page_id` vs. outside-root) with tailored + messages. That's a bigger, separate feature. What is *not* out of scope: see + R1 below — the existing `r.warnings` mechanism already reports an unresolved + image, and commit 5 extends it to links too, which is a small addition once + the link index exists rather than a new subsystem. +- **Any hook/execution system.** 025's "Hooks" section is explicitly future work. + This plan reads `markfluence.yaml` for its path only; nothing in it is parsed. +- **Generating `markfluence.yaml` for the user.** No `init` subcommand here — + that's #5. Users create the file by hand for now; its existence is its whole + meaning. +- **Refusing a batch that spans more than one discovered root.** See below — + this is allowed, not an edge case to guard against. + +## Terminology, settled + +025 uses "the root" for one idea that implementation splits into two, because +they're discovered differently and used for different things. Getting this +wrong would mean either weakening S2's fix (Scenario B) or misreporting what a +command actually did. + +- **The root.** Discovered *per markdown file*: walk up from that file's own + directory looking for `markfluence.yaml`; the first hit's directory is the + root, and reaching the filesystem root with no hit means the file's own + directory is the root. This is what bounds a file's image and `parent:` reads + (S1/S2), what its attachment `Source` is recorded relative to, and what the + link index is built from. It is reported — once per distinct value seen in a + run, not once per file. When a `markfluence.yaml` exists and every file in a + batch sits under it, every file resolves to the same root and there is + exactly one value to report; that's the intended, common case. +- **The `.env` lookup.** A separate, narrower discovery pass: walk up from the + **working directory** (not a file's directory) looking for + `markfluence.yaml`; no hit means read `.env` from the working directory + itself, as today. This exists solely to answer "where is `.env`" before any + file has been touched — credentials are resolved once, up front, for the + whole invocation, before per-file root discovery has even run. It is not + called "root" anywhere and is not reported. `--env-file` still overrides it + absolutely. + +Both passes are one function, `project.Discover(startDir string)`, called with +two different `startDir` values and two different "no hit" fallbacks (the +file's directory vs. the working directory). There are not two algorithms. + +**Addendum, landed after commit 9 (not part of the original 9-commit +sequence).** The independence stated above is about *starting point and +fallback*, not about the two passes never sharing anything. `create`, +`update`, and `attachment-upload` each build a `project.Cache` for their own +per-file root resolution regardless, and now hand that same cache to +`client.Resolve` (`Options.Roots`) instead of leaving the `.env` pass to make +its own separate `project.Discover(cwd)` call. Two consequences, for exactly +those commands: `--root` now redirects `.env` too (not just the per-file +root), and the walk is paid for once instead of twice. `internal/project.Cache` +also gained `walkAndCache`, backfilling every ancestor directory visited to a +shared `*Root` rather than caching only the exact directory `Resolve` was +called with — closing a residual quadratic-ish cost across a batch spanning +many subdirectories of one project. Caught by a self-directed code-review +pass, not planned when this section was written. + +**Multi-root batches are allowed.** Nested or sibling `markfluence.yaml` files +mean two files in one invocation can discover different roots (nearest +ancestor wins, the same rule `.editorconfig` uses). This is not special-cased: +each file's link index is scoped to its own root (a cross-root link simply +doesn't resolve — unresolved, not an error), a `parent:` escaping a file's own +root is a hard error even if the target is part of the same batch under a +*different* root, and root reporting naturally shows every distinct value used. +Refusing this outright would be extra code in service of a restriction nothing +requires. + +**`--root`** overrides discovery for the whole invocation with one value, +applied uniformly — it is a persistent flag, not a per-file setting. + +## Security review (025 item 6) + +025 asks specifically to review walk-up discovery's security history before +building it, because "discovering a file must not, by itself, authorise +anything in it to run" is free today and expensive to retrofit. Two families +reviewed, both the same shape: a tool walks up from cwd, finds *something*, and +trusts it without checking who put it there. + +- **CVE-2022-24765 / CVE-2022-29187 (git).** Pre-fix Git walked up looking for + `.git` with no ownership check. On a shared machine, another user could plant + `C:\.git` (or any ancestor `.git`) and have their config silently adopted by + everyone's git commands run from below it — including hooks. The fix, + `safe.directory`, is an ownership allowlist bolted on after the fact, and + needed a second CVE to close a Windows path-handling bypass of that same + check. 025's own "Hooks" section names this precedent already; the searches + in this review confirm it's exactly the shape (walk-up discovery, then blind + trust) and that the retrofit was not a one-shot fix. +- **Git submodule/hooks CVEs (e.g. CVE-2024-32002).** A related but distinct + lesson: several git CVEs are about a *write* landing inside `.git/` (a + submodule checkout escaping into the parent's `.git/`) rather than discovery + itself, and the payoff is always the same — a hook file that executes on the + next ordinary git operation. The lesson for markfluence isn't about + discovery's read side here; it's a second data point that "a file found by + walking a tree gets executed later" is the recurring failure, which is why + 025's constraint (discovery ≠ authorization) is worth holding even though + markfluence has no hook system yet. +- **`.editorconfig`.** No CVE turned up specific to its walk-up discovery + (its published CVEs are memory-safety bugs in `editorconfig-core-c`, + unrelated). Its discovery model is still the right one to imitate for + *semantics* — walk up, nearest file wins, an explicit marker + (`root = true`) stops the walk early — without inheriting a security + incident, because there is nothing in an `.editorconfig` file that executes. + +**What this means for `project.Discover`:** it only ever reads a filename to +decide where the root is; nothing in `markfluence.yaml` is parsed or executed, +matching the `.editorconfig` shape rather than git's pre-fix shape. The root is +always reported (visibility is the mitigation git's fix eventually converged +on anyway with `safe.directory`'s explicit allowlisting), and `.env` — the one +other thing discovery gates — is still read-only. If a hook system is ever +added, it must not be authorized merely by `markfluence.yaml`'s presence; that +already has a placeholder in 025 and isn't re-decided here. + +## Commit sequence + +### 1. `internal/project`: root discovery + +`Discover(startDir string) (*Root, error)`. `Root` carries `Dir` (absolute), +`File` (path to `markfluence.yaml`, empty when none was found), and `FS +*os.Root` opened on `Dir`. Stats a filename at each ancestor rather than +listing a directory (needs only execute permission, which is guaranteed or cwd +itself would be unreachable); `EACCES` keeps walking rather than failing; +reaching the filesystem root with no hit is not an error. `Discover` does not +follow symlinks in the walk (`filepath.Dir` on an absolute, unresolved path); +that both matches 025's non-goal and is the reason discovery cannot be tricked +by a symlinked ancestor. + +Tests: nested project files (nearest wins), no project file (fallback to +`startDir`), `EACCES` on an ancestor, filesystem-root termination. + +### 2. `.env` location + +`internal/client.loadEnvFile` calls `project.Discover(cwd)` (the second, +narrower pass above) instead of reading `./.env` directly. `--env-file` +unchanged — still absolute, still required-if-set. `dotenvPath` constant +becomes the filename joined onto the discovered directory rather than a +literal relative path. + +### 3. `--root` flag + +Persistent flag on `cmd/root.go`, directory completion via +`internal/completion`. When set, every per-file `project.Discover` call is +skipped in favor of a `Root` constructed directly from the flag value (still +opened as an `os.Root`, still validated to exist and be a directory). + +### 4. Thread the root through the converter + +- `convert.MdToConfluence` takes a `*project.Root` (per file) instead of + calling `os.Getwd()`. +- `withinRoot`'s lexical `filepath.Abs`/`filepath.Rel` comparison is replaced + by a read through `root.FS`. Mirrors `internal/attachfile`'s existing + `os.Root`-scoped `Write` — that package already has the pattern this item + copies, not invents. +- The image leaf refuses a symlink (`os.Lstat`, not `os.Stat`; anything that + isn't a regular file is broken the same way a missing file is). +- `images.go` records `Source` root-relative instead of relative to the + referencing file (025 item 3) — this is the change that fixes Scenario C and, + as a side effect, Scenario D for single-page export. +- `cmd/create` and `cmd/update` discover a root per file, caching by resolved + `Dir` across a batch so files sharing a root don't re-walk (`internal/project.Cache`). + The root actually used is reported once per distinct value, in human output + (`ui.Info`). **Landed this way; split from the plan as written below.** +- Regression suite: `test.input` gains an explicit root (default: the case's + own directory, matching the no-config fallback); `images-shared-parent`'s + golden changes, since that's the case 025 names as "the whole point." Added + dedicated (non-golden) tests for the two symlink refusals (leaf, and an + escape through a symlinked intermediate directory) — a checked-in symlink + fixture is fragile across platforms, so these are built programmatically in + Go, the way `internal/attachfile` already tests its equivalent. + +**Split off rather than done here: the `--json` half of root reporting.** +Adding `roots` to `--json` means a field on `createSummary`/`updateSummary`, +new if/then branches in `schema/json-output/v1.json`, `internal/schematest` +conformance updates, and likely touching existing literal-JSON assertions in +`create_test.go`/`update_test.go` — separable, and not worth blocking the +converter changes on. Follow-up commit, still under this item's number. + +**Caught by the regression suite, not by hand:** the first draft's boolean +guards read `lstatErr == nil` as "Lstat succeeded," but it's also true when +Lstat was never called at all (a lexically escaping path, where `info` and +`lstatErr` both sit at their zero values) — `info.Mode()` on the nil `info` +panicked. `images-broken`'s 8-`../`-deep case hit this on the first +`go test` run. Fixed by gating every derived boolean on `insideRoot` as well, +not just `lstatErr == nil`. + +### 5. Root-relative link index + +New `internal/linkindex` package. `Build(root *project.Root) (*Index, error)` +walks the tree at and below `root.Dir` once (via `root.FS`, so it cannot +descend a symlink), building the page map and anchor map keyed by root-relative +path instead of by basename in one directory. `Index.SetPage(relPath string, +entry PageEntry)` overrides/injects an entry — the hook commit 8 needs. + +`internal/convert`'s `docKey`, `buildPageMap`, `buildAnchorMap`, +`renderLink`/`rewriteDocLink` move from the per-directory, basename-keyed +lookup to a lookup against the passed-in `*linkindex.Index`. `create`/`update` +build (or reuse a cached) index per distinct discovered root and pass it into +every `MdToConfluence` call for files under that root. + +**Minimal R1, folded in here.** `rewriteDocLink` already has a clean hit/miss +against the index for anything shaped like a same-tree `.md` reference (it +already exits early with no warning for hrefs that were never meant to +resolve — external URLs, non-`.md` targets, mentions). On a miss, append to +`r.warnings` the same way `images.go` already does for a broken image. This +closes Scenario E's link half (both the missing-file case and the +exists-but-no-`page_id` case, since a `page_id`-less file was never in the +index to begin with) using plumbing that already exists, and — not +incidentally — is what makes commits 4/5/8 verifiable by hand: a test run +against the Scenario A/F fixtures now says which links didn't resolve instead +of requiring an eyeball check of rendered HTML. A dedicated `check` command +(tree-wide audit without publishing, per-reason messages) stays out of scope; +see above. + +`docs/guarantees.md`'s R1 moves from **Aspirational** to **Partial** in this +commit — not **Holds**, even though the guarantee's own wording ("every +reference... is reported") would technically be satisfied. Reserving **Holds** +for when a dedicated diagnostic exists, rather than claiming it the moment the +underlying mechanism happens to cover every case today. + +Regression suite: cases for Scenario A (cross-directory link, same basename in +two directories) and Scenario F (a link that could traverse above the root, +now resolved as "not found" rather than accidentally safe by basename +flattening), plus Scenario E's two link cases asserting they land in +`r.warnings`. + +**Landed.** Two new fixtures (`link-cross-directory`, `link-outside-root`) for +Scenario A/F; Scenario E's warning showed up as a golden change on two +*existing* fixtures (`doc-links-encoded`, `internal-doc-links`) that already +had an unresolved link, rather than needing dedicated new ones. `docKey` is +gone rather than moved — replaced by `resolveDocKey`, a method needing +`baseDir`/`root` that a free function couldn't have. `githubSlug`/ +`confluenceSlug` moved into `linkindex` (exported: `aclink.go`'s reverse +direction still needs them); a second, coincidentally-identical whitespace +regexp in `storage_to_md.go` stayed local rather than reaching into +`linkindex` for an unrelated concern. `docs/guarantees.md`: L1/C1 move to +**Holds** here; L2/L3 *also* move to **Holds**, caught up from commit 4 where +they should have been updated already rather than left for commit 9's sweep. + +### 6. S2 completion: the `parent:` read + +`cmd/create.resolveParent` currently `os.Stat`s and reads a `parent:` `.md` path +with no bound at all. It now reads through the referencing file's `root.FS`; +a path resolving outside that root is a hard error (not "not found," not an +unresolved-and-reported case — 025 is explicit that a parent is load-bearing). +A symlinked `parent:` target is refused the same way a symlinked image is. +`cmd/create` gains the root handle it has no reason to hold today. + +### 7. Attachment identity + +- `attachment-upload` records a root-relative source (`internal/project`'s + discovery from the uploaded file's directory) instead of + `filepath.Base(f)`. +- Attachment comment format shortens to `markfluence: sha256=<32hex> path=…` + (58 characters of overhead, 197-character path budget), closing #101 — + keeps the + self-describing `markfluence: ` ownership marker (what S5 rests on) and + spends the bigger, cheaper lever (128-bit truncation of a checksum that only + needs to detect a byte change, not resist an adversary) rather than the + smaller, more expensive one (shortening the prefix). `parseAttachmentComment` + already tolerates the legacy 64-hex form; both are accepted on read, only the + short form is written going forward. +- `attachfile.Resolve`'s doc comment is reframed: the clamp is a guard against + a maliciously-edited attachment comment (server data, always worth + distrusting), not a rule about legitimate layouts, since a root-relative + `Source` markfluence itself writes can no longer escape by construction. No + functional change; the existing escape tests stay, because the threat model + they guard against (a hostile comment) is unrelated to how markfluence's own + writer behaves. + +### 8. `create`'s three-phase restructure + +Preflight (today's phase 1, unchanged) → **reserve** → **publish**. + +- Reserve creates a content-less stub per file (title, parent, no body) in + topological order (a page still needs its parent's id to be created), capturing + each id. Unless `--no-persist`, the id is written back to frontmatter + immediately, matching today. Each captured id is also fed into the shared + `linkindex.Index` via `SetPage`, so publish sees ids the disk doesn't have yet + under `--no-persist`. +- Publish converts every file (now against a fully-seeded index, so link + direction and cycles both resolve) and updates each page's content, syncing + attachments. +- `--dry-run` creates nothing in reserve; publish still runs for preview using + whatever the index has (published parents resolve, in-set siblings don't have + ids yet — same limitation `--dry-run` already has today, just relocated). + +Heaviest test rewrite in this plan: `create_test.go`'s fixtures assume today's +single-pass create. + +### 9. Docs + +- README: path resolution rules, the root model, `--root`, where `.env` is now + read from. Also a new subsection (near "Markdown page structure," the + existing precedent for user-facing mechanics rather than Confluence-API + evidence) walking through the practical recipes this model changes the + answer to: + - **Moving or renaming a markdown file.** Links to it resolve automatically + via the root-relative index; nothing elsewhere needs editing. + - **Moving a page's own images along with it (use case 9a).** The gotcha: + this churns — attachments get re-uploaded under new root-relative names on + next publish, and the originals strand until #99 (`attachment-prune`) + exists. Called out explicitly because it inverts what today's behavior + trained users to expect (today, this variant is the free one). + - **Renaming or moving a shared asset.** Every page referencing it gets a new + attachment name on next publish; the old ones strand the same way. This is + L3 (identity-from-asset-location) directly: identity follows the asset, + not the page. + - **Setting up a shared assets directory across many pages.** Needs a + `markfluence.yaml` to get one stable root shared by every page under it — + without one, each page's root defaults to its own directory, and an asset + above it is `IMAGE BROKEN` (025's "no-config default is stricter" behavior, + Scenario B). +- A new doc (not under `docs/confluence/`, since none of this is + Confluence-specific) describing the root model itself — discovery, the + project file, S1/S2, link resolution — for a reader who hasn't seen 025. + Stays conceptual; links to the README subsection above for the how-tos. + +The recipe list above is a starting point, not a ceiling. Commits 1–8 will +surface scenarios worth a recipe that nobody thought of yet while writing this +plan — a fixture in the regression suite that took an extra argument to +explain, a test case for an edge in root/index caching, a failure message that +needed a "here's what to do about it" during manual verification. Flag those +as they come up rather than waiting for commit 9 to invent them from scratch. +- `docs/guarantees.md`: L1, C1, L2, L3, S2 move to their post-fix status in the + commit that actually makes each one true (not deferred to this commit) — + this entry is the final sweep, catching anything not already updated + in-place. L5/L6 stay **Partial** — the single-page round-trip works as a side + effect of commit 4, but 025's own framing ties full resolution to multi-page + export (#59), and this plan defers to that framing rather than declaring an + early win on a guarantee whose scenario table names both roundtrip + directions. R1 moved to **Partial** already, in commit 5 — this sweep just + confirms nothing here regresses it back. + +**Landed.** New doc: `docs/root-model.md` (discovery, the project file, the CVE +review restated for a reader who hasn't seen this plan, S1/S2, link +resolution, multi-root batches). README: the two stale passages that predated +this whole plan (`## Configure`'s ".env from the current directory" and +`### Body`'s "run markfluence from the root of your documentation tree," plus +its now-wrong `../assets/logo.png` → `..%2Fassets%2Flogo.png` encoding +example) are corrected in place rather than left beside a new section that +contradicted them; a new `## The documentation root` section holds the model +summary and the four recipes, placed after `## Markdown page structure` per +the plan, with a `## Configure` cross-reference for `.env`'s discovery. +`docs/guarantees.md`'s L5/L6 prose was also stale in the same way (it +attributed the Partial status to a cause commit 4 had already fixed for +single-page export) and got corrected alongside the status sweep, not just the +statuses themselves. + +**Landed after commit 9: the `--json` `roots` field split off from commit 4.** +Not a field on `createSummary`/`updateSummary` as originally sketched — a +top-level `Envelope.Roots []string`, since a root isn't really per-result, it's +per-invocation (every command shares the same field, defaulting to `[]`). +`project.Cache.Roots()` gained the getter (deduped, sorted, never nil); `create` +(both the success path and phase-1 `abort`), `update`, and `attachment-upload` +set it from the batch's `*project.Cache`. `attachment-upload` had never gotten +human-output root reporting either (only `create`/`update` had it from commit +4) — added alongside the JSON field rather than left split again. Every other +command keeps the schema's now-required `roots: []` via `NewEnvelope`'s +default; no other command has a per-file root concept to report. + +**Not part of this plan, and still open:** +- Multi-page export (item 16) — tracked as #59, as decided before commit 1. +- A dedicated `check` command for the full form of R1 — gestured at throughout, + never scoped. diff --git a/cmd/attachmentlist/json_test.go b/cmd/attachmentlist/json_test.go index 0f42c48..d36ee10 100644 --- a/cmd/attachmentlist/json_test.go +++ b/cmd/attachmentlist/json_test.go @@ -70,25 +70,6 @@ func TestBuildResultManaged(t *testing.T) { } } -// TestBuildResultLegacyManaged covers the attachment every page published -// before the encoding change still carries: a legacy checksum comment, so it is -// managed and has a checksum but no recorded source. It must not be reported as -// hand-uploaded -- managed is what tells the two apart. -func TestBuildResultLegacyManaged(t *testing.T) { - a := client.Attachment{ID: "att3", Title: "assets_x.png"} - a.Metadata.Comment = "mzcld:checksum: e733ac00" - res := buildResult(a) - if !res.Managed { - t.Error("managed = false, want true for a legacy comment") - } - if res.SHA256 == nil || *res.SHA256 != "e733ac00" { - t.Errorf("sha256 = %v, want e733ac00", res.SHA256) - } - if res.Source != nil { - t.Errorf("source = %v, want null", res.Source) - } -} - // TestBuildResultHandUploadedNullsMetadata is the signal attachment-list exists // to give: an attachment publishing will not touch. func TestBuildResultHandUploadedNullsMetadata(t *testing.T) { diff --git a/cmd/attachmentupload/attachmentupload.go b/cmd/attachmentupload/attachmentupload.go index 4624b3a..2ebd39e 100644 --- a/cmd/attachmentupload/attachmentupload.go +++ b/cmd/attachmentupload/attachmentupload.go @@ -6,12 +6,14 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/completion" "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/pageref" + "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/ui" "github.com/spf13/cobra" ) @@ -32,8 +34,9 @@ var Cmd = &cobra.Command{ Long: "Upload or replace attachments on a Confluence page.\n\n" + "PAGE is a numeric page id, a Confluence page URL, or a markdown file\n" + "whose frontmatter has a page_id.\n\n" + - "Each file is attached under its base name. A file whose contents\n" + - "already match the attachment on the page is skipped, using the same\n" + + "Each file is attached under its path relative to the documentation\n" + + "root (its base name, with no markfluence.yaml above it). A file whose\n" + + "contents already match the attachment on the page is skipped, using the same\n" + "checksum bookkeeping create/update use, so uploading by hand and\n" + "publishing agree on what is current; --force uploads anyway.\n\n" + "--name sets the attachment name for a single file, and takes a path:\n" + @@ -65,8 +68,11 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") + rootOverride, _ := cmd.Flags().GetString("root") + roots := project.NewCache(rootOverride) + defer roots.Close() c, err := client.Resolve(client.Options{ - URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, + URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, Roots: roots, }) if err != nil { return fatalFail(err.Error(), jsonout.CodeConfig) @@ -77,10 +83,13 @@ func run(cmd *cobra.Command, args []string) error { return fatalFail(err.Error(), jsonout.CodeValidation) } - attachments, err := localAttachments(files, nameFlag) + attachments, err := localAttachments(files, nameFlag, roots) if err != nil { return fatalFail(err.Error(), jsonout.CodeIO) } + for _, dir := range roots.Roots() { + ui.Info("root: " + dir) + } if dryRun && !ui.IsJSON() { ui.Warn("DRY RUN — no changes will be written.") @@ -88,9 +97,9 @@ func run(cmd *cobra.Command, args []string) error { actions, err := plan(c, pageID, attachments) if err != nil { - return operationalFail(pageID, err, jsonout.CodeFor(err)) + return operationalFail(pageID, err, jsonout.CodeFor(err), roots) } - return report(actions) + return report(actions, roots) } // plan performs the upload, or -- under --dry-run -- only the classification. @@ -131,12 +140,16 @@ func forced(actions []client.SyncAction) []client.SyncAction { // localAttachments resolves each file into an upload, checking readability up // front so a batch fails before it has half-uploaded. // -// The attachment name is the file's base name, or the encoding of --name. The -// recorded source is always the decode of the name, never the local path: if -// the two disagreed, a later publish would upload a second attachment under the -// name it computes while a download restored this one somewhere the markdown -// never references. -func localAttachments(files []string, name string) ([]client.LocalAttachment, error) { +// The attachment name is the encoding of --name, or -- with no override -- the +// file's source resolved root-relative (internal/project), the same way a +// published image's Source is: a page-specific upload of sub/img.png (no +// project file above it) still records "img.png," but a shared one under a +// declared root records "sub/img.png," matching what publishing a page that +// references the same file would record. The recorded source is always the +// decode of the name, never the local path: if the two disagreed, a later +// publish would upload a second attachment under the name it computes while a +// download restored this one somewhere the markdown never references. +func localAttachments(files []string, name string, roots *project.Cache) ([]client.LocalAttachment, error) { out := make([]client.LocalAttachment, 0, len(files)) for _, f := range files { info, err := os.Stat(f) @@ -146,9 +159,12 @@ func localAttachments(files []string, name string) ([]client.LocalAttachment, er if info.IsDir() { return nil, fmt.Errorf("%s is a directory", f) } - source := filepath.Base(f) - if name != "" { - source = name + source := name + if source == "" { + source, err = rootRelativeSource(f, roots) + if err != nil { + return nil, err + } } filename := convert.AttachmentFilename(source) if filename == "" { @@ -163,8 +179,38 @@ func localAttachments(files []string, name string) ([]client.LocalAttachment, er return out, nil } +// rootRelativeSource resolves f's root -- discovered from f's own directory, +// cached across the batch -- and returns f's path relative to it, in slash +// form. With no markfluence.yaml anywhere above f, the root falls back to f's +// own directory, so this reduces to f's bare basename exactly as before. +// +// An explicit --root can name a directory that isn't an ancestor of f at all +// (project.Resolve applies it uniformly, with no containment check of its +// own), so rel can climb above root; refuse that the same way +// internal/convert/images.go's rootRelative and create's resolveParent do, +// rather than encoding a "../"-prefixed source into the attachment name. +func rootRelativeSource(f string, roots *project.Cache) (string, error) { + abs, err := filepath.Abs(f) + if err != nil { + return "", err + } + root, err := roots.Resolve(filepath.Dir(abs)) + if err != nil { + return "", fmt.Errorf("resolving the documentation root: %w", err) + } + rel, err := filepath.Rel(root.Dir, abs) + if err != nil { + return "", err + } + rel = filepath.ToSlash(rel) + if rel == ".." || strings.HasPrefix(rel, "../") { + return "", fmt.Errorf("%s resolves outside the documentation root (%s)", f, root.Dir) + } + return rel, nil +} + // report prints the per-file actions and returns the command's exit status. -func report(actions []client.SyncAction) error { +func report(actions []client.SyncAction, roots *project.Cache) error { if ui.IsJSON() { results := make([]any, 0, len(actions)) skipped := 0 @@ -177,6 +223,7 @@ func report(actions []client.SyncAction) error { env := jsonout.NewEnvelope(command, results, map[string]int{ "total": len(actions), "succeeded": len(actions), "failed": 0, "skipped": skipped, }) + env.Roots = roots.Roots() return jsonout.Emit(os.Stdout, env) } for _, a := range actions { @@ -203,9 +250,9 @@ func fatalFail(msg string, code jsonout.Code) error { // operationalFail reports a failure against the page: under --json a results[0] // entry {ok:false,error,code}, else a human error line, exiting 1. -func operationalFail(pageID string, err error, code jsonout.Code) error { +func operationalFail(pageID string, err error, code jsonout.Code, roots *project.Cache) error { if ui.IsJSON() { - _ = jsonout.Emit(os.Stdout, failEnvelope(pageID, err, code)) + _ = jsonout.Emit(os.Stdout, failEnvelope(pageID, err, code, roots)) } else { ui.Error(err.Error()) } @@ -215,9 +262,11 @@ func operationalFail(pageID string, err error, code jsonout.Code) error { // failEnvelope is the document operationalFail writes, split out so the schema // conformance test can validate the envelope this command really emits instead // of a hand-copied duplicate of it. -func failEnvelope(pageID string, err error, code jsonout.Code) jsonout.Envelope { - return jsonout.NewEnvelope(command, []any{jsonout.NewSingleOpFailure(pageID, err, code)}, +func failEnvelope(pageID string, err error, code jsonout.Code, roots *project.Cache) jsonout.Envelope { + env := jsonout.NewEnvelope(command, []any{jsonout.NewSingleOpFailure(pageID, err, code)}, map[string]int{"total": 1, "succeeded": 0, "failed": 1, "skipped": 0}) + env.Roots = roots.Roots() + return env } // decodeName is convert.AttachmentSource, wrapped so tests can assert the diff --git a/cmd/attachmentupload/attachmentupload_test.go b/cmd/attachmentupload/attachmentupload_test.go index 6475405..f6509c0 100644 --- a/cmd/attachmentupload/attachmentupload_test.go +++ b/cmd/attachmentupload/attachmentupload_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/project" ) func writeFile(t *testing.T, dir, name string) string { @@ -24,7 +25,7 @@ func TestLocalAttachmentsUsesBaseName(t *testing.T) { dir := t.TempDir() path := writeFile(t, dir, "docs/assets/x.png") - got, err := localAttachments([]string{path}, "") + got, err := localAttachments([]string{path}, "", project.NewCache("")) if err != nil { t.Fatal(err) } @@ -42,6 +43,27 @@ func TestLocalAttachmentsUsesBaseName(t *testing.T) { } } +// TestLocalAttachmentsSourceIsRootRelative is the point of the change: under a +// declared root spanning more than the file's own directory, the recorded +// source keeps the subdirectory structure instead of flattening to a bare +// basename -- matching what publishing a page that references the same file +// would record (internal/convert/images.go). +func TestLocalAttachmentsSourceIsRootRelative(t *testing.T) { + root := t.TempDir() + path := writeFile(t, root, "docs/assets/x.png") + + got, err := localAttachments([]string{path}, "", project.NewCache(root)) + if err != nil { + t.Fatal(err) + } + if want := "docs/assets/x.png"; got[0].Source != want { + t.Errorf("source = %q, want %q", got[0].Source, want) + } + if want := "docs%2Fassets%2Fx.png"; got[0].Filename != want { + t.Errorf("filename = %q, want %q", got[0].Filename, want) + } +} + // TestLocalAttachmentsNameEncodesPath is the point of --name taking a path: the // user writes a path and markfluence produces the attachment a publish of // ![](assets/x.png) would resolve to, without them typing an escape. @@ -49,7 +71,7 @@ func TestLocalAttachmentsNameEncodesPath(t *testing.T) { dir := t.TempDir() path := writeFile(t, dir, "somewhere/else.png") - got, err := localAttachments([]string{path}, "assets/x.png") + got, err := localAttachments([]string{path}, "assets/x.png", project.NewCache("")) if err != nil { t.Fatal(err) } @@ -70,7 +92,7 @@ func TestLocalAttachmentsSourceIsAlwaysTheDecodedName(t *testing.T) { path := writeFile(t, dir, "f.png") for _, name := range []string{"", "assets/x.png", "./a/./b.png", "../shared/logo.png", "plain.png"} { - got, err := localAttachments([]string{path}, name) + got, err := localAttachments([]string{path}, name, project.NewCache("")) if err != nil { t.Fatalf("--name %q: %v", name, err) } @@ -86,12 +108,28 @@ func TestLocalAttachmentsSourceIsAlwaysTheDecodedName(t *testing.T) { } } +// TestLocalAttachmentsRejectsRootEscape covers --root naming a directory that +// isn't an ancestor of the file at all: project.Resolve applies the override +// uniformly with no containment check of its own, so rootRelativeSource must +// refuse the resulting "../"-prefixed rel itself, the same way +// internal/convert/images.go's rootRelative and create's resolveParent do, +// rather than encoding it into the attachment name. +func TestLocalAttachmentsRejectsRootEscape(t *testing.T) { + unrelatedRoot := t.TempDir() + fileDir := t.TempDir() + path := writeFile(t, fileDir, "x.png") + + if _, err := localAttachments([]string{path}, "", project.NewCache(unrelatedRoot)); err == nil { + t.Error("want an error when --root does not contain the file") + } +} + func TestLocalAttachmentsRejectsMissingAndDirs(t *testing.T) { dir := t.TempDir() - if _, err := localAttachments([]string{filepath.Join(dir, "nope.png")}, ""); err == nil { + if _, err := localAttachments([]string{filepath.Join(dir, "nope.png")}, "", project.NewCache("")); err == nil { t.Error("want an error for a missing file") } - if _, err := localAttachments([]string{dir}, ""); err == nil { + if _, err := localAttachments([]string{dir}, "", project.NewCache("")); err == nil { t.Error("want an error for a directory") } } diff --git a/cmd/attachmentupload/json_test.go b/cmd/attachmentupload/json_test.go index afca825..4426b02 100644 --- a/cmd/attachmentupload/json_test.go +++ b/cmd/attachmentupload/json_test.go @@ -8,6 +8,7 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/schematest" ) @@ -27,7 +28,7 @@ func TestSchemaConformance(t *testing.T) { // Built by the command, not restated here: a renamed key or a changed summary // in failEnvelope has to reach the schema through this test. - failEnv := failEnvelope("9", errors.New("page 9 not found"), jsonout.CodeNotFound) + failEnv := failEnvelope("9", errors.New("page 9 not found"), jsonout.CodeNotFound, project.NewCache("")) buf.Reset() if err := jsonout.Emit(&buf, failEnv); err != nil { t.Fatalf("Emit: %v", err) diff --git a/cmd/create/create.go b/cmd/create/create.go index 9276048..edb06c0 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -1,6 +1,12 @@ // Package create implements the `markfluence create` command: create new -// Confluence pages from markdown files. Creation is two-phase: every file is -// validated first, and only if all pass are the pages created (parents first). +// Confluence pages from markdown files. Creation is three-phase: every file is +// validated first (preflight); if all pass, a content-less stub is created +// for each, parents first, capturing every id (reserve); only then is every +// page converted and given real content (publish). Reserving every id before +// converting anything is what makes link resolution stop depending on +// creation order -- a link pointing "forward" in the batch resolves exactly +// like one pointing "backward," and a cycle between two pages in the same +// batch resolves too. package create import ( @@ -16,8 +22,10 @@ import ( "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" + "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/ui" "github.com/spf13/cobra" ) @@ -38,10 +46,12 @@ var Cmd = &cobra.Command{ Short: "Create new Confluence pages from markdown files", Long: "Create new Confluence pages from markdown FILEs.\n\n" + "All files are validated first; if any would fail, nothing is created.\n" + - "Otherwise pages are created parents-first. --title and --page-width override\n" + - "the frontmatter (--title requires a single FILE). Unless --no-persist is\n" + - "given, each created page's title/space/parent/page_id/page_width are written\n" + - "back into the frontmatter.", + "Otherwise a content-less stub is reserved for each, parents-first, before\n" + + "any of them is converted -- so a link between two files in the same batch\n" + + "resolves regardless of which direction it points, or whether they form a\n" + + "cycle. --title and --page-width override the frontmatter (--title requires\n" + + "a single FILE). Unless --no-persist is given, each created page's\n" + + "title/space/parent/page_id/page_width are written back into the frontmatter.", Args: cobra.MinimumNArgs(1), ValidArgsFunction: completion.MarkdownFiles, RunE: run, @@ -87,6 +97,13 @@ type record struct { spaceID string parent parentInfo width pagewidth.Width + // root bounds this file's image/parent reads and is what its attachments' + // names and recorded Source are relative to. Discovered from the file's own + // directory, cached across the batch by internal/project.Cache. + root *project.Root + // index is the tree-wide link/anchor index for root, shared by every file + // under the same root (internal/linkindex.Cache). + index *linkindex.Index } // failure is a phase-1 validation error against a file (or "(hierarchy)"). @@ -210,8 +227,11 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") + rootOverride, _ := cmd.Flags().GetString("root") + roots := project.NewCache(rootOverride) + defer roots.Close() c, err := client.Resolve(client.Options{ - URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, + URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, Roots: roots, }) if err != nil { return fatalFail(err.Error(), jsonout.CodeConfig) @@ -228,18 +248,22 @@ func run(cmd *cobra.Command, args []string) error { } } spaceCache := map[string]string{} + indexes := linkindex.NewCache() // Phase 1: validate every file, create nothing. var records []record var errs []failure for _, filename := range args { - r, err := resolveFile(filename, c, inSetAbs, spaceCache) + r, err := resolveFile(filename, c, inSetAbs, spaceCache, roots, indexes) if err != nil { errs = append(errs, newFailure(filename, err)) continue } records = append(records, r) } + for _, dir := range roots.Roots() { + ui.Info("root: " + dir) + } var ordered []record if len(errs) == 0 { @@ -261,21 +285,15 @@ func run(cmd *cobra.Command, args []string) error { } if len(errs) > 0 { - return abort(args, errs) + return abort(args, errs, roots) } - // Phase 2: create in topological order. - created := map[string]string{} + results := createAll(ordered, c, doPersist) failures := 0 - results := make([]*createResult, 0, len(ordered)) - for _, r := range ordered { - res := createInOrder(r, created, c, doPersist) - if res.ok { - created[r.absPath] = res.pageID - } else { + for _, res := range results { + if !res.ok { failures++ } - results = append(results, res) if !ui.IsJSON() { res.renderHuman() } @@ -287,6 +305,7 @@ func run(cmd *cobra.Command, args []string) error { items[i] = r.jsonResult() } env := jsonout.NewEnvelope("create", items, summarize(results)) + env.Roots = roots.Roots() if err := jsonout.Emit(os.Stdout, env); err != nil { return err } @@ -303,33 +322,219 @@ func run(cmd *cobra.Command, args []string) error { return nil } -// createInOrder resolves the effective parent id for a record and creates it, -// returning a result. A missing in-set parent (its creation failed earlier) is a -// failed result rather than a create attempt. -func createInOrder( - r record, created map[string]string, c *client.ConfluenceClient, doPersist bool, -) *createResult { - parentID := r.parent.id - if r.parent.kind == "inset" { - parentID = created[r.parent.abs] - // In a dry-run nothing is created, so an in-set parent has no id yet; - // that is not a failure (the parent would have been created first). The - // relationship is still reported via parent_file. - if parentID == "" && !dryRunOpt { - res := newResult(r) - return res.fail(errors.New("parent page was not created; skipping"), jsonout.CodeValidation) - } - } - return createOne(r, parentID, c, doPersist) +// pendingPublish is what phase 3 needs for a record whose reservation +// succeeded: the in-progress result to finish, and the stub's id/version. +type pendingPublish struct { + res *createResult + pageID string + version int +} + +// createAll runs phases 2 and 3 over ordered (already topologically sorted): +// reserve a content-less stub for every file, then convert and publish every +// one that reserved successfully. Splitting these into two full passes -- not +// reserve-then-publish per file -- is the point: every id any file in the set +// might link to already exists by the time phase 3 converts anything, so link +// resolution stops depending on topological order the way it used to. +// +// Results come back in ordered's order, one per record, regardless of which +// phase produced the final outcome -- a reserve failure and a publish failure +// look the same to the caller. +func createAll(ordered []record, c *client.ConfluenceClient, doPersist bool) []*createResult { + // Phase 2: reserve, in topological order (a page needs its parent's id at + // creation time). + created := map[string]string{} // absPath -> pageID, for resolving an in-set parent + pending := map[string]pendingPublish{} + final := map[string]*createResult{} + + for _, r := range ordered { + parentID := r.parent.id + if r.parent.kind == "inset" { + parentID = created[r.parent.abs] + // In a dry-run nothing is created, so an in-set parent has no id + // yet; that is not a failure (the parent would have been created + // first). The relationship is still reported via parent_file. + if parentID == "" && !dryRunOpt { + res := newResult(r) + final[r.absPath] = res.fail(errors.New("parent page was not created; skipping"), jsonout.CodeValidation) + continue + } + } + + res, pageID, version, ok := reserveOne(r, parentID, c, doPersist) + if !ok { + final[r.absPath] = res + continue + } + created[r.absPath] = pageID + // Seed the shared link index immediately, using the identical key + // MdToConfluence would compute for this file -- so phase 3 sees this id + // regardless of link direction or a cycle among the files being created, + // regardless of --no-persist (which skips the frontmatter write but not + // this), and regardless of --dry-run (whose pageID is empty, but the + // entry's mere presence in the index is what a link lookup checks, so a + // cross-link between two new files in the same batch still resolves in + // the preview instead of warning "not resolved"). + r.index.SetPage(convert.DocKeyFor(r.root, r.filename), linkindex.PageEntry{PageID: pageID, Title: r.title}) + pending[r.absPath] = pendingPublish{res: res, pageID: pageID, version: version} + } + + // Phase 3: convert and publish every reserved page, now that every id any + // of them might link to already exists. + for _, r := range ordered { + p, ok := pending[r.absPath] + if !ok { + continue + } + final[r.absPath] = publishOne(r, p.res, p.pageID, p.version, c) + } + + results := make([]*createResult, len(ordered)) + for i, r := range ordered { + results[i] = final[r.absPath] + } + return results +} + +// reserveOne creates a content-less stub for r (title and parent, no body) and +// persists its frontmatter fields immediately unless persist is false -- so a +// run interrupted after this point has already recorded a page_id a later +// `update` can finish publishing against, rather than leaving the file +// unpublished with no trace. Under --dry-run nothing is created; ok is still +// true, since phase 3 has a preview to run even though there is no id. +// +// ok distinguishes "reservation failed outright" (the terminal result is res; +// phase 3 must not run) from "proceed to phase 3" -- which is not the same as +// res.ok, since res is not finished until publishOne finalizes it. +func reserveOne( + r record, parentID string, c *client.ConfluenceClient, persist bool, +) (res *createResult, pageID string, version int, ok bool) { + res = newResult(r) + res.parent = nullableStr(parentID) + // parent_type tracks parent: both null for a top-level page, and both null in + // a dry-run whose parent is an in-set page that has no id yet. + if parentID != "" { + res.parentType = nullableStr(r.parent.parentType) + } + + if dryRunOpt { + res.persisted = persist + return res, "", 0, true + } + + result, err := c.CreatePage(r.spaceID, r.title, "", parentID) + if err != nil { + return res.fail(err, jsonout.CodeFor(err)), "", 0, false + } + pageID = result.ID + res.pageID = pageID + res.url = pageURL(c, result, pageID) + + if persist { + parentValue, parentComment := parentField(r.parent, parentID) + content := r.mdfile.Content + content = frontmatter.UpdateField(content, "title", r.title, "") + content = frontmatter.UpdateField(content, "space", r.spaceKey, "") + content = frontmatter.UpdateField(content, "parent", parentValue, parentComment) + content = frontmatter.UpdateField(content, "page_id", pageID, "") + content = frontmatter.UpdateField(content, "page_width", string(r.width), "") + if err := os.WriteFile(r.filename, []byte(content), 0o644); err != nil { + // The page above was already created; keep its id/url in the result or + // it becomes an orphan with no local trace at all. + return res.failKeepingPage(err, jsonout.CodeIO), "", 0, false + } + res.persisted = true + } + + return res, pageID, result.Version.Number, true +} + +// publishOne converts r's body -- now against a fully-seeded link index -- and +// gives the page reserveOne created its real content: attachments and page +// width. It always finalizes res.ok/res.status, on both success and failure; +// a failure here leaves a permanent content-less stub behind, which +// _plans/026 accepts as the cost of removing the ordering dependency (an +// interrupted run leaves stubs where the old single-pass create left pages +// missing entirely -- uglier, but every id is already persisted, so a plain +// `markfluence update` finishes the job). +func publishOne(r record, res *createResult, pageID string, version int, c *client.ConfluenceClient) *createResult { + // SiteURL, not BaseURL: rewritten links are published into the page, so they + // must point at the site even when requests go through the gateway. + pageContent, err := convert.MdToConfluence(r.mdfile, r.root, r.index, c.SiteURL(), r.spaceKey, buildinfo.Stamp()) + if err != nil { + return res.fail(err, jsonout.CodeConvert) + } + res.broken = append(res.broken, pageContent.Broken...) + res.warnings = append(res.warnings, pageContent.Warnings...) + + // --dry-run: preview without creating. The page has no id/URL (reserveOne + // never created one); every attachment would be a fresh upload, and a new + // page always has its width set. + if dryRunOpt { + for _, a := range pageContent.Attachments { + res.attachments = append(res.attachments, jsonout.Attachment{Action: "created", Filename: a.Filename}) + } + res.width = &jsonout.PageWidth{Value: string(r.width), Default: false} + res.widthSet = true + res.ok = true + res.status = statusCreated + return res + } + + result, err := c.UpdatePage(pageID, r.title, pageContent.HTML, version+1, "Initial publish via markfluence") + if err != nil { + return res.fail(err, jsonout.CodeFor(err)) + } + res.url = pageURL(c, result, pageID) + + actions, err := c.SyncAttachments(pageID, toLocalAttachments(pageContent.Attachments)) + if err != nil { + return res.fail(err, jsonout.CodeFor(err)) + } + for _, a := range actions { + res.attachments = append(res.attachments, jsonout.Attachment{Action: a.Action, Filename: a.Filename}) + } + + res.width = &jsonout.PageWidth{Value: string(r.width), Default: false} + if acts, err := pagewidth.Apply(c, pageID, r.width); err != nil { + res.width = nil + res.warnings = append(res.warnings, "could not set page width: "+err.Error()) + } else { + for _, a := range acts { + if a.Action == "set" { + res.widthSet = true + break + } + } + } + + res.ok = true + res.status = statusCreated + return res } func resolveFile( filename string, c *client.ConfluenceClient, inSetAbs map[string]bool, spaceCache map[string]string, + roots *project.Cache, indexes *linkindex.Cache, ) (record, error) { mf, err := frontmatter.ParseFile(filename) if err != nil { return record{}, err } + + abs, err := filepath.Abs(filename) + if err != nil { + return record{}, err + } + root, err := roots.Resolve(filepath.Dir(abs)) + if err != nil { + return record{}, fmt.Errorf("resolving the documentation root: %w", err) + } + index, err := indexes.Get(root) + if err != nil { + return record{}, fmt.Errorf("building the link index: %w", err) + } + title := resolveTitle(titleOpt, mf) if title == "" { return record{}, errors.New("no title given (pass --title or add a 'title:' frontmatter field)") @@ -370,7 +575,7 @@ func resolveFile( return record{}, fmt.Errorf("space %q not found", spaceKey) } - parent, err := resolveParent(filename, mf.Frontmatter, inSetAbs, c, spaceID) + parent, err := resolveParent(filename, mf.Frontmatter, inSetAbs, c, spaceID, root) if err != nil { return record{}, err } @@ -379,12 +584,19 @@ func resolveFile( return record{}, err } - abs, _ := filepath.Abs(filename) - return record{filename, abs, mf, title, spaceKey, spaceID, parent, width}, nil + return record{filename, abs, mf, title, spaceKey, spaceID, parent, width, root, index}, nil } +// resolveParent resolves a file's parent: reference. A ".md" reference is read +// through root's os.Root -- root.FS -- rather than the bare filesystem: a +// parent escaping root is a hard error (S2), not an unresolved-and-reported +// case the way a link is, because a parent is load-bearing. Publishing under +// the wrong parent -- or under none, silently -- is worse than not publishing +// at all. A symlinked parent target is refused the same way a symlinked image +// leaf is. func resolveParent( filename string, fm map[string]string, inSetAbs map[string]bool, c *client.ConfluenceClient, spaceID string, + root *project.Root, ) (parentInfo, error) { fmParent := fm["parent"] fmParentSet := fmParent != "" && fmParent != "null" @@ -401,19 +613,48 @@ func resolveParent( if strings.HasSuffix(parentValue, ".md") { parentPath := filepath.Join(filepath.Dir(filename), parentValue) - if info, err := os.Stat(parentPath); err != nil || info.IsDir() { + parentAbs, err := filepath.Abs(parentPath) + if err != nil { + return parentInfo{}, err + } + rel, err := filepath.Rel(root.Dir, parentAbs) + if err != nil { + return parentInfo{}, err + } + rel = filepath.ToSlash(rel) + if rel == ".." || strings.HasPrefix(rel, "../") { + return parentInfo{}, fmt.Errorf( + "parent %s resolves outside the documentation root (%s); a parent must be within it", + parentValue, root.Dir) + } + + info, statErr := root.FS.Lstat(rel) + if statErr != nil && strings.Contains(statErr.Error(), "escapes from parent") { + // An escape only os.Root can see -- a symlinked intermediate + // directory -- reads as "not found" otherwise, the same trap + // internal/convert/images.go's rootRelative comment names; name it + // explicitly instead of sending the author looking for a typo. + return parentInfo{}, fmt.Errorf( + "parent %s resolves outside the documentation root (%s); a parent must be within it", + parentValue, root.Dir) + } + if statErr != nil || info.IsDir() { return parentInfo{}, fmt.Errorf("parent file not found: %s", parentValue) } - parentAbs, _ := filepath.Abs(parentPath) + if info.Mode()&os.ModeSymlink != 0 { + return parentInfo{}, fmt.Errorf("parent file is a symlink, not a regular file: %s", parentValue) + } + if inSetAbs[parentAbs] { // An in-set parent is a page this run creates, so its kind is known // without asking the server. return parentInfo{kind: "inset", abs: parentAbs, parentType: "page", display: parentValue}, nil } - pmf, err := frontmatter.ParseFile(parentPath) + data, err := root.FS.ReadFile(rel) if err != nil { return parentInfo{}, err } + pmf := frontmatter.Parse(parentPath, string(data)) pID := pmf.PageID() if pID == "" { return parentInfo{}, fmt.Errorf("parent not yet published (no page_id): %s", parentValue) @@ -518,90 +759,6 @@ func parentField(p parentInfo, parentID string) (value, comment string) { return parentID, p.display } -// createOne creates one page and returns a result. It performs no output; the -// caller renders the result. -func createOne(r record, parentID string, c *client.ConfluenceClient, persist bool) *createResult { - res := newResult(r) - res.parent = nullableStr(parentID) - // parent_type tracks parent: both null for a top-level page, and both null in - // a dry-run whose parent is an in-set page that has no id yet. - if parentID != "" { - res.parentType = nullableStr(r.parent.parentType) - } - - // SiteURL, not BaseURL: rewritten links are published into the page, so they - // must point at the site even when requests go through the gateway. - pageContent, err := convert.MdToConfluence(r.mdfile, c.SiteURL(), r.spaceKey, buildinfo.Stamp()) - if err != nil { - return res.fail(err, jsonout.CodeConvert) - } - res.broken = append(res.broken, pageContent.Broken...) - res.warnings = append(res.warnings, pageContent.Warnings...) - - // --dry-run: preview without creating. The page has no id/URL yet (they stay - // null); every attachment would be a fresh upload, and a new page always has - // its width set. persisted reflects intent — dry_run signals nothing was - // actually written. - if dryRunOpt { - for _, a := range pageContent.Attachments { - res.attachments = append(res.attachments, jsonout.Attachment{Action: "created", Filename: a.Filename}) - } - res.width = &jsonout.PageWidth{Value: string(r.width), Default: false} - res.widthSet = true - res.persisted = persist - res.ok = true - res.status = statusCreated - return res - } - - result, err := c.CreatePage(r.spaceID, r.title, pageContent.HTML, parentID) - if err != nil { - return res.fail(err, jsonout.CodeFor(err)) - } - newID := result.ID - res.pageID = newID - res.url = pageURL(c, result, newID) - - actions, err := c.SyncAttachments(newID, toLocalAttachments(pageContent.Attachments)) - if err != nil { - return res.fail(err, jsonout.CodeFor(err)) - } - for _, a := range actions { - res.attachments = append(res.attachments, jsonout.Attachment{Action: a.Action, Filename: a.Filename}) - } - - res.width = &jsonout.PageWidth{Value: string(r.width), Default: false} - if acts, err := pagewidth.Apply(c, newID, r.width); err != nil { - res.width = nil - res.warnings = append(res.warnings, "could not set page width: "+err.Error()) - } else { - for _, a := range acts { - if a.Action == "set" { - res.widthSet = true - break - } - } - } - - if persist { - parentValue, parentComment := parentField(r.parent, parentID) - content := r.mdfile.Content - content = frontmatter.UpdateField(content, "title", r.title, "") - content = frontmatter.UpdateField(content, "space", r.spaceKey, "") - content = frontmatter.UpdateField(content, "parent", parentValue, parentComment) - content = frontmatter.UpdateField(content, "page_id", newID, "") - content = frontmatter.UpdateField(content, "page_width", string(r.width), "") - if err := os.WriteFile(r.filename, []byte(content), 0o644); err != nil { - return res.fail(err, jsonout.CodeIO) - } - res.persisted = true - } - - res.ok = true - res.status = statusCreated - return res -} - // wantPersist resolves the --persist/--no-persist pair; --no-persist wins. func wantPersist(persist, noPersist bool) bool { return persist && !noPersist } diff --git a/cmd/create/json.go b/cmd/create/json.go index f8764e6..1eb74fc 100644 --- a/cmd/create/json.go +++ b/cmd/create/json.go @@ -5,6 +5,7 @@ import ( "os" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/ui" ) @@ -48,6 +49,21 @@ func newResult(r record) *createResult { } func (r *createResult) fail(err error, code jsonout.Code) *createResult { + r.ok = false + r.status = statusFailed + r.errMsg = err.Error() + r.code = code + r.pageID = "" + r.url = "" + return r +} + +// failKeepingPage is fail, except it leaves pageID/url alone. Use it only when a +// page really was created on the server and reservation still failed afterward +// (persisting the frontmatter) -- the id must stay visible in the result or the +// page becomes untraceable, the same reasoning that makes pageIDFailure carry an +// id forward on failure. +func (r *createResult) failKeepingPage(err error, code jsonout.Code) *createResult { r.ok = false r.status = statusFailed r.errMsg = err.Error() @@ -157,7 +173,7 @@ func summarize(results []*createResult) createSummary { // and the abort line; in JSON mode it emits an envelope with every input file // present (validation-failed ones "failed", the rest "not_created") and an // aborted summary. Either way it exits 1. -func abort(args []string, errs []failure) error { +func abort(args []string, errs []failure, roots *project.Cache) error { if !ui.IsJSON() { for _, e := range errs { ui.Error(fmt.Sprintf("[%s] %s", e.filename, e.message)) @@ -197,6 +213,7 @@ func abort(args []string, errs []failure) error { env := jsonout.NewEnvelope("create", items, createSummary{Total: len(args), Succeeded: 0, Failed: failed, Aborted: true}) + env.Roots = roots.Roots() if err := jsonout.Emit(os.Stdout, env); err != nil { return err } diff --git a/cmd/create/json_test.go b/cmd/create/json_test.go index 60c32de..8ccac83 100644 --- a/cmd/create/json_test.go +++ b/cmd/create/json_test.go @@ -3,11 +3,15 @@ package create import ( "bytes" "encoding/json" + "io" + "os" "testing" "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/schematest" + "github.com/mozilla/markfluence/internal/ui" ) func TestSchemaConformance(t *testing.T) { @@ -66,6 +70,35 @@ type errString string func (e errString) Error() string { return string(e) } +// TestFailNullsPageIDAndURL covers the ordinary failure path: fail() must +// clear pageID/url even when the caller already set them (a failure after a +// successful CreatePage), matching the schema's null-on-failure contract. +func TestFailNullsPageIDAndURL(t *testing.T) { + r := &createResult{file: "a.md", pageID: "456", url: "https://x/456"} + r.fail(errString("boom"), jsonout.CodeConvert) + if r.pageID != "" || r.url != "" { + t.Errorf("fail() left pageID=%q url=%q, want both cleared", r.pageID, r.url) + } + if r.ok { + t.Error("fail() must leave ok false") + } +} + +// TestFailKeepingPageLeavesPageIDAndURL covers the one exception: a page was +// really created on the server and reservation still failed afterward +// (persisting the frontmatter). The id must survive so the result can name +// the orphaned page. +func TestFailKeepingPageLeavesPageIDAndURL(t *testing.T) { + r := &createResult{file: "a.md", pageID: "456", url: "https://x/456"} + r.failKeepingPage(errString("disk full"), jsonout.CodeIO) + if r.pageID != "456" || r.url != "https://x/456" { + t.Errorf("failKeepingPage() cleared pageID=%q url=%q, want both kept", r.pageID, r.url) + } + if r.ok { + t.Error("failKeepingPage() must leave ok false") + } +} + func TestJSONResultCreated(t *testing.T) { parent := "123" parentType := "folder" @@ -204,3 +237,47 @@ func TestCreateSummaryAbortedJSON(t *testing.T) { t.Errorf("summary JSON = %s, want %s", b, want) } } + +// TestAbortReportsRoots exercises abort() itself (not a hand-copied envelope), +// confirming the top-level "roots" field carries what the batch actually +// resolved rather than the empty default every other command leaves it at. +func TestAbortReportsRoots(t *testing.T) { + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + dir := t.TempDir() + roots := project.NewCache("") + t.Cleanup(roots.Close) + if _, err := roots.Resolve(dir); err != nil { + t.Fatalf("Resolve: %v", err) + } + + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("Pipe: %v", err) + } + old := os.Stdout + os.Stdout = w + err = abort([]string{"bad.md"}, []failure{{filename: "bad.md", message: "no title given"}}, roots) + os.Stdout = old + if err := w.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("abort: %v", err) + } + out, readErr := io.ReadAll(r) + if readErr != nil { + t.Fatalf("ReadAll: %v", readErr) + } + + var env struct { + Roots []string `json:"roots"` + } + if err := json.Unmarshal(out, &env); err != nil { + t.Fatalf("Unmarshal: %v\noutput: %s", err, out) + } + if len(env.Roots) != 1 || env.Roots[0] != dir { + t.Errorf("roots = %v, want [%s]", env.Roots, dir) + } +} diff --git a/cmd/create/parent_test.go b/cmd/create/parent_test.go new file mode 100644 index 0000000..e954d18 --- /dev/null +++ b/cmd/create/parent_test.go @@ -0,0 +1,171 @@ +package create + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/project" +) + +func rootFor(t *testing.T, dir string) *project.Root { + t.Helper() + root, err := project.FromPath(dir) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = root.FS.Close() }) + return root +} + +func TestResolveParentNoneGiven(t *testing.T) { + root := rootFor(t, t.TempDir()) + p, err := resolveParent(filepath.Join(root.Dir, "a.md"), map[string]string{}, nil, nil, "", root) + if err != nil { + t.Fatal(err) + } + if p.kind != "top" { + t.Errorf("kind = %q, want top", p.kind) + } +} + +func TestResolveParentMdFileNotFound(t *testing.T) { + root := rootFor(t, t.TempDir()) + _, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "missing.md"}, nil, nil, "", root) + if err == nil || !strings.Contains(err.Error(), "parent file not found") { + t.Errorf("err = %v, want a not-found error", err) + } +} + +func TestResolveParentMdFileNoPageID(t *testing.T) { + root := rootFor(t, t.TempDir()) + if err := os.WriteFile(filepath.Join(root.Dir, "parent.md"), + []byte("---\ntitle: P\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + _, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "parent.md"}, nil, nil, "", root) + if err == nil || !strings.Contains(err.Error(), "not yet published") { + t.Errorf("err = %v, want a not-yet-published error", err) + } +} + +func TestResolveParentMdFileInSet(t *testing.T) { + root := rootFor(t, t.TempDir()) + parentPath := filepath.Join(root.Dir, "parent.md") + if err := os.WriteFile(parentPath, []byte("body\n"), 0o644); err != nil { + t.Fatal(err) + } + inSetAbs := map[string]bool{parentPath: true} + p, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "parent.md"}, inSetAbs, nil, "", root) + if err != nil { + t.Fatal(err) + } + if p.kind != "inset" || p.abs != parentPath { + t.Errorf("p = %+v, want kind=inset abs=%q", p, parentPath) + } +} + +// TestResolveParentMdFilePublished is the ordinary case: an already-published +// parent, reached through root.FS rather than the bare filesystem now, but +// with the same outcome as before. +func TestResolveParentMdFilePublished(t *testing.T) { + root := rootFor(t, t.TempDir()) + if err := os.WriteFile(filepath.Join(root.Dir, "parent.md"), + []byte("---\npage_id: 100\ntitle: Parent\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + c := parentServer(t, map[string]string{"100": `{"id":"100","spaceId":"space1"}`}, nil) + + p, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "parent.md"}, nil, c, "space1", root) + if err != nil { + t.Fatal(err) + } + if p.kind != "published" || p.id != "100" { + t.Errorf("p = %+v, want kind=published id=100", p) + } +} + +// TestResolveParentEscapingRootIsHardError is S2 for parent: -- a parent +// outside the documentation root is refused outright, not left unresolved and +// reported the way an escaping link is: a parent is load-bearing, and +// publishing under the wrong one (or silently under none) is worse than not +// publishing at all. +func TestResolveParentEscapingRootIsHardError(t *testing.T) { + base := t.TempDir() + docs := filepath.Join(base, "docs") + if err := os.Mkdir(docs, 0o755); err != nil { + t.Fatal(err) + } + root := rootFor(t, docs) + // The parent target sits outside root, in base itself. + if err := os.WriteFile(filepath.Join(base, "outside.md"), + []byte("---\npage_id: 1\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + + _, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "../outside.md"}, nil, nil, "", root) + if err == nil || !strings.Contains(err.Error(), "outside the documentation root") { + t.Errorf("err = %v, want a hard error naming the escape", err) + } +} + +// TestResolveParentEscapeThroughSymlinkedDirectoryNamesTheEscape mirrors +// internal/convert's TestRenderImageRefusesEscapeThroughSymlinkedDirectory: a +// lexical containment check sees the parent as inside root, and only os.Root +// -- which resolves the symlink -- refuses it. The message must name the +// escape, not read as a plain "not found" the way any other Lstat error does; +// an author chasing that message would go looking for a typo instead of +// realizing the parent escapes the documentation root. +func TestResolveParentEscapeThroughSymlinkedDirectoryNamesTheEscape(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevated privileges on Windows") + } + root := rootFor(t, t.TempDir()) + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "parent.md"), + []byte("---\npage_id: 1\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + // "assets" looks like an ordinary subdirectory of root; it actually leads + // outside it. + if err := os.Symlink(outside, filepath.Join(root.Dir, "assets")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + _, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "assets/parent.md"}, nil, nil, "", root) + if err == nil || !strings.Contains(err.Error(), "outside the documentation root") { + t.Errorf("err = %v, want a hard error naming the escape, not \"not found\"", err) + } +} + +// TestResolveParentRefusesSymlinkedTarget mirrors the image leaf's refusal: +// even a symlink resolving inside root is not a regular file. +func TestResolveParentRefusesSymlinkedTarget(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevated privileges on Windows") + } + root := rootFor(t, t.TempDir()) + outside := t.TempDir() + real := filepath.Join(outside, "real.md") + if err := os.WriteFile(real, []byte("---\npage_id: 1\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root.Dir, "parent.md") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + _, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "parent.md"}, nil, nil, "", root) + if err == nil || !strings.Contains(err.Error(), "symlink") { + t.Errorf("err = %v, want a symlink refusal", err) + } +} diff --git a/cmd/create/run_test.go b/cmd/create/run_test.go new file mode 100644 index 0000000..25c8d37 --- /dev/null +++ b/cmd/create/run_test.go @@ -0,0 +1,461 @@ +package create + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "testing" + + "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/linkindex" + "github.com/mozilla/markfluence/internal/project" +) + +// fakeConfluence is a minimal in-memory double covering exactly what create's +// full flow touches: space resolution, title-freeness search, page +// create/update, and page-width content properties. It has no attachment +// support, so every fixture in this file must reference no local images +// (planAttachments skips ListAttachments entirely when there are none to +// sync, so nothing here needs to fake it). +type fakeConfluence struct { + t *testing.T + mu sync.Mutex + nextID int + pages map[string]*fakePage // id -> page + + // failCreateForTitle, when set, makes CreatePage fail for that one title -- + // used to test a reserve failure cascading to a child. + failCreateForTitle string + // failUpdateForTitle, when set, makes UpdatePage fail for the page that was + // created under that title -- used to test a publish-phase failure after a + // successful reserve. + failUpdateForTitle string +} + +type fakePage struct { + id, title, parentID, spaceID, body string + version int +} + +func newFakeConfluence(t *testing.T) (*client.ConfluenceClient, *fakeConfluence) { + t.Helper() + f := &fakeConfluence{t: t, nextID: 100, pages: map[string]*fakePage{}} + srv := httptest.NewServer(http.HandlerFunc(f.handle)) + t.Cleanup(srv.Close) + return client.New(client.Config{SiteURL: srv.URL, Username: "u", Token: "t"}), f +} + +func (f *fakeConfluence) handle(w http.ResponseWriter, r *http.Request) { + f.mu.Lock() + defer f.mu.Unlock() + + switch { + case r.Method == http.MethodGet && r.URL.Path == "/wiki/api/v2/spaces": + _, _ = fmt.Fprint(w, `{"results":[{"id":"space1"}]}`) + + case r.Method == http.MethodGet && r.URL.Path == "/wiki/api/v2/pages": + // The title-freeness search (checkTitleFree). Every title is free. + _, _ = fmt.Fprint(w, `{"results":[]}`) + + case r.Method == http.MethodPost && r.URL.Path == "/wiki/api/v2/pages": + f.createPage(w, r) + + case r.Method == http.MethodPut && strings.HasPrefix(r.URL.Path, "/wiki/api/v2/pages/"): + f.updatePage(w, r) + + case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/properties"): + _, _ = fmt.Fprint(w, `{"results":[]}`) + + case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/wiki/api/v2/pages/"): + // UpdatePage's updateLanded re-read after a failed PUT (client.go). Always + // answer not-found, so a forced failure in these tests is never mistaken + // for a write that actually landed. + w.WriteHeader(http.StatusNotFound) + + case r.Method == http.MethodPost && strings.Contains(r.URL.Path, "/properties"): + w.WriteHeader(http.StatusOK) + + default: + f.t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + } +} + +func (f *fakeConfluence) createPage(w http.ResponseWriter, r *http.Request) { + var body struct { + SpaceID string `json:"spaceId"` + Title string `json:"title"` + ParentID string `json:"parentId"` + Body struct { + Value string `json:"value"` + } `json:"body"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + + if f.failCreateForTitle != "" && body.Title == f.failCreateForTitle { + w.WriteHeader(http.StatusInternalServerError) + _, _ = fmt.Fprint(w, `boom`) + return + } + + f.nextID++ + id := strconv.Itoa(f.nextID) + f.pages[id] = &fakePage{ + id: id, title: body.Title, parentID: body.ParentID, spaceID: body.SpaceID, + body: body.Body.Value, version: 1, + } + _, _ = fmt.Fprintf(w, `{"id":%q,"title":%q,"spaceId":%q,"version":{"number":1},`+ + `"_links":{"webui":"/spaces/ENG/pages/%s"}}`, id, body.Title, body.SpaceID, id) +} + +func (f *fakeConfluence) updatePage(w http.ResponseWriter, r *http.Request) { + id := strings.TrimSuffix(strings.TrimPrefix(r.URL.Path, "/wiki/api/v2/pages/"), "/properties") + var body struct { + Title string `json:"title"` + Body struct { + Value string `json:"value"` + } `json:"body"` + Version struct { + Number int `json:"number"` + } `json:"version"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + p, ok := f.pages[id] + if !ok { + w.WriteHeader(http.StatusNotFound) + return + } + if f.failUpdateForTitle != "" && p.title == f.failUpdateForTitle { + w.WriteHeader(http.StatusInternalServerError) + _, _ = fmt.Fprint(w, `boom`) + return + } + p.title, p.body, p.version = body.Title, body.Body.Value, body.Version.Number + _, _ = fmt.Fprintf(w, `{"id":%q,"title":%q,"version":{"number":%d},`+ + `"_links":{"webui":"/spaces/ENG/pages/%s"}}`, id, p.title, p.version, id) +} + +// write writes a markdown fixture and returns its path. +func write(t *testing.T, dir, name, body string) string { + t.Helper() + path := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// resetOpts zeroes every package-level flag var this file's tests touch, and +// restores the previous values on cleanup -- these are cobra flag globals, so +// a test setting one must not leak it into the next. +func resetOpts(t *testing.T) { + t.Helper() + space, parent, title, width := spaceOpt, parentOpt, titleOpt, pageWidthOpt + persist, noPersist, dryRun := persistOpt, noPersistOpt, dryRunOpt + spaceOpt, parentOpt, titleOpt, pageWidthOpt = "", "", "", "" + persistOpt, noPersistOpt, dryRunOpt = true, false, false + t.Cleanup(func() { + spaceOpt, parentOpt, titleOpt, pageWidthOpt = space, parent, title, width + persistOpt, noPersistOpt, dryRunOpt = persist, noPersist, dryRun + }) +} + +// buildRecords resolves every file into a record via the same resolveFile the +// real command uses, then topologically sorts them -- the exact plumbing +// run() drives, minus the cobra/flag-parsing layer. +func buildRecords(t *testing.T, c *client.ConfluenceClient, files []string) []record { + t.Helper() + inSetAbs := map[string]bool{} + for _, f := range files { + if abs, err := filepath.Abs(f); err == nil { + inSetAbs[abs] = true + } + } + spaceCache := map[string]string{} + roots := project.NewCache("") + t.Cleanup(roots.Close) + indexes := linkindex.NewCache() + + var records []record + for _, f := range files { + r, err := resolveFile(f, c, inSetAbs, spaceCache, roots, indexes) + if err != nil { + t.Fatalf("resolveFile(%s): %v", f, err) + } + records = append(records, r) + } + byAbs := map[string]record{} + for _, r := range records { + byAbs[r.absPath] = r + } + ordered, err := topoSort(records, byAbs) + if err != nil { + t.Fatalf("topoSort: %v", err) + } + return ordered +} + +// TestCreateAllResolvesLinksRegardlessOfDirection is commit 8's whole point: +// two sibling files link to each other, with no parent relationship deciding +// which is created first. Before the reserve/publish split, the link index +// was a snapshot taken before either file existed, so *neither* direction +// resolved (a regression from the old per-directory-rebuild code, which at +// least resolved the backward case). After the split, both resolve, because +// every id is reserved before either file is converted. +func TestCreateAllResolvesLinksRegardlessOfDirection(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\n[to b](b.md)\n") + bPath := write(t, dir, "b.md", "---\ntitle: B\n---\n[to a](a.md)\n") + + c, fake := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{aPath, bPath}) + results := createAll(ordered, c, true) + + for _, res := range results { + if !res.ok { + t.Fatalf("file %s failed: %s", res.file, res.errMsg) + } + } + + aBody := fake.pages[results[0].pageID].body + bBody := fake.pages[results[1].pageID].body + if !strings.Contains(aBody, "/pages/") || strings.Contains(aBody, `href="b.md"`) { + t.Errorf("a.md's link to b.md did not resolve:\n%s", aBody) + } + if !strings.Contains(bBody, "/pages/") || strings.Contains(bBody, `href="a.md"`) { + t.Errorf("b.md's link to a.md did not resolve:\n%s", bBody) + } +} + +// TestCreateAllNoPersistStillResolvesLinks is the other half of the point: +// --no-persist means the reserved id is never written to frontmatter, but the +// link index is seeded in memory regardless, so publish still resolves a link +// to it within the same run. +func TestCreateAllNoPersistStillResolvesLinks(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\n[to b](b.md)\n") + bPath := write(t, dir, "b.md", "---\ntitle: B\n---\nno links here\n") + + c, fake := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{aPath, bPath}) + results := createAll(ordered, c, false) // doPersist=false + + for _, res := range results { + if !res.ok { + t.Fatalf("file %s failed: %s", res.file, res.errMsg) + } + if res.persisted { + t.Errorf("file %s reports persisted under --no-persist", res.file) + } + } + aBody := fake.pages[results[0].pageID].body + if !strings.Contains(aBody, "/pages/") || strings.Contains(aBody, `href="b.md"`) { + t.Errorf("a.md's link to b.md did not resolve under --no-persist:\n%s", aBody) + } + + // Frontmatter on disk must be untouched. + raw, err := os.ReadFile(aPath) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "page_id") { + t.Errorf("--no-persist wrote page_id back to %s", aPath) + } +} + +// TestCreateAllReserveFailureSkipsChildPublish covers cascading failure: a +// parent whose reservation fails must never reach CreatePage for its child, +// and the child's result must say why. +func TestCreateAllReserveFailureSkipsChildPublish(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + parentPath := write(t, dir, "parent.md", "---\ntitle: Parent\n---\nbody\n") + childPath := write(t, dir, "child.md", "---\ntitle: Child\nparent: parent.md\n---\nbody\n") + + c, fake := newFakeConfluence(t) + fake.failCreateForTitle = "Parent" + ordered := buildRecords(t, c, []string{parentPath, childPath}) + results := createAll(ordered, c, true) + + if results[0].ok { + t.Fatal("parent reservation should have failed") + } + if results[1].ok { + t.Fatal("child must not be created when its parent failed") + } + if !strings.Contains(results[1].errMsg, "parent page was not created") { + t.Errorf("child errMsg = %q, want it to name the reason", results[1].errMsg) + } + if len(fake.pages) != 0 { + t.Errorf("no page should have been created, got %d", len(fake.pages)) + } +} + +// TestCreateAllDryRunCreatesNothing asserts the reserve/publish split honors +// --dry-run at both phases: no CreatePage, no UpdatePage, nothing written to +// disk, but the preview still resolves links (against whatever the index +// already has) and reports as if it had run. +func TestCreateAllDryRunCreatesNothing(t *testing.T) { + resetOpts(t) + dryRunOpt = true + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\nbody\n") + + c, fake := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{aPath}) + results := createAll(ordered, c, true) + + if !results[0].ok { + t.Fatalf("dry-run result failed: %s", results[0].errMsg) + } + if results[0].pageID != "" { + t.Errorf("pageID = %q, want empty under --dry-run", results[0].pageID) + } + if !results[0].persisted { + t.Error("persisted should reflect intent (true) even though nothing was written") + } + if len(fake.pages) != 0 { + t.Errorf("dry-run must create nothing, got %d pages", len(fake.pages)) + } + raw, err := os.ReadFile(aPath) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "page_id") { + t.Error("dry-run must not write to the file") + } +} + +// TestCreateAllPublishFailureNullsPageIDAndURL covers a publish-phase failure +// after a successful reserve: the stub really was created on the server, but +// the result must still report page_id/url as null on failure -- the same +// contract abortedResult holds for every failure except a blocked page_id -- +// or a --json consumer sees a "failed" result that also names a page. +func TestCreateAllPublishFailureNullsPageIDAndURL(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\nbody\n") + + c, fake := newFakeConfluence(t) + fake.failUpdateForTitle = "A" + ordered := buildRecords(t, c, []string{aPath}) + results := createAll(ordered, c, true) + + if results[0].ok { + t.Fatal("publish should have failed") + } + if results[0].pageID != "" { + t.Errorf("pageID = %q, want empty on a publish-phase failure", results[0].pageID) + } + if results[0].url != "" { + t.Errorf("url = %q, want empty on a publish-phase failure", results[0].url) + } + j := results[0].jsonResult() + if j.PageID != nil || j.URL != nil { + t.Errorf("json page_id=%v url=%v, want both null", j.PageID, j.URL) + } +} + +// TestCreateAllDryRunCrossLinkResolves covers a batch of two new files linking +// to each other under --dry-run: since neither has a real id yet, the shared +// link index must still be seeded (with an empty id) so the preview resolves +// the link exactly like a real run would, rather than warning it can't be +// resolved. +func TestCreateAllDryRunCrossLinkResolves(t *testing.T) { + resetOpts(t) + dryRunOpt = true + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\n[to b](b.md)\n") + bPath := write(t, dir, "b.md", "---\ntitle: B\n---\n[to a](a.md)\n") + + c, _ := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{aPath, bPath}) + results := createAll(ordered, c, true) + + for _, res := range results { + if !res.ok { + t.Fatalf("file %s failed: %s", res.file, res.errMsg) + } + for _, w := range res.warnings { + t.Errorf("file %s: unexpected warning in dry-run preview: %s", res.file, w) + } + } +} + +// TestReserveOneWriteFailureKeepsPageIDVisible covers the orphan case: the +// stub is really created on the server, and only the frontmatter write-back +// afterward fails. res must still carry the id/url -- via failKeepingPage -- +// or the page becomes untraceable, with nothing local pointing at it. +func TestReserveOneWriteFailureKeepsPageIDVisible(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\nbody\n") + + c, fake := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{aPath}) + r := ordered[0] + // os.WriteFile refuses a directory, simulating the write-back failing + // after CreatePage has already succeeded against the fake server. + r.filename = dir + + res, pageID, _, ok := reserveOne(r, "", c, true) + if ok { + t.Fatal("reserveOne should report failure when the write-back fails") + } + if pageID != "" { + t.Errorf("returned pageID = %q, want empty (nothing for phase 3 to publish)", pageID) + } + if res.ok { + t.Error("result should be failed") + } + if res.pageID == "" || res.url == "" { + t.Errorf("result pageID=%q url=%q, want both kept so the orphaned page stays traceable", + res.pageID, res.url) + } + if len(fake.pages) != 1 { + t.Fatalf("fake pages = %d, want exactly the one CreatePage created before the write failed", len(fake.pages)) + } +} + +// TestCreateAllStubIsEmptyThenPublished checks the actual sequencing: the +// stub created in phase 2 has no body, and phase 3's UpdatePage is what gives +// it real content -- the "every page's v1 is a stub" cost 025 accepts. +func TestCreateAllStubIsEmptyThenPublished(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + aPath := write(t, dir, "a.md", "---\ntitle: A\n---\n# Hello\n") + + c, fake := newFakeConfluence(t) + ordered := buildRecords(t, c, []string{aPath}) + results := createAll(ordered, c, true) + + if !results[0].ok { + t.Fatalf("failed: %s", results[0].errMsg) + } + p := fake.pages[results[0].pageID] + if p.version != 2 { + t.Errorf("final version = %d, want 2 (stub at 1, published at 2)", p.version) + } + if !strings.Contains(p.body, "Hello") { + t.Errorf("published body missing content: %q", p.body) + } +} diff --git a/cmd/root.go b/cmd/root.go index 2f813a0..05e891c 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -22,6 +22,7 @@ import ( "github.com/mozilla/markfluence/cmd/update" "github.com/mozilla/markfluence/internal/buildinfo" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/completion" "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/ui" "github.com/spf13/cobra" @@ -32,6 +33,7 @@ var ( usernameFlag string cloudIDFlag string envFileFlag string + rootFlag string debugFlag bool noColorFlag bool jsonFlag bool @@ -124,7 +126,12 @@ func init() { "Atlassian cloud ID; set to use a scoped API token via the api.atlassian.com "+ "gateway (falls back to $CONFLUENCE_CLOUD_ID, then .env)") rootCmd.PersistentFlags().StringVar(&envFileFlag, "env-file", "", - "Path to an env file to read (default: ./.env in the working directory)") + "Path to an env file to read (default: .env at the discovered project root, "+ + "or the working directory if none)") + rootCmd.PersistentFlags().StringVar(&rootFlag, "root", "", + "Documentation root, overriding discovery (default: the directory holding "+ + "markfluence.yaml, found by walking up from each file, or the file's own "+ + "directory if none)") rootCmd.PersistentFlags().BoolVarP(&debugFlag, "debug", "d", false, "Enable verbose debug output") rootCmd.PersistentFlags().BoolVar(&noColorFlag, "no-color", false, @@ -132,6 +139,7 @@ func init() { rootCmd.PersistentFlags().BoolVar(&jsonFlag, "json", false, "Emit machine-readable JSON to stdout instead of human output") rootCmd.PersistentFlags().SortFlags = false + completion.RegisterFlag(rootCmd, "root", completion.Directories) // The stamp already carries its own "markfluence v" prefix; print it verbatim // rather than cobra's default "markfluence version <...>" wrapper. diff --git a/cmd/root_test.go b/cmd/root_test.go index 7a30173..96e8128 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -15,7 +15,7 @@ func TestRootCommandWiring(t *testing.T) { if rootCmd.Use != "markfluence" { t.Errorf("rootCmd.Use = %q, want %q", rootCmd.Use, "markfluence") } - for _, flag := range []string{"url", "debug", "no-color", "json"} { + for _, flag := range []string{"url", "debug", "no-color", "json", "env-file", "root"} { if rootCmd.PersistentFlags().Lookup(flag) == nil { t.Errorf("persistent flag --%s not registered", flag) } diff --git a/cmd/update/update.go b/cmd/update/update.go index 1b116f6..ceefc9c 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "strings" "time" @@ -15,8 +16,10 @@ import ( "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pageref" "github.com/mozilla/markfluence/internal/pagewidth" + "github.com/mozilla/markfluence/internal/project" "github.com/mozilla/markfluence/internal/ui" "github.com/spf13/cobra" ) @@ -70,8 +73,11 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") + rootOverride, _ := cmd.Flags().GetString("root") + roots := project.NewCache(rootOverride) + defer roots.Close() c, err := client.Resolve(client.Options{ - URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, + URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, Roots: roots, }) if err != nil { if ui.IsJSON() { @@ -85,11 +91,12 @@ func run(cmd *cobra.Command, args []string) error { if dryRun { ui.Warn("DRY RUN — no changes will be written.") } + indexes := linkindex.NewCache() failures := 0 results := make([]*updateResult, 0, len(args)) for _, filename := range args { - r := processFile(filename, c) + r := processFile(filename, c, roots, indexes) results = append(results, r) if !ui.IsJSON() { r.renderHuman() @@ -98,6 +105,9 @@ func run(cmd *cobra.Command, args []string) error { failures++ } } + for _, dir := range roots.Roots() { + ui.Info("root: " + dir) + } if ui.IsJSON() { items := make([]any, len(results)) @@ -105,6 +115,7 @@ func run(cmd *cobra.Command, args []string) error { items[i] = r.jsonResult() } env := jsonout.NewEnvelope("update", items, summarize(results)) + env.Roots = roots.Roots() if err := jsonout.Emit(os.Stdout, env); err != nil { return err } @@ -123,7 +134,9 @@ func run(cmd *cobra.Command, args []string) error { // processFile publishes one file and returns a result describing the outcome. It // performs no output itself; the caller renders the result (human lines or JSON). -func processFile(filename string, c *client.ConfluenceClient) *updateResult { +func processFile( + filename string, c *client.ConfluenceClient, roots *project.Cache, indexes *linkindex.Cache, +) *updateResult { r := &updateResult{file: filename, dryRun: dryRun} mf, err := frontmatter.ParseFile(filename) if err != nil { @@ -176,9 +189,22 @@ func processFile(filename string, c *client.ConfluenceClient) *updateResult { } } + 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) + } + // SiteURL, not BaseURL: rewritten links are published into the page, so they // must point at the site even when requests go through the gateway. - pageContent, err := convert.MdToConfluence(mf, c.SiteURL(), r.space, buildinfo.Stamp()) + pageContent, err := convert.MdToConfluence(mf, root, index, c.SiteURL(), r.space, buildinfo.Stamp()) if err != nil { return r.fail(err, jsonout.CodeConvert) } diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index 5be6949..7d2ddc7 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -11,7 +11,9 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pagewidth" + "github.com/mozilla/markfluence/internal/project" ) func TestResolveTitlePageID(t *testing.T) { @@ -146,7 +148,8 @@ func TestProcessFileRejectsNonNumericPageID(t *testing.T) { t.Fatalf("writing fixture: %v", err) } - r := processFile(path, client.New(client.Config{SiteURL: "https://wiki.example.net"})) + c := client.New(client.Config{SiteURL: "https://wiki.example.net"}) + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) if r.ok { t.Fatal("a non-numeric page_id must fail the file") } @@ -176,7 +179,7 @@ func TestProcessFileReportsMissingPage(t *testing.T) { t.Fatalf("writing fixture: %v", err) } - r := processFile(path, client.New(client.Config{SiteURL: srv.URL})) + r := processFile(path, client.New(client.Config{SiteURL: srv.URL}), project.NewCache(""), linkindex.NewCache()) if r.ok { t.Fatal("a page_id that resolves to nothing must fail the file") } diff --git a/docs/confluence/attachments.md b/docs/confluence/attachments.md index c135ad2..99a8ec2 100644 --- a/docs/confluence/attachments.md +++ b/docs/confluence/attachments.md @@ -27,6 +27,47 @@ spaced filename was reachable only through the rarely-used `![]()` spelling; now the ordinary `%20` spelling reaches it, so these names are common rather than theoretical. +## How long a name and a comment may be + +Both cap at **255 characters**, and they fail very differently. + +**Verified 2026-08-28** by uploading to a scratch page, stepping the length of +each field: + +| field | 255 | 256 | over-limit response | +|---|---|---|---| +| comment | stored whole | rejected | **400** — `IllegalArgumentException: The comment is longer than 255 characters` | +| name | stored whole | rejected | **500** — a raw `HibernateJdbcException` / `SQLException [25P02]` | + +Nothing is truncated. That matters more than the limit itself: a silently +truncated comment would make the stored `path=` disagree with the local source on +every publish, and [a wrong recorded path repairs itself](#a-wrong-recorded-path-repairs-itself) +would re-upload the attachment forever trying to correct it. + +The name's failure is an unvalidated constraint violation rather than a checked +one, so it says nothing useful. It is at least not retried — a bare 500 with no +`Retry-After` is not retryable under the rules in [api.md](api.md#retries). + +### The comment limit is what bounds a path + +The comment carries fixed overhead — `markfluence: ` + `sha256=` + 64 hex + +` path=` is **90 characters** — so a recorded source path may be at most +**165 characters**. + +The name limit never binds first. A name is the path with each `/` expanded to +`%2F`, so a 165-character path would need 45 slashes before it reached 255. + +Measured against realistic paths: + +``` +path= 12 name= 14 comment= 102 images/x.png +path= 74 name= 86 comment= 164 platform/services/authentication/docs/runbooks/images/failover-diagram.png +path= 130 name= 146 comment= 220 engineering/infrastructure/kubernetes/clusters/production/runbooks/… +``` + +165 characters is enough for a deep tree but not by a wide margin, and the +overhead is mostly the checksum — 64 of the 90 characters. + ## An unlabeled multipart text part is decoded as Latin-1 markfluence records the source path and a checksum in the attachment's comment: diff --git a/docs/guarantees.md b/docs/guarantees.md new file mode 100644 index 0000000..591fe72 --- /dev/null +++ b/docs/guarantees.md @@ -0,0 +1,296 @@ +# What markfluence guarantees + +Properties markfluence holds itself to. They exist to be cited: a change that +would break one needs an argument, and a new feature that cannot satisfy one is +telling you something about the design rather than about the guarantee. + +This is the counterpart to [confluence/](confluence/), which records what we know +about *Confluence*. These are claims about **markfluence**. + +Each is deliberately about one thing, so it can be argued with on its own. + +## How to read an entry + +Every guarantee carries a status, for the same reason every entry in +[confluence/](confluence/) carries provenance — "we promise this" and "we intend +this" deserve different amounts of trust: + +- **Holds** — true today, with the thing that enforces it named. +- **Partial** — true on some paths. The failing ones are named. +- **Aspirational** — not true yet. What would make it true is named. +- **Vacuous** — nothing exercises it yet, so it is untested rather than proven. + +## Changing this document + +**Identifiers are permanent.** Never renumber and never reuse. Each guarantee +also carries a **label** — `no-read-outside-root` — which is a mnemonic for +reading and citing: "S2 (no-read-outside-root)" says enough in a heading that a +sentence-long gloss is unnecessary. Labels are permanent too, for the same reason +ids are: they get cited, and a renamed label makes an old reference silently +wrong. The id is what is authoritative when the two ever disagree. A guarantee that +stops making sense is marked retired, with the reason, rather than deleted — +plans and pull requests cite these by id, and a recycled id makes an old +reference silently wrong. + +**A change may not quietly downgrade a status.** Taking a guarantee from Holds to +Partial is a decision that belongs in the commit message and in this file, not a +side effect noticed later. + +*Retired: "Nothing in Confluence is deleted." An implementation fact rather than +a principle — it would have gone false the day the first prune feature shipped. +Replaced by S4–S6, which survive that feature and constrain how it works.* + +## Safety + +A violation here does damage, rather than producing a wrong answer. + +| | label | guarantee | status | +|---|---|---|---| +| **S1** | `no-write-outside-root` | No file is written outside the root. | Holds | +| **S2** | `no-read-outside-root` | No file is read outside the root. | Holds | +| **S3** | `no-overwrite-without-force` | No existing file is overwritten without `--force`. | Holds | +| **S4** | `no-removal-as-side-effect` | Nothing is removed as a side effect. Removal is a command's stated purpose or it does not happen. | Vacuous | +| **S5** | `remove-only-ours` | markfluence removes only what markfluence created. | Vacuous | +| **S6** | `removal-is-previewable` | A command that removes says what it will remove before doing it, and honours `--dry-run`. | Vacuous | + +**S1** is enforced by `attachfile.Resolve`, which refuses a traversing path +rather than clipping it. + +**S2** now holds for all three reads `_plans/025` names. + +The image leaf is enforced through `root.FS`, an `os.Root` scoped to the +documentation root (`internal/convert/images.go`): a lexically escaping path is +refused before ever asking it, and an escape only `os.Root` can see — a +symlinked intermediate directory — is refused too, closing what `withinRoot`'s +purely lexical comparison used to miss. The same leaf also refuses a symlink +outright via `os.Lstat`, even one resolving inside the root. + +Link and anchor resolution needs no clamp at all: `internal/linkindex.Build` +walks *down* from the root once, so nothing outside it can be in the index and +no file outside it is ever opened for this purpose — the guarantee holds by +construction rather than by a check, exactly as `_plans/025` describes. See +[Non-goals](#symlinks). + +A frontmatter `parent:` path is read the same way the image leaf is +(`cmd/create.resolveParent`, through `root.FS`), but the failure mode differs +on purpose: an escaping or symlinked parent is a **hard error**, not an +unresolved-and-reported case the way a link is. A parent is load-bearing — +publishing under the wrong one, or silently under none, is worse than not +publishing at all (`_plans/026` commit 6). + +**S3** is enforced by `export`, which stats the destination and skips both the +markdown and each attachment unless `--force`. + +### Overwriting and removing are not the same risk + +S3 covers overwriting and S4–S6 cover removal, because the risks have different +shapes. + +Overwriting is **in scope** of the operation. `export` was told to write to that +path, so the exposure is one file at a path the user named, and consent is a +flag. + +Removal is **out of scope**. Nothing in "publish this file" or "export this page" +implies deleting anything, so the exposure is unbounded in principle — which +things, and how many — and no consent gesture is defined for it. "Not without +`--force`" is the wrong shape for removal. The right shape is that it does not +happen unless removing is what was asked for. + +### Why S4–S6 are written before anything removes + +Nothing removes a local file (there is no `os.Remove` in the tree) and nothing +deletes in Confluence, so all three are vacuous and untested. They are written +anyway, because the alternative is a guarantee that expires, and because removal +is already visible on the horizon in two places: + +- **Orphaned attachments.** An attachment's identity is derived from its path, so + renaming an asset strands the old attachment. The README already tells people + to remove those by hand. +- **`export --clean`.** Subtree export will want it, so that re-exporting does + not leave pages deleted upstream lying around as stale files. + +**S5 is the one with teeth, and the machinery exists.** +`client.AttachmentMeta.Managed` is true when an attachment carries the +`markfluence: ` comment prefix and false for a hand-uploaded one. Today it is +only reported, by `attachment-list`. It is what lets a prune remove stranded +markfluence attachments while never touching a file someone attached by hand. + +## Laws + +Algebraic properties of the three mappings markfluence performs — **Resolve** (a +markdown reference to a local file), **Name** (a local file to a Confluence +identity), and **Place** (a Confluence attachment to a local file). Each is +stated so a property test can generate trees and assert it. + +| | label | guarantee | status | +|---|---|---|---| +| **L1** | `resolve-what-was-named` | A reference resolves to the file it names, or to nothing. | Holds | +| **L2** | `invocation-independent` | How a reference resolves, and what an attachment is named, depend only on the files on disk — not on the working directory, nor on which files were passed in the same command. | Holds | +| **L3** | `identity-from-asset-location` | An attachment's identity depends only on the asset's location. | Holds | +| **L4** | `publish-is-idempotent` | Publishing a file that has not changed makes no change in Confluence. | Holds | +| **L5** | `roundtrip-from-confluence` | Exporting a page, then publishing it back unedited, makes no change to the page. | Partial | +| **L6** | `roundtrip-from-disk` | Publishing a file, then exporting it, yields markdown that publishes to the same page. | Partial | +| **L7** | `output-is-valid-markdown` | Anything markfluence writes to disk is markdown that renders. | Holds | +| **L8** | `no-layout-inference` | Page identity and hierarchy are never inferred from disk layout. | Holds | + +**L1** is about correctness, not cardinality. A basename lookup used to +resolve to exactly one file — just not the one the reference named, which was +how a link to `sub/dup.md` reached `./dup.md`. `internal/linkindex` resolves by +path instead, so a basename can no longer match the wrong file +(`_plans/026` commit 5). + +**L2** is deliberately narrow. `--title` and `--page-width` change what gets +published and are meant to, so the law constrains resolution and naming only. +Within that scope it rules out a root derived from the working directory, and +equally one derived from the *set* of arguments — the same file would otherwise +be named differently depending on what else was in the batch. `internal/project` +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). + +**L3** is what makes moving a page free. Moving an *asset* still changes its +identity; buying that back would need content-addressed names, at the cost of +being able to reconstruct a tree on export. `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). + +**L5** and **L6** stay Partial, deferred to #59 (multi-page export), even +though the mechanism that made them fail is already repaired: since +`_plans/026` commit 4 records an attachment's `Source` relative to the root, +`attachfile.Resolve`'s `dest + source` join for a layout with an asset above +the page no longer escapes, and single-page export's round-trip already +works. What's still missing is multi-page export itself (Use case 8) — +provenance-based attachment placement, directory mirroring — which is what +these guarantees' own wording actually describes (a whole tree, either +roundtrip direction). Calling them Holds now would be declaring a win on +half the guarantee. + +**L7** is why a markdown destination is percent-encoded on the way out: an +unencoded space produces a file that no longer parses as a link. + +**L8** is stated negatively on purpose. Identity does not come *only* from +frontmatter — `--page-id` overrides it — so the claim worth guaranteeing is that +neither identity nor hierarchy is ever derived from where a file sits on disk. +That is what forecloses inferring a parent from a directory. + +### A corollary worth naming + +**Two people publishing the same repository from different checkouts produce the +same attachment names and the same links.** This follows from L2 and L3 together +rather than standing on its own, and it is not worth satisfying separately. It is +named because it is the form a user recognises, and because it is the one that +visibly forbids recording a checkout's disk layout on the shared server copy. + +## Conformance + +| | label | guarantee | status | +|---|---|---|---| +| **C1** | `preview-compatible-resolution` | A reference resolves the way a Markdown preview resolves it, GitHub's included. | Holds | + +Not an internal property: agreement with an external specification. It always +held for images, which resolve page-relative; links now resolve the same way +(root-relative internally, but composed from the referencing page's own +directory the same way a preview would) rather than by basename in one +directory (`_plans/026` commit 5). + +Kept separate from L1 because this is the one that could in principle be traded +away — markfluence could choose its own resolution rules and document them — and +L1 could not. + +## Reporting + +Not invariants. Publishing a dead link may be acceptable; doing it **silently** +is not. The obligation is to communicate, which is why R1 can be false while +nothing is computing a wrong answer. + +| | label | guarantee | status | +|---|---|---|---| +| **R1** | `report-unresolved-references` | Every reference markfluence could not resolve is reported. | Partial | +| **R2** | `report-unplaceable-attachments` | Every attachment markfluence could not place is reported. | Holds | + +**R1** was false by design and documented as such: the README said an +unresolved link was "published as-is, which on Confluence is a dead relative +link. There is no warning for this." A same-tree `.md` link that doesn't +resolve now lands in the same `warnings` list an unresolved image already +used (`_plans/026` commit 5) — Partial rather than Holds because that's a +minimal warning reusing an existing mechanism, not the dedicated diagnostic +(distinguishing *why* a reference failed, auditing a tree without publishing) +`_plans/025` gestures at and leaves for later. + +## Non-goals + +Decisions about what markfluence will not do. They live here because they +constrain future work the way a guarantee does: a request that needs one reversed +is a design conversation, not a bug report. + +### Symlinks + +**markfluence does not follow symlinks.** Not ones that leave the project, and +not ones that stay inside it either — one rule, because two rules produced a +system where the same symlink worked for an image and failed for a link. + +Three enforcement points, in decreasing order of how much work they take: + +| where | mechanism | cost | +|---|---|---| +| the link and anchor index | `filepath.WalkDir`, which reports a symlinked directory and does not descend it | free | +| reading a leaf, such as an image | `os.Lstat` and refuse anything that is not a regular file | one call | +| an escape through an intermediate symlinked directory | `os.Root` scoped to the root, which refuses it | already the pattern in `internal/attachfile` | + +**Verified 2026-08-28.** `WalkDir` from `docs/` over a tree containing +`docs/escape → ../outside`: + +``` +dir docs/ +SYMLINK (not descended) docs/escape ← outside/out.md never enumerated +dir docs/sub/ +file docs/sub/in.md +``` + +And `os.Root` scoped to `docs/`, for the escape case an `Lstat` on a leaf cannot +see: + +| path | `os.Root` | +|---|---| +| no symlink | allowed | +| relative symlink staying inside the root | allowed | +| relative symlink escaping the root | refused — `path escapes from parent` | +| absolute symlink, any target | refused | + +Two consequences worth having. + +**Where the root came from stops mattering.** Paths are addressed relative to the +open root handle, so a tree reached through a symlinked checkout works — `/tmp` is +`/private/tmp` on macOS and home directories are frequently links, and none of +that needs special-casing. + +**Sharing an asset directory by symlink is not supported**, deliberately. The +capability people reach for it for — one asset directory used by many pages — is +what recording attachment sources relative to the root provides directly. Git +symlinks are also not portable to Windows without `core.symlinks` and developer +mode, so a repository depending on them is not cloneable everywhere. + +This is how **S2** stops being lexical. `convert.withinRoot` compares +`filepath.Abs` output with `filepath.Rel` and never resolves anything, so today a +symlinked directory inside the tree passes the clamp while its bytes come from +outside it. + +## When a guarantee cannot be met + +Best effort, and where best effort is unavailable, an error that names the +problem and a safe next step. Never a silent partial success. + +This is a policy rather than a guarantee: it cannot be property-tested, and it +applies to all of the above rather than sitting beside them. It is also the +reason S1 refuses a traversing attachment path instead of clipping it — clipping +would write *something*, under a name nobody chose. + +## How each kind is verified + +| kind | verified by | +|---|---| +| Safety | adversarial tests: traversal attempts, pre-existing files | +| Laws | property tests: generate trees, assert the equation | +| Conformance | fixtures checked against what a Markdown preview renders | +| Reporting | example tests asserting a specific message appears | +| Policy | review judgement | diff --git a/docs/root-model.md b/docs/root-model.md new file mode 100644 index 0000000..5cbf707 --- /dev/null +++ b/docs/root-model.md @@ -0,0 +1,163 @@ +# The documentation root + +How markfluence decides which directory bounds a markdown file's reads and +names its attachments. The model itself is `_plans/025_file-organization.md`; +this is a reader-facing explanation of what it settled, without re-deriving +the reasoning. `_plans/026_file-organization-implementation.md` is the +commit-by-commit implementation log, if you want to see exactly when a given +piece landed. For the practical "how do I—" version of this (moving a file, +sharing an asset across pages), see the README's +[Documentation root](../README.md#the-documentation-root) section. + +## The root, and the other thing that looks like one + +There are two different directory lookups in markfluence, and conflating them +was the source of most of the confusion this model exists to remove. + +**The root** is discovered *per markdown file*: walk up from that file's own +directory looking for `markfluence.yaml`; the first ancestor that has one is +the root, and reaching the filesystem root with no hit means the file's own +directory is the root. This is the one that matters for correctness — it +bounds what a file may read (an image, a `parent:` reference), it's what an +attachment's recorded name and source are relative to, and it's what the +tree-wide link index is built from. When a project file exists and every file +in a batch sits under it, every file resolves to the same root — that's the +intended, ordinary case. It only fragments per file when there's no project +file at all, or when a batch happens to span more than one project (see +below). + +**The `.env` lookup** is a separate, narrower pass: it starts from the +**working directory** — not a file's directory — and with no hit falls back +to the working directory itself. It exists solely to answer "where is +`.env`," runs once per invocation before any file is touched, and doesn't +bound anything. It is not called "root" anywhere in the code and it is not +reported. `--env-file` overrides it absolutely, unaffected by any of this. + +Both passes walk up using the same primitive (`internal/project`'s +per-directory marker check), called with two different starting points and +two different "no hit" fallbacks. There is not a second algorithm — only a +second starting point. In the bare case (`project.Discover(cwd)`) that walk +is independent of anything else in the invocation. But `create`, `update`, +and `attachment-upload` each already build a `project.Cache` for their own +per-file root resolution, and hand that same cache to the client config +resolver (`client.Options.Roots`) instead of leaving `.env` to make its own, +separate walk. Two consequences follow, for exactly those commands (one with +no per-file root concept, like `read` or `search`, never builds a cache to +share): `--root`'s override — which otherwise only redirects the per-file +root images/links/`parent:` resolve against — now redirects `.env` too, and +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: + +```yaml +# Marks the root of a markfluence project. Image and link paths are recorded +# relative to this directory. https://github.com/mozilla/markfluence +``` + +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 +[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), +and `.editorconfig`'s discovery model — walk up, nearest wins, no execution — +is the one this borrows rather than git's. If a hook system is ever added, its +own consent step has to be separate from this file's presence; that isn't +decided here. + +**No `init` command generates this file.** Create it by hand; that's +[#5](https://github.com/mozilla/markfluence/issues/5). + +## `--root` + +A persistent flag overriding discovery for the whole invocation, with one +value applied uniformly to every file — not a per-file setting, since it's +meant to say "treat this directory as the root, full stop," including for a +tree that has no `markfluence.yaml` and never will (a checkout you don't +control, a generated snapshot). + +## What the root bounds + +**S1/S2** (`docs/guarantees.md`): no file is written, and — as of +`_plans/026` commit 6 — no file is read, outside the root. Three reads exist: + +- An **image leaf**. Resolved relative to the root (so `../assets/logo.png` + from a page one directory below the root is fine, and the same reference + from a page at the root is not); a path that resolves outside the root is + `IMAGE BROKEN`, and a symlink is refused outright even when it resolves + inside the root (`os.Lstat`, not `os.Stat`). +- A frontmatter **`parent:` path**. Read through the same root, but the + failure mode is different on purpose: an escaping or symlinked parent is a + **hard error**, not a broken-and-reported image. A parent is load-bearing — + publishing under the wrong one, or silently under none, is worse than not + publishing at all. +- **Link and anchor resolution** needs no clamp at all. The index + (`internal/linkindex`) is built by walking *down* from the root, so nothing + outside it can ever be in the index, and a destination that would escape + simply isn't found there — the guarantee holds by construction rather than + by a check. See [Non-goals](guarantees.md#symlinks) for why the walk itself + cannot be tricked by a symlinked ancestor either. + +## Attachment identity + +An image's recorded `Source` — what its Confluence attachment name encodes, +and what `read`/`export` use to put it back where it came from — is relative +to the root, not to the page that references it (`_plans/026` commit 4). Two +pages at different depths referencing the same file now record the same +source and get the same attachment; before, each recorded the reference as +written, and the same file had two identities in Confluence. This is L3 +(`identity-from-asset-location`, `docs/guarantees.md`), and it's also why +moving a page's own images along with it now churns where it used to be +free — see the README's recipes for what that means in practice. + +## Link resolution + +`internal/linkindex.Build` walks the root's tree once, keying the page and +anchor maps by each file's path relative to the root rather than by bare +filename — so a link to `setup/overview.md` can't be satisfied by an unrelated +`overview.md` sharing that basename elsewhere in the tree (`_plans/025` +Scenario A). The index is built once per root and shared across every file +converted under it in the same command, which is also an ~80× performance +win at 400 files — rebuilding it per directory, per conversion, was an +accidental O(n²) (`_plans/025`'s measurement). + +A link that would resolve outside the root — `../../../../etc/passwd.md` — +isn't refused; it's simply never in the index, so it resolves the same way +any other unresolved link does: left exactly as written, and (since +`_plans/026` commit 5) reported. That's the minimal form of **R1** +(`report-unresolved-references`): a same-tree `.md`-shaped link that doesn't +resolve lands in the same warnings list an unresolved image already used. +It's not the dedicated diagnostic `_plans/025` gestures at (auditing a whole +tree without publishing, distinguishing *why* a reference failed) — that's +still open work, tracked loosely against a future `check` command. + +`create`'s reserve phase (`_plans/026` commit 8) is the other half of link +resolution: every file in a batch gets its Confluence id reserved — a +content-less stub — before any of them is converted, and each id is fed into +the shared index immediately (`Index.SetPage`), including under +`--no-persist`. That's what makes a link between two files in the same batch +resolve regardless of which direction it points, or whether the two link to +each other. + +## Multi-root batches are allowed + +A single invocation can span more than one project — nested +`markfluence.yaml` files, or files under entirely separate ones — and nothing +refuses this. Each file's root is discovered independently; a link across a +root boundary simply doesn't resolve (unresolved, not an error, same as any +other miss); a `parent:` escaping a file's own root is still a hard error even +when the target happens to be part of the same command under a *different* +root. This falls out of per-file discovery with no special-casing, the same +way `.editorconfig` and `tsconfig.json` resolution nest without needing to +forbid it. diff --git a/internal/attachfile/attachfile.go b/internal/attachfile/attachfile.go index 2eecf56..8cf7c57 100644 --- a/internal/attachfile/attachfile.go +++ b/internal/attachfile/attachfile.go @@ -67,11 +67,19 @@ type Options struct { // so decoding by default would scatter a literally-named file into a/b.png. An // attachment with no recorded source keeps its stored name. // -// This check is *lexical*. A source path may legitimately contain ".." -- an -// image in a directory above its page is a supported layout -- so ".." cannot -// simply be refused; the cleaned path is compared against root instead. -// Escaping is an error rather than a silent clip, because the path comes from an -// attachment comment, which anyone who can edit the page controls. +// This check is *lexical*, and by the time a root-relative model records a +// Source (025), the ".." this clamp refuses is never one markfluence itself +// would produce: an image whose resolved path climbed above the documentation +// root was refused as broken before it was ever recorded as an attachment, so +// a legitimate Source is already root-relative with no leading "..". What +// remains is a comment someone else wrote or mangled -- the attachment +// comment is server data, editable by anyone who can edit the page, so this +// guards against that rather than against a layout markfluence's own writer +// produces. The cleaned path is still compared against root, not refused for +// containing ".." outright, since a *stored* path may contain one for +// reasons that have nothing to do with the model here (a hand-crafted +// upload, an older markfluence's page-relative Source). Escaping is an error +// rather than a silent clip either way. // // Being lexical, it cannot see symlinks: if a directory inside root is a link // pointing out of it, a path that looks contained resolves elsewhere on disk. diff --git a/internal/client/client.go b/internal/client/client.go index 4cea889..9d78c0c 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -44,12 +44,22 @@ import ( // the image's original location exactly instead of inferring it from the // attachment name. const ( - // attachmentCommentPrefix marks an attachment as markfluence-managed. + // attachmentCommentPrefix marks an attachment as markfluence-managed. This + // is the only comment form markfluence writes or recognizes -- no older + // format is parsed. An attachment stamped by a markfluence predating a + // comment-format change (the Python tool's "mzcld:checksum:" prefix; this + // tool's own 64-hex checksum before it was truncated) reads as unmanaged + // and is re-uploaded once, the same as any other hand-uploaded file. attachmentCommentPrefix = "markfluence: " - // legacyChecksumPrefix is the older checksum-only comment form. It is still - // parsed -- comparing the parsed checksum rather than the whole comment is - // what lets the format change without re-uploading every attachment. - legacyChecksumPrefix = "mzcld:checksum: " + // checksumHexLen truncates a file's SHA-256 hex digest to 128 bits before + // it goes into a comment. The comment only needs to detect that a file's + // bytes changed, not resist an adversary, so this is the bigger and + // cheaper of the two levers on the 255-character comment ceiling -- + // truncating it buys roughly twice what shortening the "markfluence: " + // prefix would (#101), and the prefix is worth keeping full-length: it is + // the ownership marker S5 rests on, readable by a human browsing a page's + // attachments in the Confluence UI. + checksumHexLen = 32 ) const ( @@ -363,13 +373,11 @@ func attachmentComment(sum, source string) string { return c } -// parseAttachmentComment reads both the current form ("markfluence: sha256= -// path=") and the legacy checksum-only form, so an attachment written by -// an older markfluence is still recognized as unchanged. +// parseAttachmentComment reads the one form markfluence writes: +// "markfluence: sha256= path=". Anything else -- a hand-uploaded +// attachment, or one stamped by a markfluence predating this format -- comes +// back unmanaged. func parseAttachmentComment(comment string) AttachmentMeta { - if sum, ok := strings.CutPrefix(comment, legacyChecksumPrefix); ok { - return AttachmentMeta{SHA256: strings.TrimSpace(sum), Managed: true} - } rest, ok := strings.CutPrefix(comment, attachmentCommentPrefix) if !ok { return AttachmentMeta{} @@ -993,6 +1001,7 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt if err != nil { return nil, err } + sum = sum[:checksumHexLen] comment := attachmentComment(sum, att.Source) contentType := mime.TypeByExtension(filepath.Ext(att.Filename)) if contentType == "" { @@ -1010,6 +1019,9 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt case !ok: p.action = "created" case meta.SHA256 != sum: + // A stored checksum in a format markfluence no longer writes (a + // different length, or unmanaged entirely) never equals sum, so + // this also re-uploads once whatever predates the current format. p.action = "updated" case meta.Source != "" && meta.Source != att.Source: // The bytes are unchanged but the recorded path is wrong, so re-upload @@ -1017,13 +1029,10 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt // survive every later publish. The name is the encoding of the path, so // the two move together: a disagreement under the same name means the // stored comment does not say what we wrote. An empty Source is not a - // disagreement -- that is a legacy comment, and re-uploading every one - // of those is exactly the churn the checksum comparison avoids. + // disagreement -- a comment with no source recorded at all is a normal + // case (see attachmentComment), not something to treat as mangled. p.action = "updated" default: - // Compare the checksum, not the whole comment: an attachment stamped - // by an older markfluence is unchanged and must not be re-uploaded - // merely because the comment format has moved on. p.action = "skipped" } plans = append(plans, p) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 53c80dc..d61a9c1 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -499,29 +499,14 @@ func TestSyncAttachmentsCreatesWhenAbsent(t *testing.T) { } } -// A legacy comment still identifies an unchanged file, so a format change does -// not force a re-upload of every attachment. -func TestSyncAttachmentsSkipsWhenLegacyChecksumMatches(t *testing.T) { - path, sum := writeTempImage(t) - list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - legacyChecksumPrefix + sum + `"}}]}` - c, s := newServer(t, resp{200, list}) - actions, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}) - if err != nil { - t.Fatal(err) - } - if len(actions) != 1 || actions[0].Action != "skipped" { - t.Fatalf("actions = %v, want [skipped]", actions) - } - if !eqStrings(s.calls, []string{"GET"}) { - t.Errorf("calls = %v, want [GET] (no upload)", s.calls) - } -} - +// TestSyncAttachmentsUpdatesWhenChecksumDiffers also covers an attachment +// stamped by any format markfluence no longer writes: an unrecognized +// comment parses as unmanaged (empty SHA256), which never equals a real +// checksum, so it re-uploads once rather than being treated as unchanged. func TestSyncAttachmentsUpdatesWhenChecksumDiffers(t *testing.T) { path, _ := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - legacyChecksumPrefix + `stale"}}]}` + attachmentComment("stale", "") + `"}}]}` c, s := newServer(t, resp{200, list}, resp{200, `{}`}) actions, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}) if err != nil { @@ -539,8 +524,8 @@ func TestPlanAttachmentsClassifiesWithoutUploading(t *testing.T) { path, sum := writeTempImage(t) // same.png matches (skip), stale.png differs (update), new.png is absent (create). list := `{"results":[` + - `{"id":"a1","title":"same.png","metadata":{"comment":"` + legacyChecksumPrefix + sum + `"}},` + - `{"id":"a2","title":"stale.png","metadata":{"comment":"` + legacyChecksumPrefix + `stale"}}` + + `{"id":"a1","title":"same.png","metadata":{"comment":"` + attachmentComment(sum[:checksumHexLen], "") + `"}},` + + `{"id":"a2","title":"stale.png","metadata":{"comment":"` + attachmentComment("stale", "") + `"}}` + `]}` c, s := newServer(t, resp{200, list}) actions, err := c.PlanAttachments("1", []LocalAttachment{ @@ -901,6 +886,47 @@ func TestAttachmentCommentRecordsSource(t *testing.T) { } } +// TestSyncAttachmentsWritesATruncatedChecksum is #101's fix: the checksum +// recorded in a fresh comment is 128 bits (32 hex characters), not the full +// 256-bit digest -- the bigger, cheaper lever on the 255-character comment +// ceiling, since the comment only needs to detect a byte change, not resist +// an adversary. +func TestSyncAttachmentsWritesATruncatedChecksum(t *testing.T) { + path, sum := writeTempImage(t) + c, s := newServer(t, resp{200, `{"results":[]}`}, resp{200, `{}`}) + if _, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}); err != nil { + t.Fatal(err) + } + got := uploadParts(t, s)["comment"].value + if want := attachmentComment(sum[:checksumHexLen], ""); got != want { + t.Errorf("comment = %q, want %q", got, want) + } + n := len(strings.TrimPrefix(got, attachmentCommentPrefix+"sha256=")) + if n != checksumHexLen { + t.Errorf("recorded checksum is %d hex characters, want %d", n, checksumHexLen) + } +} + +// TestAttachmentCommentBudget pins the fixed overhead #101 measured: with the +// checksum truncated to 32 hex characters, "markfluence: sha256=<32> path=" +// costs 58 of the 255 characters Confluence allows, leaving 197 for the path +// itself -- up from 165 before the truncation. +func TestAttachmentCommentBudget(t *testing.T) { + const overhead = len("markfluence: ") + len("sha256=") + checksumHexLen + len(" path=") + if overhead != 58 { + t.Fatalf("overhead = %d, want 58", overhead) + } + sum := strings.Repeat("a", checksumHexLen) + longestFittingPath := strings.Repeat("p", 255-overhead) + if got := attachmentComment(sum, longestFittingPath); len(got) != 255 { + t.Errorf("comment length = %d, want exactly 255 at the boundary", len(got)) + } + tooLong := longestFittingPath + "x" + if got := attachmentComment(sum, tooLong); len(got) <= 255 { + t.Errorf("comment length = %d, want > 255 one character past the boundary", len(got)) + } +} + func TestParseAttachmentComment(t *testing.T) { cases := []struct { name string @@ -914,8 +940,6 @@ func TestParseAttachmentComment(t *testing.T) { // The path is written last and unquoted, so it may contain spaces. {"source with spaces", "markfluence: sha256=abc123 path=my docs/a b.png", AttachmentMeta{SHA256: "abc123", Source: "my docs/a b.png", Managed: true}}, - {"legacy form", legacyChecksumPrefix + "abc123", - AttachmentMeta{SHA256: "abc123", Managed: true}}, {"hand-uploaded", "a note from a human", AttachmentMeta{}}, {"empty", "", AttachmentMeta{}}, } @@ -1006,7 +1030,7 @@ func TestSyncAttachmentsLabelsTextPartsUTF8(t *testing.T) { t.Errorf("%s part Content-Type = %q, want a UTF-8 charset", name, got) } } - if want := attachmentComment(sum, source); parts["comment"].value != want { + if want := attachmentComment(sum[:checksumHexLen], source); parts["comment"].value != want { t.Errorf("comment part = %q, want %q", parts["comment"].value, want) } // The name rides in Content-Disposition, which was never affected; check it @@ -1035,19 +1059,20 @@ func TestSyncAttachmentsRestampsMangledSource(t *testing.T) { if len(actions) != 1 || actions[0].Action != "updated" { t.Fatalf("actions = %v, want [updated]", actions) } - if got := uploadParts(t, s)["comment"].value; got != attachmentComment(sum, "assets/probe-café.png") { + if got := uploadParts(t, s)["comment"].value; got != attachmentComment(sum[:checksumHexLen], "assets/probe-café.png") { t.Errorf("restamped comment = %q", got) } } -// TestSyncAttachmentsSkipsLegacyCommentWithNoSource pins the limit of that -// restamping: a legacy comment records no path at all, which is not a -// disagreement. Treating it as one would re-upload every attachment stamped by -// an older markfluence -- the churn the checksum comparison exists to avoid. -func TestSyncAttachmentsSkipsLegacyCommentWithNoSource(t *testing.T) { +// TestSyncAttachmentsSkipsCommentWithNoSourceRecorded pins that a comment +// recording no path at all -- attachmentComment omits "path=" when Source is +// empty -- is not treated as a disagreement with the local attachment's own +// (non-empty) Source. Only a *recorded* Source that disagrees is a mangled +// comment worth repairing. +func TestSyncAttachmentsSkipsCommentWithNoSourceRecorded(t *testing.T) { path, sum := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - legacyChecksumPrefix + sum + `"}}]}` + attachmentComment(sum[:checksumHexLen], "") + `"}}]}` c, s := newServer(t, resp{200, list}) actions, err := c.SyncAttachments("1", []LocalAttachment{ {Path: path, Filename: "x.png", Source: "assets/x.png"}, @@ -1063,12 +1088,10 @@ func TestSyncAttachmentsSkipsLegacyCommentWithNoSource(t *testing.T) { } } -// TestSyncAttachmentsSkipsWhenCurrentChecksumMatches is the new-format twin of the -// legacy skip test. func TestSyncAttachmentsSkipsWhenCurrentChecksumMatches(t *testing.T) { path, sum := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - attachmentComment(sum, "x.png") + `"}}]}` + attachmentComment(sum[:checksumHexLen], "x.png") + `"}}]}` c, s := newServer(t, resp{200, list}) actions, err := c.SyncAttachments("1", []LocalAttachment{ {Path: path, Filename: "x.png", Source: "x.png"}, diff --git a/internal/client/config.go b/internal/client/config.go index 952301c..ca2bf57 100644 --- a/internal/client/config.go +++ b/internal/client/config.go @@ -4,8 +4,11 @@ import ( "errors" "fmt" "os" + "path/filepath" "regexp" "strings" + + "github.com/mozilla/markfluence/internal/project" ) const ( @@ -28,8 +31,17 @@ type Options struct { // CloudID is the --cloud-id value; set it to route requests through the // platform API gateway, which a scoped service-account token requires. CloudID string - // EnvFile is the --env-file value; empty means the default ./.env. + // EnvFile is the --env-file value; empty means .env at the discovered + // project root (see loadEnvFile). EnvFile string + // Roots, when set, is the caller's own per-file project.Cache -- built + // before Resolve is called, and passed back in so the .env lookup's + // directory resolution reuses it instead of a second, independent + // Discover/os.OpenRoot pass. The ordinary case is the two passes landing + // on the same root; sharing the cache is what makes that cost one + // discovery instead of two. Resolve does not close anything found this + // way -- the cache still owns it, for the caller's later per-file work. + Roots *project.Cache } // Resolve builds a client from the site URL, username, cloud ID, and token. Each @@ -37,15 +49,16 @@ type Options struct { // the URL, username, and cloud ID come from opts when set, then // $CONFLUENCE_URL/$CONFLUENCE_USERNAME/$CONFLUENCE_CLOUD_ID, then the .env file; // the API token comes only from $CONFLUENCE_TOKEN, then .env -- never a flag. -// opts.EnvFile selects which .env is read: when empty the default ./.env is read -// best-effort (a missing file is fine); when set it's an explicit path that must +// opts.EnvFile selects which .env is read: when empty, .env at the discovered +// project root is read best-effort (a missing file is fine; see loadEnvFile +// for what "discovered" means here); when set it's an explicit path that must // be readable. It returns a friendly error listing whatever is missing. // // The cloud ID is optional: without one, requests go to the site domain exactly // as before, which is what an unscoped personal token and any Data Center site // need. func Resolve(opts Options) (*ConfluenceClient, error) { - env, err := loadEnvFile(opts.EnvFile) + env, err := loadEnvFile(opts.EnvFile, opts.Roots) if err != nil { return nil, err } @@ -105,10 +118,22 @@ func resolveValue(flagVal, envKey string, dotenv map[string]string) string { } // loadEnvFile resolves which .env to read and parses it. An explicit envFile -// (from --env-file) must be readable, so a read failure is an error. With no -// explicit path the default ./.env is best-effort: a missing file yields an -// empty map, matching the prior behavior. -func loadEnvFile(envFile string) (map[string]string, error) { +// (from --env-file) must be readable, so a read failure is an error, and it +// overrides everything below absolutely -- including roots. With no explicit +// path, .env is read from the project root -- the directory holding +// markfluence.yaml, found by walking up from the working directory, or the +// working directory itself when there is none. When roots is given (a +// caller's own per-file project.Cache, built before Resolve is called), its +// Resolve is used for that walk instead of a bare project.Discover call, so +// a caller with its own --root override applies it here too, and doesn't pay +// for a second discovery (and a second os.OpenRoot) of the identical root; +// roots owns closing the handle, so none happens here. This is its own +// discovery pass, separate from the per-file root the converter uses: it +// starts at the working directory rather than a markdown file's directory, +// 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. +func loadEnvFile(envFile string, roots *project.Cache) (map[string]string, error) { if envFile != "" { env, err := loadDotenv(envFile) if err != nil { @@ -116,9 +141,22 @@ func loadEnvFile(envFile string) (map[string]string, error) { } return env, nil } - env, err := loadDotenv(dotenvPath) + + 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() + } + } + + env, err := loadDotenv(filepath.Join(dir, dotenvPath)) if err != nil { - return map[string]string{}, nil // a missing ./.env is fine + return map[string]string{}, nil // a missing .env is fine } return env, nil } diff --git a/internal/client/config_test.go b/internal/client/config_test.go index 9337cba..5f1b339 100644 --- a/internal/client/config_test.go +++ b/internal/client/config_test.go @@ -5,6 +5,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/mozilla/markfluence/internal/project" ) // clearConfluenceEnv unsets the CONFLUENCE_* vars for a test so .env / flags are @@ -72,6 +74,80 @@ func TestResolveDefaultEnvFileMissingIsFine(t *testing.T) { } } +func TestResolveDefaultEnvFileFoundInCwdWithNoProjectFile(t *testing.T) { + clearConfluenceEnv(t) + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".env"), + []byte("CONFLUENCE_URL=https://wiki\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n"), 0o644); err != nil { + t.Fatal(err) + } + // No markfluence.yaml anywhere above dir, so discovery falls back to dir + // itself -- today's behavior, preserved. + t.Chdir(dir) + + c, err := Resolve(Options{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if c.BaseURL() != "https://wiki" { + t.Errorf("baseURL = %q, want https://wiki", c.BaseURL()) + } +} + +func TestResolveDefaultEnvFileFoundAtDiscoveredProjectRoot(t *testing.T) { + clearConfluenceEnv(t) + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "markfluence.yaml"), []byte("# marker\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) + } + sub := filepath.Join(root, "docs", "team") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + // No .env in the working directory itself -- only at the project root + // discovery finds by walking up. + t.Chdir(sub) + + c, err := Resolve(Options{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if c.BaseURL() != "https://wiki" { + t.Errorf("baseURL = %q, want https://wiki from the project root's .env", c.BaseURL()) + } +} + +// TestResolveRootsOverridesEnvDiscovery covers Options.Roots: when the caller +// passes its own --root-backed project.Cache, .env is read from that root, not +// from a plain upward walk from the working directory -- so a --root pointed +// at a different project also redirects which .env create/update/ +// attachment-upload read, matching the flag's stated meaning of overriding +// discovery for the whole invocation. +func TestResolveRootsOverridesEnvDiscovery(t *testing.T) { + clearConfluenceEnv(t) + cwd := t.TempDir() // no .env here + override := t.TempDir() + if err := os.WriteFile(filepath.Join(override, ".env"), + []byte("CONFLUENCE_URL=https://from-root\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n"), 0o644); err != nil { + t.Fatal(err) + } + t.Chdir(cwd) + + roots := project.NewCache(override) + defer roots.Close() + c, err := Resolve(Options{Roots: roots}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if c.BaseURL() != "https://from-root" { + t.Errorf("baseURL = %q, want https://from-root from --root's .env", c.BaseURL()) + } +} + func TestResolveCloudIDPrecedence(t *testing.T) { clearConfluenceEnv(t) path := writeEnvFile(t, diff --git a/internal/convert/aclink.go b/internal/convert/aclink.go index 0422cb0..75d8a49 100644 --- a/internal/convert/aclink.go +++ b/internal/convert/aclink.go @@ -14,6 +14,8 @@ package convert import ( "fmt" "strings" + + "github.com/mozilla/markfluence/internal/linkindex" ) // PageLinkTarget identifies the page an points at. Confluence names it @@ -104,10 +106,10 @@ func walkNodes(n *snode, fn func(*snode)) { // headingSlugs maps each heading's Confluence anchor to its GitHub one, which is // how a same-page recovers a markdown fragment. // -// The slug cannot be inverted -- confluenceSlug turns both a space and a hyphen -// into "-", so "DOM-Security-Team" could have come from either -- but the -// heading that produced it is in the document being converted, so the mapping is -// exact rather than guessed. +// The slug cannot be inverted -- linkindex.ConfluenceSlug turns both a space and +// a hyphen into "-", so "DOM-Security-Team" could have come from either -- but +// the heading that produced it is in the document being converted, so the +// mapping is exact rather than guessed. func headingSlugs(root *snode) map[string]string { out := map[string]string{} walkNodes(root, func(n *snode) { @@ -115,7 +117,7 @@ func headingSlugs(root *snode) map[string]string { return } if text := strings.TrimSpace(collapse(textContent(n))); text != "" { - out[confluenceSlug(text)] = githubSlug(text) + out[linkindex.ConfluenceSlug(text)] = linkindex.GithubSlug(text) } }) return out diff --git a/internal/convert/convert.go b/internal/convert/convert.go index c84221f..7758f40 100644 --- a/internal/convert/convert.go +++ b/internal/convert/convert.go @@ -7,11 +7,12 @@ package convert import ( "bytes" - "os" "path/filepath" "strings" "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/linkindex" + "github.com/mozilla/markfluence/internal/project" "github.com/yuin/goldmark" "github.com/yuin/goldmark/extension" "github.com/yuin/goldmark/parser" @@ -53,28 +54,29 @@ func newMarkdown(r *storageRenderer) goldmark.Markdown { // MdToConfluence converts a markdown file's body to Confluence storage-format // HTML. baseURL and spaceKey build the Confluence URLs that internal document // links point at; md.Filename locates sibling files for link/anchor rewriting -// and resolves image paths. version is the build stamp substituted for the +// and resolves image paths. root bounds which images and parent references may +// be read (S1/S2) and is what an image's recorded Source is relative to; the +// caller discovers it (per-file, via internal/project) rather than +// MdToConfluence assuming the working directory. index is the tree-wide +// link/anchor index for root -- built once and shared across every file +// converted under it (internal/linkindex.Build), not rebuilt here per +// conversion. version is the build stamp substituted for the // token. -func MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version string) (*ConfluencePage, error) { +func MdToConfluence( + md *frontmatter.MarkdownFile, root *project.Root, index *linkindex.Index, baseURL, spaceKey, version string, +) (*ConfluencePage, error) { // Shield raw ac:/ri: storage tags so goldmark passes them through instead of // escaping them; restore them after rendering. shielded, unshield := shieldStorage(md.Body) dir := filepath.Dir(md.Filename) - // The documentation root is the working directory: markfluence is run from - // the root of a documentation tree. An unresolvable cwd disables the check - // rather than failing the conversion. - root, err := os.Getwd() - if err != nil { - root = "" - } r := &storageRenderer{ baseDir: dir, root: root, currentBasename: filepath.Base(md.Filename), + currentDocKey: DocKeyFor(root, md.Filename), baseURL: baseURL, spaceKey: spaceKey, - anchorMap: buildAnchorMap(dir), - pageMap: buildPageMap(dir), + index: index, } var buf bytes.Buffer if err := newMarkdown(r).Convert([]byte(shielded), &buf); err != nil { @@ -102,3 +104,38 @@ func MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version str } return page, nil } + +// DocKeyFor resolves filename's own path to the key the link index would use +// for it: relative to root, slash-separated. filename is always at or under +// its own root by construction (root was discovered from this same file's +// directory), so this cannot escape the way an arbitrary link destination +// could; the fallback (filename's bare basename) only matters if filepath.Abs +// or filepath.Rel itself fails, which needs an unreadable working directory. +// +// Exported so a caller seeding the link index directly -- create's reserve +// phase, injecting an id that exists only in memory before publish -- computes +// the identical key MdToConfluence would, rather than a second copy that could +// silently drift from it. +func DocKeyFor(root *project.Root, filename string) string { + abs, err := filepath.Abs(filename) + if err != nil { + return filepath.Base(filename) + } + key, ok := rootRelativeKey(root, abs) + if !ok { + return filepath.Base(filename) + } + return key +} + +// rootRelativeKey converts an already-absolute path into the root-relative, +// slash-separated form the link index is keyed by -- the one step DocKeyFor +// and (*storageRenderer).resolveDocKey (links.go) share; each computes abs +// its own way and picks its own fallback on failure, but not this one. +func rootRelativeKey(root *project.Root, abs string) (key string, ok bool) { + rel, err := filepath.Rel(root.Dir, abs) + if err != nil { + return "", false + } + return filepath.ToSlash(rel), true +} diff --git a/internal/convert/images.go b/internal/convert/images.go index 50eb2fa..9b8a03f 100644 --- a/internal/convert/images.go +++ b/internal/convert/images.go @@ -39,44 +39,75 @@ func (r *storageRenderer) renderImage( } alt := nodeText(node, source) attrs := r.parseImageTitle(string(n.Title), src) - // src is a URL; fsPath is the file it names. Everything touching the - // filesystem -- and the attachment name derived from it -- uses fsPath. The - // broken messages stay on src so they echo what the author wrote. Decoding - // before withinRoot is what keeps an encoded "..%2F" from slipping past it. - fsPath := decodeDestination(src) - switch { - case isRemoteURL(src): + // Remote and unsupported-extension images are decided on src/fsPath alone + // and never touch the filesystem -- checked and returned before any of the + // root/Lstat work below, so a remote URL costs no syscalls just because + // its "path" happens to parse. + if isRemoteURL(src) { _, _ = w.WriteString(acImage(alt, attrs, "", src)) - - case !supportedImageExts[strings.ToLower(filepath.Ext(fsPath))]: + return ast.WalkSkipChildren, nil + } + // src is a URL; fsPath is the file it names. Everything touching the + // filesystem -- and the attachment name derived from it -- uses the path + // relative to root, not fsPath itself. The broken messages stay on src so + // they echo what the author wrote. Decoding before computing that relative + // path is what keeps an encoded "..%2F" from slipping past the root check. + fsPath := decodeDestination(src) + if !supportedImageExts[strings.ToLower(filepath.Ext(fsPath))] { msg := fmt.Sprintf("IMAGE BROKEN: %s (unsupported type)", src) r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) + return ast.WalkSkipChildren, nil + } + + rootRel, insideRoot := rootRelative(r.root.Dir, r.baseDir, fsPath) - case !r.withinRoot(filepath.Join(r.baseDir, fsPath)): + var info os.FileInfo + var lstatErr error + if insideRoot { + info, lstatErr = r.root.FS.Lstat(rootRel) + } + // An escape only os.Root can see -- a symlinked intermediate directory -- + // is folded into the same "outside" case as a lexically escaping path, + // wrapped the way internal/attachfile already wraps it: os.Root's bare + // error names neither the image nor the reason. Every other boolean below + // is guarded by insideRoot too, not just lstatErr == nil -- Lstat is never + // called at all when insideRoot is false, so lstatErr and info both sit at + // their zero values (nil), and "lstatErr == nil" alone would read as + // "Lstat succeeded" when it really means "Lstat was never asked." + escapesRoot := !insideRoot || (lstatErr != nil && strings.Contains(lstatErr.Error(), "escapes from parent")) + notFound := insideRoot && lstatErr != nil && !escapesRoot + isSymlink := insideRoot && lstatErr == nil && info.Mode()&os.ModeSymlink != 0 + notRegular := insideRoot && lstatErr == nil && !isSymlink && !info.Mode().IsRegular() + + switch { + case escapesRoot: msg := fmt.Sprintf("IMAGE BROKEN: %s (outside the documentation root)", src) r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) - case !isFile(filepath.Join(r.baseDir, fsPath)): + case notFound, notRegular: + // Not found, or something that was never a file to begin with (a + // directory, say) -- both read the same as "not found" always has. msg := fmt.Sprintf("IMAGE BROKEN: %s (not found)", src) r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) + case isSymlink: + msg := fmt.Sprintf("IMAGE BROKEN: %s (symlink, not a regular file)", src) + r.broken = append(r.broken, msg) + _, _ = w.WriteString(html.EscapeString(msg)) + default: - filename := AttachmentFilename(fsPath) + filename := AttachmentFilename(rootRel) if !r.seen[filename] { if r.seen == nil { r.seen = map[string]bool{} } r.seen[filename] = true - abs, err := filepath.Abs(filepath.Join(r.baseDir, fsPath)) - if err != nil { - abs = filepath.Join(r.baseDir, fsPath) - } r.attachments = append(r.attachments, Attachment{ - Filename: filename, Path: abs, Source: normalizeSrc(fsPath), + Filename: filename, Path: filepath.Join(r.root.Dir, rootRel), Source: rootRel, }) } _, _ = w.WriteString(acImage(alt, attrs, filename, "")) @@ -177,34 +208,30 @@ func isRemoteURL(src string) bool { strings.HasPrefix(src, "//") } -func isFile(path string) bool { - info, err := os.Stat(path) - return err == nil && !info.IsDir() -} - -// withinRoot reports whether an image path resolves inside the documentation -// root. markfluence is meant to be run from the root of a documentation tree, so -// an image above it -- "../../../secrets/x.png" -- is a mistake rather than a -// shared asset, and is reported broken instead of published. +// rootRelative resolves an image destination (fsPath, already decoded and +// still relative to baseDir -- the referencing file's own directory) to a path +// relative to root, in slash form for recording as an attachment Source and +// for passing to root.FS. ok is false when the result climbs above root, which +// the caller reports as broken rather than ever asking root.FS about it. // -// A path at or below the root is fine, including one reached via ".." from a -// page in a subdirectory: "../assets/logo.png" from docs/guide/foo.md is the -// ordinary shared-assets layout. The check fails open when the root is unknown -// or a path cannot be resolved -- it is an authoring guard, not a security -// boundary. -func (r *storageRenderer) withinRoot(p string) bool { - if r.root == "" { - return true - } - abs, err := filepath.Abs(p) +// A path at or below root is fine, including one reached via ".." from a page +// in a subdirectory: "../assets/logo.png" from docs/guide/foo.md is the +// ordinary shared-assets layout, and resolves to "assets/logo.png" once +// docs/guide's ".." cancels out -- not to something climbing past root itself. +func rootRelative(root, baseDir, fsPath string) (rel string, ok bool) { + abs, err := filepath.Abs(filepath.Join(baseDir, fsPath)) if err != nil { - return true + return "", false } - rel, err := filepath.Rel(r.root, abs) + r, err := filepath.Rel(root, abs) if err != nil { - return true + return "", false + } + r = filepath.ToSlash(r) + if r == ".." || strings.HasPrefix(r, "../") { + return "", false } - return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) + return r, true } func isDigits(s string) bool { diff --git a/internal/convert/links.go b/internal/convert/links.go index 6896998..c2651fe 100644 --- a/internal/convert/links.go +++ b/internal/convert/links.go @@ -3,129 +3,14 @@ package convert import ( "fmt" "net/url" - "os" "path/filepath" - "regexp" "strings" - "github.com/mozilla/markfluence/internal/frontmatter" "github.com/yuin/goldmark/ast" gmhtml "github.com/yuin/goldmark/renderer/html" "github.com/yuin/goldmark/util" ) -// pageEntry is a sibling document's Confluence coordinates for link rewriting. -type pageEntry struct { - pageID string - title string -} - -var ( - // nonSlugRE strips everything except letters, digits, underscore, whitespace, - // and hyphens (Unicode-aware). - nonSlugRE = regexp.MustCompile(`[^\p{L}\p{N}_\s-]`) - whitespaceRE = regexp.MustCompile(`\s`) - whitespaceRunRE = regexp.MustCompile(`\s+`) -) - -// githubSlug replicates GitHub's heading-anchor slugger: lowercase; strip all but -// letters/digits/underscore/whitespace/hyphen; each whitespace char becomes one -// hyphen; trim leading/trailing hyphens. -func githubSlug(heading string) string { - s := strings.ToLower(heading) - s = nonSlugRE.ReplaceAllString(s, "") - s = whitespaceRE.ReplaceAllString(s, "-") - return strings.Trim(s, "-") -} - -// confluenceSlug replicates Confluence's scheme: preserve case and punctuation, -// collapsing runs of whitespace to single hyphens. -func confluenceSlug(heading string) string { - return whitespaceRunRE.ReplaceAllString(strings.TrimSpace(heading), "-") -} - -// extractHeadings returns the text of each ATX heading in a frontmatter-stripped -// body, skipping fenced code blocks so "#" lines inside samples aren't headings. -func extractHeadings(body string) []string { - var headings []string - inCode := false - for _, line := range strings.Split(body, "\n") { - if strings.HasPrefix(line, "```") { - inCode = !inCode - continue - } - if inCode { - continue - } - hashes := 0 - for hashes < len(line) && line[hashes] == '#' { - hashes++ - } - if hashes == 0 || hashes >= len(line) { - continue - } - rest := line[hashes:] - if strings.TrimLeft(rest, " \t") == rest { // no whitespace after the #s - continue - } - if text := strings.TrimSpace(rest); text != "" { - headings = append(headings, text) - } - } - return headings -} - -// buildAnchorMap maps each *.md file in dir to its {githubSlug: confluenceSlug}. -func buildAnchorMap(dir string) map[string]map[string]string { - out := map[string]map[string]string{} - entries, err := os.ReadDir(dir) - if err != nil { - return out - } - for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".md") { - continue - } - data, err := os.ReadFile(filepath.Join(dir, e.Name())) - if err != nil { - continue - } - _, body := frontmatter.Extract(string(data)) - anchors := map[string]string{} - for _, h := range extractHeadings(body) { - if gh := githubSlug(h); gh != "" { - anchors[gh] = confluenceSlug(h) - } - } - out[e.Name()] = anchors - } - return out -} - -// buildPageMap maps each *.md file in dir with a usable page_id to its -// Confluence coordinates. -func buildPageMap(dir string) map[string]pageEntry { - out := map[string]pageEntry{} - entries, err := os.ReadDir(dir) - if err != nil { - return out - } - for _, e := range entries { - if e.IsDir() || !strings.HasSuffix(e.Name(), ".md") { - continue - } - data, err := os.ReadFile(filepath.Join(dir, e.Name())) - if err != nil { - continue - } - mf := frontmatter.Parse(e.Name(), string(data)) - if id := mf.PageID(); id != "" { - out[e.Name()] = pageEntry{pageID: id, title: mf.Title()} - } - } - return out -} - // renderLink renders a link, rewriting same-page and cross-file anchors to // Confluence ids and sibling .md links to their published Confluence URLs. Links // it does not rewrite fall back to the default goldmark rendering. @@ -162,7 +47,7 @@ func (r *storageRenderer) rewriteHref(href string) (string, bool) { rewritten := false if strings.HasPrefix(href, "#") { - if nf := r.anchorMap[r.currentBasename][decodeDestination(href[1:])]; nf != "" { + if nf, ok := r.index.Anchor(r.currentDocKey, decodeDestination(href[1:])); ok { // Same-page anchors become fake cross-file links to the current // file so the doc-link step can fully qualify them. The filename is // encoded going in because what is being built here is a @@ -172,7 +57,7 @@ func (r *storageRenderer) rewriteHref(href string) (string, bool) { rewritten = true } } else if path, frag, ok := splitMarkdownAnchor(href); ok { - if nf := r.anchorMap[docKey(path)][decodeDestination(frag)]; nf != "" { + if nf, ok := r.index.Anchor(r.resolveDocKey(path), decodeDestination(frag)); ok { // path keeps the spelling it was written with: it is a destination, // and the doc-link step decodes it again for its own lookup. href = path + "#" + escapeFragment(nf) @@ -188,7 +73,11 @@ func (r *storageRenderer) rewriteHref(href string) (string, bool) { // rewriteDocLink rewrites a sibling .md href (with optional fragment) to its // Confluence URL. It returns ok=false for absolute URLs, non-.md hrefs, or files -// not in the page map. +// not in the link index -- the last one warns (minimal R1: every reference that +// looks like it should resolve and doesn't is said out loud, via the same +// r.warnings list images.go already populates on a broken reference). The first +// two don't warn, because they were never meant to resolve here in the first +// place -- a mention, an attachment link, or an external URL. func (r *storageRenderer) rewriteDocLink(href string) (string, bool) { path, fragment := href, "" if i := strings.Index(href, "#"); i >= 0 { @@ -200,32 +89,44 @@ func (r *storageRenderer) rewriteDocLink(href string) (string, bool) { if strings.Contains(path, "://") || strings.HasPrefix(path, "//") { return "", false } - entry, ok := r.pageMap[docKey(path)] + entry, ok := r.index.Page(r.resolveDocKey(path)) if !ok { + r.warnings = append(r.warnings, fmt.Sprintf("link not resolved: %s", href)) return "", false } var newHref string if r.spaceKey != "" { slug := "" - if entry.title != "" { - slug = url.QueryEscape(entry.title) + if entry.Title != "" { + slug = url.QueryEscape(entry.Title) } newHref = fmt.Sprintf("%s/wiki/spaces/%s/pages/%s/%s", - r.baseURL, r.spaceKey, entry.pageID, slug) + r.baseURL, r.spaceKey, entry.PageID, slug) } else { - newHref = fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", r.baseURL, entry.pageID) + newHref = fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", r.baseURL, entry.PageID) } return newHref + fragment, true } -// docKey turns a link destination into the key the page and anchor maps are -// built with: the bare filename as it appears on disk. Both maps are keyed by -// os.ReadDir's e.Name(), so the destination has to be decoded first -- a link -// to "my doc.md" is spelled "my%20doc.md", and comparing that to the filename -// silently misses, publishing a relative href that is a dead link on Confluence. -func docKey(dest string) string { - return filepath.Base(decodeDestination(dest)) +// resolveDocKey resolves a link/anchor destination -- relative to r.baseDir, +// the referencing file's own directory, same as an image src -- to the +// root-relative, slash-separated path the link index is keyed by. +// +// An escaping destination (one that climbs above root) still returns its +// computed string rather than refusing it: the index is built by walking +// downward from root, so it can never contain an entry for a path outside it, +// and an escaping key is therefore already guaranteed to miss. That is what +// lets link resolution need no clamp at all (025's S2 discussion) where an +// image leaf and a parent: read still need one -- both of those are reads, +// and this is a lookup against data already collected inside root. +func (r *storageRenderer) resolveDocKey(dest string) string { + abs, err := filepath.Abs(filepath.Join(r.baseDir, decodeDestination(dest))) + if err != nil { + return "" + } + key, _ := rootRelativeKey(r.root, abs) + return key } // splitMarkdownAnchor splits "path.md#fragment" into its path and fragment, diff --git a/internal/convert/links_test.go b/internal/convert/links_test.go index cb9af41..58f8775 100644 --- a/internal/convert/links_test.go +++ b/internal/convert/links_test.go @@ -1,24 +1,33 @@ package convert -import "testing" - -// TestDocKey covers the lookup key the page and anchor maps are consulted with. -// Both are keyed by os.ReadDir's e.Name(), so a destination has to be decoded -// down to a bare filename before it will match. Getting this wrong is silent: -// the link is simply not rewritten, and a relative href that means nothing on -// Confluence is published with no warning. -func TestDocKey(t *testing.T) { +import ( + "path/filepath" + "testing" + + "github.com/mozilla/markfluence/internal/project" +) + +// TestResolveDocKeyDecodesBeforeResolving covers the lookup key the link index +// is consulted with. The index is keyed by root-relative path as it appears on +// disk, so a destination has to be decoded down to that spelling before it will +// match. Getting this wrong is silent: the link is simply not rewritten, and a +// relative href that means nothing on Confluence is published -- though now, at +// least, with a warning (minimal R1). +func TestResolveDocKeyDecodesBeforeResolving(t *testing.T) { + root := t.TempDir() + r := &storageRenderer{baseDir: root, root: &project.Root{Dir: root}} + cases := []struct { dest string key string }{ {"plain.md", "plain.md"}, - {"docs/plain.md", "plain.md"}, - {"../plain.md", "plain.md"}, + {"docs/plain.md", "docs/plain.md"}, + {"../plain.md", "../plain.md"}, // escapes root -- returned as-is, not refused // The bug: a filename with a space is spelled with "%20". {"my%20doc.md", "my doc.md"}, - {"docs/my%20doc.md", "my doc.md"}, + {"docs/my%20doc.md", "docs/my doc.md"}, {"./my%20doc.md", "my doc.md"}, // Non-ASCII filenames encode the same way. @@ -33,25 +42,69 @@ func TestDocKey(t *testing.T) { {"my%2520doc.md", "my%20doc.md"}, } for _, c := range cases { - if got := docKey(c.dest); got != c.key { - t.Errorf("docKey(%q) = %q, want %q", c.dest, got, c.key) + if got := r.resolveDocKey(c.dest); got != c.key { + t.Errorf("resolveDocKey(%q) = %q, want %q", c.dest, got, c.key) } } } -// TestDocKeyAgreesOnBothSpellings is the property that matters more than any -// single mapping: the two legal ways to write a destination containing a space -// have to reach the same entry, or one of them silently fails to resolve. -func TestDocKeyAgreesOnBothSpellings(t *testing.T) { +// TestResolveDocKeyAgreesOnBothSpellings is the property that matters more +// than any single mapping: the two legal ways to write a destination +// containing a space have to reach the same entry, or one of them silently +// fails to resolve. +func TestResolveDocKeyAgreesOnBothSpellings(t *testing.T) { + root := t.TempDir() + r := &storageRenderer{baseDir: root, root: &project.Root{Dir: root}} + for _, pair := range [][2]string{ {"my%20doc.md", "my doc.md"}, {"docs/my%20doc.md", "docs/my doc.md"}, {"caf%C3%A9.md", "café.md"}, } { - encoded, literal := docKey(pair[0]), docKey(pair[1]) + encoded, literal := r.resolveDocKey(pair[0]), r.resolveDocKey(pair[1]) if encoded != literal { - t.Errorf("docKey(%q) = %q but docKey(%q) = %q; both spellings must agree", + t.Errorf("resolveDocKey(%q) = %q but resolveDocKey(%q) = %q; both spellings must agree", pair[0], encoded, pair[1], literal) } } } + +// TestResolveDocKeyDistinguishesSameBasenameInDifferentDirectories is +// Scenario A's fix, at the resolveDocKey level: two links spelled +// "overview.md" from two different directories must resolve to two different +// keys, one per directory -- not collide on the bare filename the way the old +// basename-only docKey did. +func TestResolveDocKeyDistinguishesSameBasenameInDifferentDirectories(t *testing.T) { + root := t.TempDir() + + fromRoot := &storageRenderer{baseDir: root, root: &project.Root{Dir: root}} + fromSub := &storageRenderer{baseDir: filepath.Join(root, "setup"), root: &project.Root{Dir: root}} + + top := fromRoot.resolveDocKey("overview.md") + nested := fromSub.resolveDocKey("overview.md") + if top == nested { + t.Errorf("resolveDocKey(overview.md) from two directories collided on %q", top) + } + if want := "overview.md"; top != want { + t.Errorf("top-level resolveDocKey = %q, want %q", top, want) + } + if want := "setup/overview.md"; nested != want { + t.Errorf("nested resolveDocKey = %q, want %q", nested, want) + } +} + +// TestResolveDocKeyFollowsUpAndAcrossDirectories covers link direction: up +// from a subdirectory to a sibling of root, and back down into another +// subdirectory -- both must land on the same root-relative key a sibling file +// would resolve to from its own directory. +func TestResolveDocKeyFollowsUpAndAcrossDirectories(t *testing.T) { + root := t.TempDir() + fromTeam := &storageRenderer{baseDir: filepath.Join(root, "team"), root: &project.Root{Dir: root}} + + if got, want := fromTeam.resolveDocKey("../index.md"), "index.md"; got != want { + t.Errorf("resolveDocKey(../index.md) = %q, want %q", got, want) + } + if got, want := fromTeam.resolveDocKey("../ops/runbook.md"), "ops/runbook.md"; got != want { + t.Errorf("resolveDocKey(../ops/runbook.md) = %q, want %q", got, want) + } +} diff --git a/internal/convert/regression_test.go b/internal/convert/regression_test.go index b2997c8..403433d 100644 --- a/internal/convert/regression_test.go +++ b/internal/convert/regression_test.go @@ -13,6 +13,8 @@ import ( "github.com/mozilla/markfluence/internal/convert" "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/linkindex" + "github.com/mozilla/markfluence/internal/project" ) // update, when set (`go test ./internal/convert -run TestRegression -update`, @@ -62,18 +64,44 @@ type caseConfig struct { filename string baseURL string spaceKey string - files []string + // root is a path relative to caseDir naming the documentation root, e.g. + // "." for the case directory itself. Empty means unset -- the default + // (no project file) then applies: the root is the primary file's own + // directory, exactly as it would be for a real file with no + // markfluence.yaml above it. + root string + files []string } // runCase resolves a case's config, runs the converter, and returns the golden bytes. func runCase(t *testing.T, caseDir string) []byte { cfg := loadConfig(t, caseDir) - md, err := frontmatter.ParseFile(filepath.Join(caseDir, cfg.filename)) + mdPath := filepath.Join(caseDir, cfg.filename) + md, err := frontmatter.ParseFile(mdPath) if err != nil { t.Fatalf("parsing primary file: %v", err) } + + rootDir := filepath.Dir(mdPath) + if cfg.root != "" { + rootDir = filepath.Join(caseDir, cfg.root) + } + rootDir, err = filepath.Abs(rootDir) + if err != nil { + t.Fatalf("resolving root: %v", err) + } + root, err := project.FromPath(rootDir) + if err != nil { + t.Fatalf("building root: %v", err) + } + t.Cleanup(func() { _ = root.FS.Close() }) + index, err := linkindex.Build(root) + if err != nil { + t.Fatalf("building link index: %v", err) + } + // A fixed version stamp keeps goldens deterministic; no case uses the token. - page, err := convert.MdToConfluence(md, cfg.baseURL, cfg.spaceKey, "markfluence vtest") + page, err := convert.MdToConfluence(md, root, index, cfg.baseURL, cfg.spaceKey, "markfluence vtest") if err != nil { t.Fatalf("MdToConfluence: %v", err) } @@ -93,6 +121,7 @@ func loadConfig(t *testing.T, caseDir string) caseConfig { } mustUnmarshal(t, raw, "filename", &cfg.filename) mustUnmarshal(t, raw, "base_url", &cfg.baseURL) + mustUnmarshal(t, raw, "root", &cfg.root) if v, ok := raw["space_key"]; ok { // Present but possibly JSON null (no resolvable space key). var s *string diff --git a/internal/convert/renderer.go b/internal/convert/renderer.go index 3188376..e415323 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -5,6 +5,8 @@ import ( "fmt" "strings" + "github.com/mozilla/markfluence/internal/linkindex" + "github.com/mozilla/markfluence/internal/project" "github.com/yuin/goldmark/ast" "github.com/yuin/goldmark/renderer" gmhtml "github.com/yuin/goldmark/renderer/html" @@ -21,16 +23,24 @@ import ( type storageRenderer struct { baseDir string - // root bounds which images may be published: the documentation root, which - // markfluence is expected to be run from. Empty disables the check. - root string + // root bounds which images and parent references may be read (S1/S2): the + // documentation root, discovered per file rather than assumed to be the + // working directory. Every attachment's Source is recorded relative to it. + root *project.Root // Link/anchor rewriting context, populated per conversion. + // + // currentBasename is the bare filename, used to build a same-page anchor's + // fake self-link; currentDocKey is the same file's root-relative path, used + // to look up its own anchors in index -- the two differ by more than a + // leading directory whenever the file isn't at the index's root. currentBasename string + currentDocKey string baseURL string spaceKey string - anchorMap map[string]map[string]string // filename -> github slug -> confluence slug - pageMap map[string]pageEntry // filename -> page id + title + // index is the tree-wide link/anchor index, built once per root and shared + // across every file converted under it (internal/linkindex). + index *linkindex.Index // Image side effects. attachments []Attachment diff --git a/internal/convert/storage_to_md.go b/internal/convert/storage_to_md.go index 5e5ffac..7218591 100644 --- a/internal/convert/storage_to_md.go +++ b/internal/convert/storage_to_md.go @@ -326,6 +326,13 @@ var alignSeparators = map[string]string{ // attribute. var textAlignRE = regexp.MustCompile(`(?i)text-align\s*:\s*([a-z]+)`) +// whitespaceRunRE collapses a run of whitespace to a single space in collapse +// below. Not the same concern as linkindex's identically-shaped regexp (that +// one collapses a run to a hyphen, for a Confluence anchor slug); duplicated +// rather than imported, since sharing it would couple this file's general +// text-collapsing to an unrelated package over one regexp literal. +var whitespaceRunRE = regexp.MustCompile(`\s+`) + // columnSeparators builds the delimiter row, recovering each column's alignment // from its cells. // diff --git a/internal/convert/storage_to_md_test.go b/internal/convert/storage_to_md_test.go index 6a77f59..c25a024 100644 --- a/internal/convert/storage_to_md_test.go +++ b/internal/convert/storage_to_md_test.go @@ -107,7 +107,8 @@ func TestRoundTripStableCallouts(t *testing.T) { }, "\n") + "\n" md := frontmatter.Parse("main.md", src) - page, err := convert.MdToConfluence(md, "https://wiki.example.net", "ENG", "vtest") + root := testRoot(t, "") + page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { t.Fatalf("MdToConfluence: %v", err) } @@ -137,7 +138,8 @@ func TestRoundTripTableAlignment(t *testing.T) { }, "\n") + "\n" md := frontmatter.Parse("main.md", src) - page, err := convert.MdToConfluence(md, "https://wiki.example.net", "ENG", "vtest") + root := testRoot(t, "") + page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { t.Fatalf("MdToConfluence: %v", err) } @@ -184,7 +186,8 @@ func TestRoundTripPassthrough(t *testing.T) { t.Fatalf("reading golden: %v", err) } md := frontmatter.Parse("main.md", string(src)) - page, err := convert.MdToConfluence(md, "https://wiki.example.net", "ENG", "vtest") + root := testRoot(t, "") + page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { t.Fatalf("MdToConfluence: %v", err) } diff --git a/internal/convert/symlink_test.go b/internal/convert/symlink_test.go new file mode 100644 index 0000000..2cf758f --- /dev/null +++ b/internal/convert/symlink_test.go @@ -0,0 +1,99 @@ +package convert_test + +// Symlink refusal at the image leaf, and the os.Root backstop for an escape +// through a symlinked intermediate directory. Built programmatically rather +// than as checked-in golden fixtures -- a checked-in symlink is fragile across +// platforms and git's core.symlinks setting -- mirroring how +// internal/attachfile tests the same class of escape. + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/convert" + "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/project" +) + +func skipIfNoSymlinks(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevated privileges on Windows") + } +} + +func TestRenderImageRefusesSymlinkedLeaf(t *testing.T) { + skipIfNoSymlinks(t) + root := t.TempDir() + outside := t.TempDir() + target := filepath.Join(outside, "real.png") + if err := os.WriteFile(target, []byte("PNG"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "logo.png") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + md := frontmatter.Parse(filepath.Join(root, "main.md"), "![logo](logo.png)\n") + r, err := project.FromPath(root) + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.FS.Close() }() + + idx := testIndex(t, r) + page, err := convert.MdToConfluence(md, r, idx, "https://wiki.example.net", "ENG", "vtest") + if err != nil { + t.Fatalf("MdToConfluence: %v", err) + } + if len(page.Attachments) != 0 { + t.Errorf("a symlinked leaf must not become an attachment, got %v", page.Attachments) + } + if len(page.Broken) != 1 || !strings.Contains(page.Broken[0], "symlink") { + t.Errorf("broken = %v, want one entry naming a symlink", page.Broken) + } + if strings.Contains(page.HTML, "ri:attachment") { + t.Errorf("published body references an attachment for a refused symlink:\n%s", page.HTML) + } +} + +func TestRenderImageRefusesEscapeThroughSymlinkedDirectory(t *testing.T) { + skipIfNoSymlinks(t) + root := t.TempDir() + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "logo.png"), []byte("PNG"), 0o644); err != nil { + t.Fatal(err) + } + // "assets" looks like an ordinary subdirectory of root; it is actually a + // link leading outside it. A lexical check (filepath.Rel on root vs. + // root/assets/logo.png) sees this as contained -- only os.Root, which + // resolves the symlink and refuses one leading outside its scope, catches + // it. This is the backstop 025 assigns to os.Root, distinct from the leaf + // refusal above. + if err := os.Symlink(outside, filepath.Join(root, "assets")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + md := frontmatter.Parse(filepath.Join(root, "main.md"), "![logo](assets/logo.png)\n") + r, err := project.FromPath(root) + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.FS.Close() }() + + idx := testIndex(t, r) + page, err := convert.MdToConfluence(md, r, idx, "https://wiki.example.net", "ENG", "vtest") + if err != nil { + t.Fatalf("MdToConfluence: %v", err) + } + if len(page.Attachments) != 0 { + t.Errorf("an escape through a symlinked directory must not become an attachment, got %v", page.Attachments) + } + if len(page.Broken) != 1 || !strings.Contains(page.Broken[0], "outside the documentation root") { + t.Errorf("broken = %v, want one entry naming the escape", page.Broken) + } +} diff --git a/internal/convert/testdata/regression/doc-links-encoded/test.output b/internal/convert/testdata/regression/doc-links-encoded/test.output index e295c7d..ed76934 100644 --- a/internal/convert/testdata/regression/doc-links-encoded/test.output +++ b/internal/convert/testdata/regression/doc-links-encoded/test.output @@ -2,5 +2,7 @@ "attachments": [], "broken": [], "html": "

Encoded Doc Links

\n

A link destination is a URL, so a sibling whose filename contains a space has to be percent-encoded to be linked at all. This is the spelling editors and previews produce, and it must resolve to the same page as the angle-bracket form below:

\n

percent-encoded

\n

angle brackets

\n

A bare space is not a valid destination, so this is not a link at all and stays literal text -- the same as GitHub and a local preview render it:

\n

[bare space](my sibling.md)

\n

The fragment is a URL too. An encoded anchor has to be decoded before it can be matched against a heading slug, which is Unicode-aware:

\n

encoded fragment

\n

literal fragment

\n

A same-page anchor takes the same path through the anchor map:

\n

same page, encoded

\n

same page, literal

\n

Both halves at once -- encoded filename and encoded fragment:

\n

both encoded

\n

A sibling that is not in this set is left exactly as written, still encoded, because nothing was resolved and nothing should be rewritten:

\n

unknown

\n

An absolute URL keeps its encoding untouched:

\n

external

\n

Café Section

\n

Content under a heading whose slug is not ASCII.

\n", - "warnings": [] + "warnings": [ + "link not resolved: nosuch%20file.md" + ] } diff --git a/internal/convert/testdata/regression/images-shared-parent/test.input b/internal/convert/testdata/regression/images-shared-parent/test.input index e72fb1d..685b2b0 100644 --- a/internal/convert/testdata/regression/images-shared-parent/test.input +++ b/internal/convert/testdata/regression/images-shared-parent/test.input @@ -1,4 +1,5 @@ { "filename": "sub/main.md", + "root": ".", "files": ["sub/main.md", "assets/logo.png"] } diff --git a/internal/convert/testdata/regression/images-shared-parent/test.output b/internal/convert/testdata/regression/images-shared-parent/test.output index 11c8486..33fca76 100644 --- a/internal/convert/testdata/regression/images-shared-parent/test.output +++ b/internal/convert/testdata/regression/images-shared-parent/test.output @@ -1,12 +1,12 @@ { "attachments": [ { - "filename": "..%2Fassets%2Flogo.png", + "filename": "assets%2Flogo.png", "path": "/assets/logo.png", - "source": "../assets/logo.png" + "source": "assets/logo.png" } ], "broken": [], - "html": "

Shared Assets

\n

A page in a subdirectory referencing an asset directory above it -- the layout GitHub renders too -- is published, with the path preserved in the attachment name:

\n

\n", + "html": "

Shared Assets

\n

A page in a subdirectory referencing an asset directory above it -- the layout GitHub renders too -- is published, with the path preserved in the attachment name:

\n

\n", "warnings": [] } diff --git a/internal/convert/testdata/regression/internal-doc-links/test.output b/internal/convert/testdata/regression/internal-doc-links/test.output index b1cf50b..1a726e8 100644 --- a/internal/convert/testdata/regression/internal-doc-links/test.output +++ b/internal/convert/testdata/regression/internal-doc-links/test.output @@ -2,5 +2,7 @@ "attachments": [], "broken": [], "html": "

Internal Doc Links

\n

A link to a sibling doc is rewritten to its Confluence URL: the sibling page.

\n

A link to a doc that isn't in this set is left untouched: unknown.

\n

An absolute URL is never rewritten: external.

\n", - "warnings": [] + "warnings": [ + "link not resolved: unknown.md" + ] } diff --git a/internal/convert/testdata/regression/link-cross-directory/index.md b/internal/convert/testdata/regression/link-cross-directory/index.md new file mode 100644 index 0000000..e5c8abe --- /dev/null +++ b/internal/convert/testdata/regression/link-cross-directory/index.md @@ -0,0 +1,8 @@ +--- +page_id: 100 +--- +# Index + +A [link to the nested overview](setup/overview.md) must resolve to the nested +page, not the top-level page that happens to share its basename -- 025's +Scenario A. diff --git a/internal/convert/testdata/regression/link-cross-directory/overview.md b/internal/convert/testdata/regression/link-cross-directory/overview.md new file mode 100644 index 0000000..24f3e0a --- /dev/null +++ b/internal/convert/testdata/regression/link-cross-directory/overview.md @@ -0,0 +1,7 @@ +--- +page_id: 999 +title: Top Overview +--- +# Top Overview + +The wrong page a basename-only lookup used to resolve to. diff --git a/internal/convert/testdata/regression/link-cross-directory/setup/overview.md b/internal/convert/testdata/regression/link-cross-directory/setup/overview.md new file mode 100644 index 0000000..cb5981e --- /dev/null +++ b/internal/convert/testdata/regression/link-cross-directory/setup/overview.md @@ -0,0 +1,7 @@ +--- +page_id: 777 +title: Setup Overview +--- +# Setup Overview + +The page index.md's link actually names. diff --git a/internal/convert/testdata/regression/link-cross-directory/test.input b/internal/convert/testdata/regression/link-cross-directory/test.input new file mode 100644 index 0000000..5ab24d7 --- /dev/null +++ b/internal/convert/testdata/regression/link-cross-directory/test.input @@ -0,0 +1,5 @@ +{ + "filename": "index.md", + "root": ".", + "files": ["index.md", "overview.md", "setup/overview.md"] +} diff --git a/internal/convert/testdata/regression/link-cross-directory/test.output b/internal/convert/testdata/regression/link-cross-directory/test.output new file mode 100644 index 0000000..e9e62a3 --- /dev/null +++ b/internal/convert/testdata/regression/link-cross-directory/test.output @@ -0,0 +1,6 @@ +{ + "attachments": [], + "broken": [], + "html": "

Index

\n

A link to the nested overview must resolve to the nested page, not the top-level page that happens to share its basename -- 025's Scenario A.

\n", + "warnings": [] +} diff --git a/internal/convert/testdata/regression/link-outside-root/docs/link.md b/internal/convert/testdata/regression/link-outside-root/docs/link.md new file mode 100644 index 0000000..c46cbe8 --- /dev/null +++ b/internal/convert/testdata/regression/link-outside-root/docs/link.md @@ -0,0 +1,8 @@ +# Escaping Link + +A [link to a page outside the documentation root](../outside/linked.md) is +left exactly as written -- unresolved, and reported (minimal R1) -- because +the target sits above root, so the link index never walked it. Making +resolution path-aware makes this a real path lookup rather than accidentally +safe by basename flattening (025's Scenario F); it still needs no clamp, +since a path outside root is simply never in the index in the first place. diff --git a/internal/convert/testdata/regression/link-outside-root/outside/linked.md b/internal/convert/testdata/regression/link-outside-root/outside/linked.md new file mode 100644 index 0000000..8a057a5 --- /dev/null +++ b/internal/convert/testdata/regression/link-outside-root/outside/linked.md @@ -0,0 +1,7 @@ +--- +page_id: 555 +title: Linked Outside +--- +# Linked Outside + +Above the declared root; the link naming this file must not resolve to it. diff --git a/internal/convert/testdata/regression/link-outside-root/test.input b/internal/convert/testdata/regression/link-outside-root/test.input new file mode 100644 index 0000000..2f0dea8 --- /dev/null +++ b/internal/convert/testdata/regression/link-outside-root/test.input @@ -0,0 +1,5 @@ +{ + "filename": "docs/link.md", + "root": "docs", + "files": ["docs/link.md", "outside/linked.md"] +} diff --git a/internal/convert/testdata/regression/link-outside-root/test.output b/internal/convert/testdata/regression/link-outside-root/test.output new file mode 100644 index 0000000..df1cdfa --- /dev/null +++ b/internal/convert/testdata/regression/link-outside-root/test.output @@ -0,0 +1,8 @@ +{ + "attachments": [], + "broken": [], + "html": "

Escaping Link

\n

A link to a page outside the documentation root is left exactly as written -- unresolved, and reported (minimal R1) -- because the target sits above root, so the link index never walked it. Making resolution path-aware makes this a real path lookup rather than accidentally safe by basename flattening (025's Scenario F); it still needs no clamp, since a path outside root is simply never in the index in the first place.

\n", + "warnings": [ + "link not resolved: ../outside/linked.md" + ] +} diff --git a/internal/convert/testroot_test.go b/internal/convert/testroot_test.go new file mode 100644 index 0000000..df0fe87 --- /dev/null +++ b/internal/convert/testroot_test.go @@ -0,0 +1,41 @@ +package convert_test + +import ( + "path/filepath" + "testing" + + "github.com/mozilla/markfluence/internal/linkindex" + "github.com/mozilla/markfluence/internal/project" +) + +// testRoot builds a *project.Root for tests that call MdToConfluence but don't +// exercise image/root behavior themselves -- their markdown has no local image +// references, so any real, existing directory is a valid root. dir defaults to +// the current directory when "". +func testRoot(t *testing.T, dir string) *project.Root { + t.Helper() + if dir == "" { + dir = "." + } + abs, err := filepath.Abs(dir) + if err != nil { + t.Fatal(err) + } + root, err := project.FromPath(abs) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = root.FS.Close() }) + return root +} + +// testIndex builds the link index for root, for tests that call +// MdToConfluence but don't exercise link resolution themselves. +func testIndex(t *testing.T, root *project.Root) *linkindex.Index { + t.Helper() + idx, err := linkindex.Build(root) + if err != nil { + t.Fatal(err) + } + return idx +} diff --git a/internal/convert/version_test.go b/internal/convert/version_test.go index ca61f3d..19d5c06 100644 --- a/internal/convert/version_test.go +++ b/internal/convert/version_test.go @@ -15,7 +15,8 @@ func TestVersionTokenReplaced(t *testing.T) { "# Title\n\n\n", ) const stamp = "markfluence v1.2.3 2020-01-01T00:00:00Z" - page, err := convert.MdToConfluence(md, "https://wiki.example.net", "ENG", stamp) + root := testRoot(t, filepath.Dir(md.Filename)) + page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", stamp) if err != nil { t.Fatal(err) } diff --git a/internal/jsonout/jsonout.go b/internal/jsonout/jsonout.go index f468116..db0b45d 100644 --- a/internal/jsonout/jsonout.go +++ b/internal/jsonout/jsonout.go @@ -42,8 +42,15 @@ type Envelope struct { SchemaVersion int `json:"schema_version"` MarkfluenceVersion string `json:"markfluence_version"` Command string `json:"command"` - Results []any `json:"results"` - Summary any `json:"summary"` + // Roots is every distinct documentation root the command resolved, + // sorted -- empty for a command with no per-file root concept (find, + // search, schema, ...), and for create/update/attachment-upload's + // pre-flight failure paths that never reached root resolution. Not + // omitted: every envelope carries this key, [] when there is nothing to + // report, the same convention Results already follows. + Roots []string `json:"roots"` + Results []any `json:"results"` + Summary any `json:"summary"` } // ErrorObject is the stderr document for a fatal/pre-flight failure in --json @@ -56,7 +63,10 @@ type ErrorObject struct { } // NewEnvelope builds an envelope for a command, stamping the schema and build -// version. results is emitted as [] (never null) when empty. +// version. results is emitted as [] (never null) when empty; so is Roots, +// which callers with a root to report set afterward -- most commands have +// none, so making it a constructor parameter would force all of them to pass +// nil for a concept they don't have. func NewEnvelope(command string, results []any, summary any) Envelope { if results == nil { results = []any{} @@ -65,6 +75,7 @@ func NewEnvelope(command string, results []any, summary any) Envelope { SchemaVersion: SchemaVersion, MarkfluenceVersion: buildinfo.Version, Command: command, + Roots: []string{}, Results: results, Summary: summary, } diff --git a/internal/linkindex/cache.go b/internal/linkindex/cache.go new file mode 100644 index 0000000..0264583 --- /dev/null +++ b/internal/linkindex/cache.go @@ -0,0 +1,33 @@ +package linkindex + +import "github.com/mozilla/markfluence/internal/project" + +// Cache builds an Index per distinct root, reusing it for every later call +// with the same root. Most batch commands hit this constantly: many files in +// one invocation typically share one project, and should share one index -- +// built once, not once per file or per directory, which is the cost Build's +// doc comment measures. +// +// Not safe for concurrent use. +type Cache struct { + byRoot map[string]*Index +} + +// NewCache builds an empty Cache. +func NewCache() *Cache { + return &Cache{byRoot: map[string]*Index{}} +} + +// Get returns the Index for root, building it (via Build) only the first time +// a given root.Dir is seen. +func (c *Cache) Get(root *project.Root) (*Index, error) { + if idx, ok := c.byRoot[root.Dir]; ok { + return idx, nil + } + idx, err := Build(root) + if err != nil { + return nil, err + } + c.byRoot[root.Dir] = idx + return idx, nil +} diff --git a/internal/linkindex/linkindex.go b/internal/linkindex/linkindex.go new file mode 100644 index 0000000..dde89e5 --- /dev/null +++ b/internal/linkindex/linkindex.go @@ -0,0 +1,170 @@ +// Package linkindex builds the tree-wide index internal/convert resolves +// document links and same-page/cross-file anchors against. +// +// It replaces a per-directory, basename-keyed lookup with a path-keyed one +// covering the whole tree below a project.Root: a link destination resolves +// by where it actually points, not by matching a bare filename against +// whatever happens to share the linking file's own directory. That is what +// fixes a same-basename file in a different directory resolving to the wrong +// page (025 Scenario A) and what makes a link that could traverse above root +// simply not found rather than accidentally safe by basename flattening +// (Scenario F) -- no clamp is needed here, since an escaping path is never in +// an index built by walking downward from root in the first place. +// +// Built once per root and shared across every file converted under it. +// Rebuilding per file, per directory, was the pre-025 code's accidental +// O(n^2): each conversion re-read its own directory, so a directory of 40 +// files cost 40 reads for each of 400 conversions. See _plans/025's +// measurement. +package linkindex + +import ( + "io/fs" + "regexp" + "strings" + + "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/project" +) + +// PageEntry is a markdown file's Confluence coordinates, keyed in an Index by +// its path relative to the root. +type PageEntry struct { + PageID string + Title string +} + +// Index is the tree-wide page and anchor map. Keys are always root-relative, +// slash-separated paths -- the same form internal/convert resolves a link +// destination to before looking it up. +type Index struct { + pages map[string]PageEntry + anchors map[string]map[string]string +} + +// Build walks root's tree once, via root.FS, collecting every *.md file's +// page_id/title (when it has one) and heading anchors. Walking through +// root.FS is what keeps the walk from ever descending a symlinked directory: +// a symlink's directory entry reports its own type (a link, not a +// directory), so fs.WalkDir calls the visit function for it once and does +// not recurse -- matching the non-goal in docs/guarantees.md#symlinks. An +// unreadable file is skipped rather than failing the whole build; a page with +// no page_id yet is walked (its anchors still work) but has no PageEntry, the +// same as a draft with no page_id has always had no place in this lookup. +func Build(root *project.Root) (*Index, error) { + idx := &Index{pages: map[string]PageEntry{}, anchors: map[string]map[string]string{}} + fsys := root.FS.FS() + err := fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(d.Name(), ".md") { + return nil + } + data, err := fs.ReadFile(fsys, path) + if err != nil { + return nil + } + mf := frontmatter.Parse(path, string(data)) + if id := mf.PageID(); id != "" { + idx.pages[path] = PageEntry{PageID: id, Title: mf.Title()} + } + anchors := map[string]string{} + for _, h := range extractHeadings(mf.Body) { + if gh := GithubSlug(h); gh != "" { + anchors[gh] = ConfluenceSlug(h) + } + } + idx.anchors[path] = anchors + return nil + }) + if err != nil { + return nil, err + } + return idx, nil +} + +// Page returns the coordinates recorded for path (root-relative, +// slash-separated), and whether an entry exists there. +func (idx *Index) Page(path string) (PageEntry, bool) { + e, ok := idx.pages[path] + return e, ok +} + +// Anchor returns the Confluence-side slug matching a GitHub-style slug on the +// page at path, and whether it exists. +func (idx *Index) Anchor(path, githubSlug string) (string, bool) { + m, ok := idx.anchors[path] + if !ok { + return "", false + } + s, ok := m[githubSlug] + return s, ok +} + +// SetPage overrides -- or injects -- the entry for path. create's reserve +// phase uses this to seed ids that exist only in memory (not yet, or never, +// under --no-persist, written back to frontmatter) before its publish phase +// converts anything, so every link resolves regardless of creation order. +func (idx *Index) SetPage(path string, entry PageEntry) { + idx.pages[path] = entry +} + +var ( + // nonSlugRE strips everything except letters, digits, underscore, + // whitespace, and hyphens (Unicode-aware). + nonSlugRE = regexp.MustCompile(`[^\p{L}\p{N}_\s-]`) + whitespaceRE = regexp.MustCompile(`\s`) + whitespaceRunRE = regexp.MustCompile(`\s+`) +) + +// GithubSlug replicates GitHub's heading-anchor slugger: lowercase; strip all +// but letters/digits/underscore/whitespace/hyphen; each whitespace char +// becomes one hyphen; trim leading/trailing hyphens. +func GithubSlug(heading string) string { + s := strings.ToLower(heading) + s = nonSlugRE.ReplaceAllString(s, "") + s = whitespaceRE.ReplaceAllString(s, "-") + return strings.Trim(s, "-") +} + +// ConfluenceSlug replicates Confluence's scheme: preserve case and +// punctuation, collapsing runs of whitespace to single hyphens. Confluence +// assigns this anchor to a heading itself -- there is no id attribute for +// internal/convert to emit -- so this exists purely to compute what +// Confluence's own anchor will be. +func ConfluenceSlug(heading string) string { + return whitespaceRunRE.ReplaceAllString(strings.TrimSpace(heading), "-") +} + +// extractHeadings returns the text of each ATX heading in a +// frontmatter-stripped body, skipping fenced code blocks so "#" lines inside +// samples aren't headings. +func extractHeadings(body string) []string { + var headings []string + inCode := false + for _, line := range strings.Split(body, "\n") { + if strings.HasPrefix(line, "```") { + inCode = !inCode + continue + } + if inCode { + continue + } + hashes := 0 + for hashes < len(line) && line[hashes] == '#' { + hashes++ + } + if hashes == 0 || hashes >= len(line) { + continue + } + rest := line[hashes:] + if strings.TrimLeft(rest, " \t") == rest { // no whitespace after the #s + continue + } + if text := strings.TrimSpace(rest); text != "" { + headings = append(headings, text) + } + } + return headings +} diff --git a/internal/linkindex/linkindex_test.go b/internal/linkindex/linkindex_test.go new file mode 100644 index 0000000..c5996f8 --- /dev/null +++ b/internal/linkindex/linkindex_test.go @@ -0,0 +1,134 @@ +package linkindex + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/mozilla/markfluence/internal/project" +) + +func write(t *testing.T, path, body string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } +} + +func rootAt(t *testing.T, dir string) *project.Root { + t.Helper() + r, err := project.FromPath(dir) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = r.FS.Close() }) + return r +} + +// TestBuildKeysByRootRelativePathNotBasename is Scenario A's fix: two files +// sharing a basename in different directories must not collide -- each gets +// its own entry, keyed by its full path from root. +func TestBuildKeysByRootRelativePathNotBasename(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "overview.md"), "---\npage_id: 999\ntitle: Product Overview\n---\nbody\n") + write(t, filepath.Join(root, "setup", "overview.md"), "---\npage_id: 777\ntitle: Setup Overview\n---\nbody\n") + + idx, err := Build(rootAt(t, root)) + if err != nil { + t.Fatal(err) + } + + top, ok := idx.Page("overview.md") + if !ok || top.PageID != "999" { + t.Errorf("Page(%q) = %+v, %v; want page_id 999", "overview.md", top, ok) + } + nested, ok := idx.Page("setup/overview.md") + if !ok || nested.PageID != "777" { + t.Errorf("Page(%q) = %+v, %v; want page_id 777", "setup/overview.md", nested, ok) + } +} + +// TestBuildCollectsAnchorsPerPage covers a heading on one page not leaking +// into another page's anchor set. +func TestBuildCollectsAnchorsPerPage(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "a.md"), "# Section One\n") + write(t, filepath.Join(root, "b.md"), "# Section Two\n") + + idx, err := Build(rootAt(t, root)) + if err != nil { + t.Fatal(err) + } + + if slug, ok := idx.Anchor("a.md", "section-one"); !ok || slug != "Section-One" { + t.Errorf("Anchor(a.md, section-one) = %q, %v", slug, ok) + } + if _, ok := idx.Anchor("a.md", "section-two"); ok { + t.Error("a.md must not see b.md's heading") + } +} + +// TestBuildSkipsPageWithNoPageID covers a draft (no page_id yet, the ordinary +// state of an unpublished file): it must not appear in Page, since 025 treats +// "not in the index" as the unresolved case a link falls through on. +func TestBuildSkipsPageWithNoPageID(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "draft.md"), "---\ntitle: Draft\n---\nbody\n") + + idx, err := Build(rootAt(t, root)) + if err != nil { + t.Fatal(err) + } + if _, ok := idx.Page("draft.md"); ok { + t.Error("a page_id-less file must not be in the index") + } +} + +// TestBuildDoesNotDescendSymlinkedDirectory is the non-goal: a symlinked +// directory inside root must not be walked into, even though its target +// (here, outside root) holds a page_id-bearing file that would otherwise be +// found. +func TestBuildDoesNotDescendSymlinkedDirectory(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevated privileges on Windows") + } + root := t.TempDir() + outside := t.TempDir() + write(t, filepath.Join(outside, "secret.md"), "---\npage_id: 555\ntitle: Secret\n---\nbody\n") + if err := os.Symlink(outside, filepath.Join(root, "linked")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + idx, err := Build(rootAt(t, root)) + if err != nil { + t.Fatal(err) + } + if _, ok := idx.Page("linked/secret.md"); ok { + t.Error("the walk must not descend a symlinked directory") + } +} + +func TestSetPageOverridesAndInjects(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "a.md"), "---\npage_id: 1\ntitle: A\n---\nbody\n") + + idx, err := Build(rootAt(t, root)) + if err != nil { + t.Fatal(err) + } + idx.SetPage("a.md", PageEntry{PageID: "2", Title: "A renamed"}) + if got, ok := idx.Page("a.md"); !ok || got.PageID != "2" { + t.Errorf("Page(a.md) = %+v, %v; want the override", got, ok) + } + + // b.md was never on disk with a page_id -- this is create's reserve + // phase injecting an id that exists only in memory. + idx.SetPage("b.md", PageEntry{PageID: "3", Title: "B"}) + if got, ok := idx.Page("b.md"); !ok || got.PageID != "3" { + t.Errorf("Page(b.md) = %+v, %v; want the injected entry", got, ok) + } +} diff --git a/internal/project/cache.go b/internal/project/cache.go new file mode 100644 index 0000000..5b6b06f --- /dev/null +++ b/internal/project/cache.go @@ -0,0 +1,127 @@ +package project + +import ( + "path/filepath" + "sort" +) + +// Cache resolves a root per starting directory, reusing the result -- and its +// open os.Root handle -- for every later call with the same directory. Most +// batch commands hit this constantly: many files in one invocation typically +// share a directory or a project, and re-walking for each one would be the +// same per-conversion cost 025 measured as quadratic at scale. +// +// Not safe for concurrent use. +type Cache struct { + override string + byDir map[string]*Root +} + +// NewCache builds a Cache that applies override -- --root's value, or "" when +// the flag wasn't set -- to every Resolve call. +func NewCache(override string) *Cache { + return &Cache{override: override, byDir: map[string]*Root{}} +} + +// 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 +// 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 +// starting directory (the quadratic cost 025 measured, reintroduced at the +// per-directory granularity a naive byDir[startDir] cache still leaves). +func (c *Cache) Resolve(startDir string) (*Root, error) { + abs, err := filepath.Abs(startDir) + if err != nil { + return nil, err + } + if root, ok := c.byDir[abs]; ok { + return root, nil + } + if c.override != "" { + root, err := FromPath(c.override) + if err != nil { + return nil, err + } + c.byDir[abs] = root + return root, nil + } + return c.walkAndCache(abs) +} + +// walkAndCache walks upward from abs exactly like Discover, but checks the +// cache at every level first: once the walk reaches a directory this Cache +// has already resolved a root for, every directory visited since abs is +// backfilled to that same *Root instead of opening a second os.Root for a +// root a sibling subtree already found. The no-project-file fallback is +// backfilled only to abs itself, never to an ancestor: that Root is bound to +// abs specifically (Discover's own contract -- see TestDiscoverFallsBackToStartDir +// and TestCacheRootsReturnsDistinctSortedValues), so caching it at an +// ancestor would wrongly hand every unrelated directory above it the same +// fallback root. +func (c *Cache) walkAndCache(abs string) (*Root, error) { + var visited []string + dir := abs + for { + if root, ok := c.byDir[dir]; ok { + for _, d := range visited { + c.byDir[d] = root + } + return root, nil + } + visited = append(visited, dir) + + hit, file, err := probeMarker(dir) + if err != nil { + return nil, err + } + if hit { + root, err := open(dir, file) + if err != nil { + return nil, err + } + for _, d := range visited { + c.byDir[d] = root + } + return root, nil + } + + parent := filepath.Dir(dir) + if parent == dir { + root, err := open(abs, "") + if err != nil { + return nil, err + } + c.byDir[abs] = root + return root, nil + } + dir = parent + } +} + +// Roots returns every distinct root Dir this cache has resolved, sorted, for +// reporting -- once per distinct value, not once per file, since a batch +// commonly resolves the same root for every file in it. +func (c *Cache) Roots() []string { + seen := map[string]bool{} + out := []string{} // never nil: a caller may marshal this straight into --json + for _, root := range c.byDir { + if !seen[root.Dir] { + seen[root.Dir] = true + out = append(out, root.Dir) + } + } + sort.Strings(out) + return out +} + +// Close closes every root handle this cache opened. Call it once, after every +// 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 { + _ = root.FS.Close() + } + clear(c.byDir) +} diff --git a/internal/project/cache_test.go b/internal/project/cache_test.go new file mode 100644 index 0000000..edc8864 --- /dev/null +++ b/internal/project/cache_test.go @@ -0,0 +1,161 @@ +package project + +import ( + "os" + "path/filepath" + "testing" +) + +func TestCacheResolveReusesRootForSameDirectory(t *testing.T) { + dir := t.TempDir() + c := NewCache("") + defer c.Close() + + first, err := c.Resolve(dir) + if err != nil { + t.Fatal(err) + } + second, err := c.Resolve(dir) + if err != nil { + t.Fatal(err) + } + if first != second { + t.Error("Resolve for the same directory should return the cached Root, not discover again") + } +} + +func TestCacheRootsReturnsDistinctSortedValues(t *testing.T) { + base := t.TempDir() + a := filepath.Join(base, "a") + b := filepath.Join(base, "b") + for _, d := range []string{a, b} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + // No project file anywhere, so each falls back to its own directory -- + // two distinct roots. + c := NewCache("") + defer c.Close() + if _, err := c.Resolve(b); err != nil { + t.Fatal(err) + } + if _, err := c.Resolve(a); err != nil { + t.Fatal(err) + } + + got := c.Roots() + want := []string{a, b} + if len(got) != 2 || got[0] != want[0] || got[1] != want[1] { + t.Errorf("Roots() = %v, want %v (sorted)", got, want) + } +} + +// TestCacheResolveBackfillsSharedRoot is the point of walkAndCache: two +// directories under the same project root must share one *Root -- opened +// once -- rather than each independently discovering (and os.OpenRoot-ing) +// the identical root, which is the double-cost a per-startDir-only cache +// still pays across a batch spanning many subdirectories of one project. +func TestCacheResolveBackfillsSharedRoot(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, Filename), []byte("# marker\n"), 0o644); err != nil { + t.Fatal(err) + } + x := filepath.Join(root, "docs", "a") + y := filepath.Join(root, "docs", "b") + for _, d := range []string{x, y} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + + c := NewCache("") + defer c.Close() + rx, err := c.Resolve(x) + if err != nil { + t.Fatal(err) + } + ry, err := c.Resolve(y) + if err != nil { + t.Fatal(err) + } + if rx != ry { + t.Error("Resolve for two directories under the same project root returned distinct *Root values, want one shared") + } + if _, ok := c.byDir[filepath.Join(root, "docs")]; !ok { + t.Error("the shared ancestor 'docs', visited resolving x, should be backfilled into the cache") + } +} + +// TestCacheResolveFallbackDoesNotContaminateAncestors guards the hazard +// walkAndCache's backfill has to avoid: the no-project-file fallback binds a +// Root to the directory Resolve was actually called with, not to any +// ancestor visited along the way, so a second, unrelated directory sharing +// that ancestor must fall back to *itself*, not inherit the first +// directory's fallback root. +func TestCacheResolveFallbackDoesNotContaminateAncestors(t *testing.T) { + base := t.TempDir() + x := filepath.Join(base, "a", "x") + y := filepath.Join(base, "a", "y") + for _, d := range []string{x, y} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + + c := NewCache("") + defer c.Close() + rx, err := c.Resolve(x) + if err != nil { + t.Fatal(err) + } + ry, err := c.Resolve(y) + if err != nil { + t.Fatal(err) + } + if rx.Dir != x { + t.Errorf("Resolve(x).Dir = %q, want %q (no project file, falls back to itself)", rx.Dir, x) + } + if ry.Dir != y { + t.Errorf("Resolve(y).Dir = %q, want %q -- got %q instead, contaminated by x's fallback", + ry.Dir, y, ry.Dir) + } +} + +func TestCacheOverrideAppliesToEveryResolve(t *testing.T) { + override := t.TempDir() + base := t.TempDir() + a := filepath.Join(base, "a") + b := filepath.Join(base, "b") + for _, d := range []string{a, b} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + + c := NewCache(override) + defer c.Close() + for _, dir := range []string{a, b} { + got, err := c.Resolve(dir) + if err != nil { + t.Fatal(err) + } + if got.Dir != override { + t.Errorf("Resolve(%q).Dir = %q, want the override %q", dir, got.Dir, override) + } + } + if roots := c.Roots(); len(roots) != 1 || roots[0] != override { + t.Errorf("Roots() = %v, want exactly [%q]", roots, override) + } +} + +func TestCacheCloseClearsEntries(t *testing.T) { + c := NewCache("") + if _, err := c.Resolve(t.TempDir()); err != nil { + t.Fatal(err) + } + c.Close() + if len(c.byDir) != 0 { + t.Errorf("byDir has %d entries after Close, want 0", len(c.byDir)) + } +} diff --git a/internal/project/project.go b/internal/project/project.go new file mode 100644 index 0000000..6e291d9 --- /dev/null +++ b/internal/project/project.go @@ -0,0 +1,152 @@ +// 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. +// +// 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: +// once per invocation from the working directory, to locate .env before any +// file is touched, and once per markdown file, from that file's own +// directory, to bound its reads and name its attachments. The two coincide +// whenever the file sits under the project the working directory is in, and +// diverge only when there is no markfluence.yaml at all, or a file belongs to +// a different project than the working directory does -- both legitimate, +// neither an error. +package project + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" +) + +// Filename is the marker file Discover looks for. +const Filename = "markfluence.yaml" + +// Root is a discovered (or defaulted) markfluence project root. +type Root struct { + // Dir is the root directory, absolute. + Dir string + // File is the absolute path to markfluence.yaml, or "" when none was + // found and Dir fell back to the starting directory. + File string + // 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. + FS *os.Root +} + +// Discover walks up from startDir looking for markfluence.yaml, stopping at +// the first ancestor that has one or at the filesystem root. Reaching the +// filesystem root with no hit is not an error: Dir falls back to startDir +// itself, and File stays "". +// +// It stats a filename at each level rather than listing the directory, which +// needs only execute (search) permission on each ancestor -- guaranteed, or +// startDir itself would be unreachable. An ancestor that can't be stat'd for +// permissions is treated as "not here" rather than fatal: the walk keeps +// going rather than failing a command over a directory it was never going to +// read anything else from. +// +// Discover does not follow symlinks in the walk itself -- it climbs lexically +// via filepath.Dir on an absolute path, so a symlinked ancestor is never +// substituted in. That closes the class of failure 025 names for the +// pre-model code: comparing paths after resolving one side and not the other. +func Discover(startDir string) (*Root, error) { + abs, err := filepath.Abs(startDir) + if err != nil { + return nil, err + } + + dir := abs + for { + hit, candidate, err := probeMarker(dir) + if err != nil { + return nil, err + } + if hit { + return open(dir, candidate) + } + + parent := filepath.Dir(dir) + if parent == dir { + // Reached the filesystem root with no hit. + return open(abs, "") + } + dir = parent + } +} + +// probeMarker stats markfluence.yaml directly inside dir, reporting whether +// it's a hit and, if so, its path. Factored out of Discover so Cache's own +// walk (cache.go) can consult its cache at every level using the identical +// per-directory check, rather than re-deriving it. +func probeMarker(dir string) (hit bool, file string, err error) { + candidate := filepath.Join(dir, Filename) + info, statErr := os.Stat(candidate) + switch { + case statErr == nil && !info.IsDir(): + return true, candidate, nil + case statErr == nil: + // A directory happens to be named markfluence.yaml -- not a hit; keep + // walking as if nothing were there. + return false, "", nil + case errors.Is(statErr, fs.ErrNotExist), errors.Is(statErr, fs.ErrPermission): + // Not here, or we can't tell -- keep walking. + return false, "", nil + default: + return false, "", statErr + } +} + +// open builds a Root for dir, opening an os.Root scoped to it. +func open(dir, file string) (*Root, error) { + root, err := os.OpenRoot(dir) + if err != nil { + return nil, err + } + return &Root{Dir: dir, File: file, FS: root}, nil +} + +// FromPath builds a Root directly from an explicit directory, bypassing +// discovery entirely. It exists for --root, which overrides discovery for the +// whole invocation with one value applied uniformly to every file. dir must +// exist and be a directory; a markfluence.yaml inside it is still noted in +// File, for accurate reporting, even though --root's meaning doesn't depend +// on one being there. +func FromPath(dir string) (*Root, error) { + abs, err := filepath.Abs(dir) + if err != nil { + return nil, err + } + info, err := os.Stat(abs) + if err != nil { + return nil, err + } + if !info.IsDir() { + return nil, fmt.Errorf("--root %s is not a directory", dir) + } + + file := "" + candidate := filepath.Join(abs, Filename) + if info, err := os.Stat(candidate); err == nil && !info.IsDir() { + file = candidate + } + return open(abs, file) +} + +// Resolve is the entry point a command should use once it has both an +// optional --root override and a starting directory: override wins, +// bypassing discovery via FromPath; an empty override discovers normally via +// Discover. +func Resolve(override, startDir string) (*Root, error) { + if override != "" { + return FromPath(override) + } + return Discover(startDir) +} diff --git a/internal/project/project_test.go b/internal/project/project_test.go new file mode 100644 index 0000000..60344c4 --- /dev/null +++ b/internal/project/project_test.go @@ -0,0 +1,246 @@ +package project + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +func writeProjectFile(t *testing.T, dir string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, Filename), []byte("# marker\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestDiscoverFindsProjectFile(t *testing.T) { + root := t.TempDir() + writeProjectFile(t, root) + sub := filepath.Join(root, "team", "sub") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + + got, err := Discover(sub) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != root { + t.Errorf("Dir = %q, want %q", got.Dir, root) + } + if want := filepath.Join(root, Filename); got.File != want { + t.Errorf("File = %q, want %q", got.File, want) + } +} + +func TestDiscoverNearestAncestorWins(t *testing.T) { + outer := t.TempDir() + writeProjectFile(t, outer) + inner := filepath.Join(outer, "team") + if err := os.Mkdir(inner, 0o755); err != nil { + t.Fatal(err) + } + writeProjectFile(t, inner) + sub := filepath.Join(inner, "sub") + if err := os.Mkdir(sub, 0o755); err != nil { + t.Fatal(err) + } + + got, err := Discover(sub) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != inner { + t.Errorf("Dir = %q, want the nearer project file's directory %q", got.Dir, inner) + } +} + +func TestDiscoverFallsBackToStartDir(t *testing.T) { + // No markfluence.yaml anywhere above a fresh temp directory. This walks + // the real filesystem up to "/", which is fine -- it's a handful of Stat + // calls -- and relies on the test host not having a markfluence.yaml + // somewhere above the OS temp directory, same assumption every upward + // project-file scanner (go.mod, .git, .editorconfig) makes. + start := t.TempDir() + + got, err := Discover(start) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != start { + t.Errorf("Dir = %q, want the fallback %q", got.Dir, start) + } + if got.File != "" { + t.Errorf("File = %q, want empty (no project file found)", got.File) + } +} + +func TestDiscoverIgnoresADirectoryNamedLikeTheMarker(t *testing.T) { + root := t.TempDir() + // A directory happens to be named markfluence.yaml -- not a hit. + if err := os.Mkdir(filepath.Join(root, Filename), 0o755); err != nil { + t.Fatal(err) + } + sub := filepath.Join(root, "sub") + if err := os.Mkdir(sub, 0o755); err != nil { + t.Fatal(err) + } + + got, err := Discover(sub) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.File != "" { + t.Errorf("File = %q, want empty -- a directory is not a hit", got.File) + } +} + +func TestDiscoverEACCESKeepsWalking(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX permission bits don't apply") + } + if os.Geteuid() == 0 { + t.Skip("running as root bypasses permission checks") + } + + base := t.TempDir() + writeProjectFile(t, base) // the project file this test expects to still find + blocked := filepath.Join(base, "blocked") + deep := filepath.Join(blocked, "c", "d") + if err := os.MkdirAll(deep, 0o755); err != nil { + t.Fatal(err) + } + // Deny search permission on "blocked" itself: resolving any path through + // it -- including deep, several levels below -- now fails with EACCES, + // not just a stat of "blocked" directly. + if err := os.Chmod(blocked, 0o000); err != nil { + t.Fatal(err) + } + defer func() { _ = os.Chmod(blocked, 0o755) }() // let TempDir cleanup remove it + + got, err := Discover(deep) + if err != nil { + t.Fatalf("Discover returned an error instead of walking past the blocked ancestor: %v", err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != base { + t.Errorf("Dir = %q, want %q (the project file above the blocked ancestor)", got.Dir, base) + } +} + +func TestFromPathUsesTheGivenDirectory(t *testing.T) { + dir := t.TempDir() + // A markfluence.yaml elsewhere doesn't matter; --root isn't discovery. + got, err := FromPath(dir) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != dir { + t.Errorf("Dir = %q, want %q", got.Dir, dir) + } + if got.File != "" { + t.Errorf("File = %q, want empty (no marker written)", got.File) + } +} + +func TestFromPathNotesAnExistingMarker(t *testing.T) { + dir := t.TempDir() + writeProjectFile(t, dir) + + got, err := FromPath(dir) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if want := filepath.Join(dir, Filename); got.File != want { + t.Errorf("File = %q, want %q", got.File, want) + } +} + +func TestFromPathRejectsAFile(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "not-a-dir") + if err := os.WriteFile(file, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if _, err := FromPath(file); err == nil { + t.Fatal("want an error for a --root that names a file, not a directory") + } +} + +func TestFromPathRejectsAMissingDirectory(t *testing.T) { + if _, err := FromPath(filepath.Join(t.TempDir(), "nope")); err == nil { + t.Fatal("want an error for a --root that doesn't exist") + } +} + +func TestResolveOverrideSkipsDiscovery(t *testing.T) { + override := t.TempDir() + // A project file sits above startDir; if Resolve discovered instead of + // honoring the override, it would find this one, not override. + outer := t.TempDir() + writeProjectFile(t, outer) + startDir := filepath.Join(outer, "sub") + if err := os.Mkdir(startDir, 0o755); err != nil { + t.Fatal(err) + } + + got, err := Resolve(override, startDir) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != override { + t.Errorf("Dir = %q, want the override %q, not the discovered %q", got.Dir, override, outer) + } +} + +func TestResolveEmptyOverrideDiscovers(t *testing.T) { + outer := t.TempDir() + writeProjectFile(t, outer) + startDir := filepath.Join(outer, "sub") + if err := os.Mkdir(startDir, 0o755); err != nil { + t.Fatal(err) + } + + got, err := Resolve("", startDir) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + if got.Dir != outer { + t.Errorf("Dir = %q, want the discovered %q", got.Dir, outer) + } +} + +func TestDiscoverRootFSIsScopedToDir(t *testing.T) { + root := t.TempDir() + writeProjectFile(t, root) + + got, err := Discover(root) + if err != nil { + t.Fatal(err) + } + defer func() { _ = got.FS.Close() }() + + f, err := got.FS.Open(Filename) + if err != nil { + t.Fatalf("opening %s through FS: %v", Filename, err) + } + _ = f.Close() +} diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index 434b953..97f96f6 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -5,11 +5,12 @@ "description": "The single JSON document markfluence writes to stdout under --json. results holds one object per target (a single element for info/read; one per attachment for attachment-list); its item shape and the summary shape depend on command. The typed error object written to stderr on a fatal/pre-flight failure conforms to #/$defs/errorObject.", "type": "object", "additionalProperties": false, - "required": ["schema_version", "markfluence_version", "command", "results", "summary"], + "required": ["schema_version", "markfluence_version", "command", "roots", "results", "summary"], "properties": { "schema_version": { "const": 1 }, "markfluence_version": { "type": "string" }, "command": { "enum": ["info", "read", "update", "create", "fix", "children", "find", "search", "attachment-list", "attachment-upload", "attachment-download", "export"] }, + "roots": { "type": "array", "items": { "type": "string" }, "description": "Every distinct documentation root the command resolved, sorted. Empty for a command with no per-file root concept, and for a pre-flight failure that never reached root resolution." }, "results": { "type": "array" }, "summary": { "type": "object" } }, @@ -366,7 +367,7 @@ } }, "createResult": { - "description": "One input file's create outcome. page_id and url describe the created page; on a failure they are null, except when the file's frontmatter page_id blocked creation -- then page_id is that id and url is the page already at it (null when the id resolves to nothing).", + "description": "One input file's create outcome. page_id and url describe the created page; on a failure they are null, except in two cases where a real page exists and its id must stay visible: the file's frontmatter page_id blocked creation (page_id is that id, url is the page already at it, null when the id resolves to nothing), or a page was created on the server but persisting its frontmatter afterward failed (page_id/url name that orphaned page so it can be recovered).", "type": "object", "additionalProperties": false, "required": [