diff --git a/CLAUDE.md b/CLAUDE.md index 389a770..a4c7af7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -50,7 +50,8 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd ### 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. -- `cmd/{update,create,fix,info,read,export,children,find,search}/` — one package per command (each exports `Cmd`), orchestrating the `internal` packages and `internal/ui` output. `create` is two-phase and transactional (validate all, then create parents-first in topological order); `fix` is read-only on the server. `create` accepts a **page or a folder** as `parent`: `checkParentInSpace` asks the page route, then `/folders/{id}`, and finding nothing as a page proves nothing until both have been asked — that single missing fallback was #68, since Confluence itself accepts a folder `parentId` with no other accommodation. It also validates a frontmatter `page_id` **first** (before space/parent/title lookups: most specific error, three fewer API calls) and treats all three outcomes as failures — non-numeric, resolves to nothing, or already taken. The middle one is the bug from #18: publishing anyway would create a second page and overwrite the id, so an id that can't be explained is never "create a new page". Both "already exists" errors (page_id taken, title clash) link the page in the way; the `pageIDFailure` typed error carries `page_id`/`url` so `--json` reports them as fields, which is the only case where a failed result names a page. +- `cmd/{update,create,fix,check,info,read,export,children,find,search}/` — one package per command (each exports `Cmd`), orchestrating the `internal` packages and `internal/ui` output. `create` is two-phase and transactional (validate all, then create parents-first in topological order); `fix` is read-only on the server; `check` is read-only on both the server and disk and touches neither, since it never constructs one (see its own bullet below). `create` accepts a **page or a folder** as `parent`: `checkParentInSpace` asks the page route, then `/folders/{id}`, and finding nothing as a page proves nothing until both have been asked — that single missing fallback was #68, since Confluence itself accepts a folder `parentId` with no other accommodation. It also validates a frontmatter `page_id` **first** (before space/parent/title lookups: most specific error, three fewer API calls) and treats all three outcomes as failures — non-numeric, resolves to nothing, or already taken. The middle one is the bug from #18: publishing anyway would create a second page and overwrite the id, so an id that can't be explained is never "create a new page". Both "already exists" errors (page_id taken, title clash) link the page in the way; the `pageIDFailure` typed error carries `page_id`/`url` so `--json` reports them as fields, which is the only case where a failed result names a page. +- `cmd/check/` — `check` (#42): validate one or more markdown FILEs against the converter and frontmatter rules with **no network access, no credentials, and no writes** — the first command whose `run()` never constructs a `client.ConfluenceClient` (`root.go`'s `PersistentPreRunE` doesn't force one into existence either, so nothing upstream requires it). It builds `root`/`index` per file exactly like `update`/`create` (`internal/project.Cache`/`internal/linkindex.Cache`), against hardcoded `baseURL`/`spaceKey` (the regression suite's own `https://wiki.example.net`/`ENG`) rather than flags — both are read only to build a rewritten doc-link's *text*, and nothing in `Broken`/`Warnings` reads either, so hardcoding them costs nothing and makes `check` byte-identical across machines. Frontmatter validation is deliberately narrow: an unparseable/unterminated block (`frontmatter.ErrUnterminatedFrontmatter`), an invalid `page_width` (`pagewidth.Declared`), a present-but-non-numeric `page_id` (`pageref.IsDigits`) — never whether `page_id`/`space`/`parent` are set at all, since `check` cannot know whether the caller is about to `create` or `update`, and a false positive there is worse than a miss. A `broken` result is `ok: false` with `error`/`code` both left `null`: unlike every other failure, `broken`/`warnings` already say everything there is to say, so `code: VALIDATION` is reserved for `status: failed` (a file that never reached the converter at all). `--show-html` surfaces `ConfluencePage.HTML`/`Attachments` — nothing else in the CLI ever prints either — as `debug: {html, attachments} | null`; `html` stays compact/unindented in `--json` (matching what `update`/`create` would literally publish) while human output indents it by nesting depth (`indentHTML`, a per-line indent based on tag-open/close counting, not a whitespace-normalizing reformat, since the renderer already breaks lines at every structural boundary and reformatting within a line could alter meaningful inline text). - `cmd/export/` — `export`: `pagedoc` for the body, `attachfile` for the attachments. Markdown only, and attachments land at their recorded paths — there is deliberately no `--attachments-dir`, since collecting them would require rewriting image `src`s, which would make the next `update` publish under different attachment names and orphan the originals. Only referenced attachments are exported, found by scanning raw storage for `ri:filename` (not just `ac:image`, which is all the converter special-cases, so a link target or a macro-internal reference would otherwise be dropped). A reference with no attachment is a warning, not a failure. - `cmd/children/` — `children`: list the pages and folders under a page or folder, via `internal/pagetree`. `--depth` is a **string** vocabulary (a positive number or `all`, default `1`), not an int: `all` is not a number, and `0` is refused rather than read as "unlimited" the way it is elsewhere, because silently walking a whole space for someone who meant "none" is worse than an error that names `all`. Folder rows are emitted with a `type` column, which is what makes "a folder counts as a level" safe. Empty is a success: `No children.` and exit 0. - `cmd/find/` — `find`: resolve a title to the ids carrying it, via `client.FindByTitle`. A title is the one handle `internal/pageref` cannot resolve. It reports **current pages, archived pages, and folders**, which takes two requests because no single API sees all three — and the three-way split is the thing to keep straight before touching it ([docs/confluence/search.md](docs/confluence/search.md)). An **archived** page is reported, with a `status` column, because it is absent from the page tree yet still reserves its title; a **folder** is reported because a folder id is a legitimate `parent`, but a folder reserves nothing, so a folder row must never be treated as a naming conflict. `--space` is a space **key**, and an unknown one is a hard error rather than an empty result — CQL answers an unknown key with zero rows, which reads exactly like "no such page". Either half failing fails the whole command: a partial answer reads as "nothing found", and the caller's next move on that is to create a duplicate. Empty is a success: `No matches found.` and exit 0. Its operational failure is an `errorObject` on stderr rather than a `results[0]` entry — there is no page id to name — which it shares with `search` and nothing else. @@ -63,8 +64,8 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `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; 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`; `storage_to_md.go`'s `cellTexts` reverses this, reading `data-highlight-colour` back into a `bg:` marker (`cellBGNames`, the reverse of `tables.go`'s swatch map — a hex outside the 21 swatches round-trips as the literal hex, and where two names share a hex the British spelling wins, matching Confluence's own `-colour`), and a column's GFM alignment becomes a `

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

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

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

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

` tag, stamped with `data-layout="align-start"` so tables auto-size and left-align — this must stay if column widths are ever emitted, or a `` silently induces a layout; plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour`; `storage_to_md.go`'s `cellTexts` reverses this, reading `data-highlight-colour` back into a `bg:` marker (`cellBGNames`, the reverse of `tables.go`'s swatch map — a hex outside the 21 swatches round-trips as the literal hex, and where two names share a hex the British spelling wins, matching Confluence's own `-colour`), and a column's GFM alignment becomes a `

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

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

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

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

\n\n\n

after

" + if got := indentHTML(html); got != want { + t.Errorf("indentHTML =\n%q\nwant\n%q", got, want) + } +} + +type errString string + +func (e errString) Error() string { return string(e) } diff --git a/cmd/create/create.go b/cmd/create/create.go index e2ecae9..0ec55e1 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -669,7 +669,10 @@ func resolveParent( if err != nil { return parentInfo{}, err } - pmf := frontmatter.Parse(parentPath, string(data)) + pmf, err := frontmatter.Parse(parentPath, string(data)) + if err != nil { + return parentInfo{}, fmt.Errorf("parent %s: %w", parentValue, err) + } pID := pmf.PageID() if pID == "" { return parentInfo{}, fmt.Errorf("parent not yet published (no page_id): %s", parentValue) diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index 1742a80..80ec5ce 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -94,14 +94,20 @@ func TestCheckPageIDLocalCases(t *testing.T) { } func TestResolveTitle(t *testing.T) { - mf := frontmatter.Parse("f.md", "---\ntitle: FM Title\n---\nb\n") + mf, err := frontmatter.Parse("f.md", "---\ntitle: FM Title\n---\nb\n") + if err != nil { + t.Fatal(err) + } if got := resolveTitle("CLI Title", mf); got != "CLI Title" { t.Errorf("flag override = %q, want CLI Title", got) } if got := resolveTitle("", mf); got != "FM Title" { t.Errorf("frontmatter = %q, want FM Title", got) } - empty := frontmatter.Parse("f.md", "body, no frontmatter\n") + empty, err := frontmatter.Parse("f.md", "body, no frontmatter\n") + if err != nil { + t.Fatal(err) + } if got := resolveTitle("", empty); got != "" { t.Errorf("absent = %q, want empty", got) } diff --git a/cmd/root.go b/cmd/root.go index 05e891c..b726600 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -10,6 +10,7 @@ import ( "github.com/mozilla/markfluence/cmd/attachmentdownload" "github.com/mozilla/markfluence/cmd/attachmentlist" "github.com/mozilla/markfluence/cmd/attachmentupload" + "github.com/mozilla/markfluence/cmd/check" "github.com/mozilla/markfluence/cmd/children" "github.com/mozilla/markfluence/cmd/create" "github.com/mozilla/markfluence/cmd/export" @@ -153,6 +154,7 @@ func init() { rootCmd.AddCommand(update.Cmd) rootCmd.AddCommand(create.Cmd) rootCmd.AddCommand(fix.Cmd) + rootCmd.AddCommand(check.Cmd) rootCmd.AddCommand(info.Cmd) rootCmd.AddCommand(read.Cmd) rootCmd.AddCommand(children.Cmd) diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index 99a46d3..c83f732 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -20,7 +20,10 @@ import ( ) func TestResolveTitlePageID(t *testing.T) { - mf := frontmatter.Parse("f.md", "---\ntitle: FM Title\npage_id: 111\n---\nbody\n") + mf, err := frontmatter.Parse("f.md", "---\ntitle: FM Title\npage_id: 111\n---\nbody\n") + if err != nil { + t.Fatal(err) + } tests := []struct { name string @@ -44,7 +47,10 @@ func TestResolveTitlePageID(t *testing.T) { } func TestResolveTitlePageIDEmptyWhenAbsent(t *testing.T) { - mf := frontmatter.Parse("f.md", "body only, no frontmatter\n") + mf, err := frontmatter.Parse("f.md", "body only, no frontmatter\n") + if err != nil { + t.Fatal(err) + } title, pageID := resolveTitlePageID("", "", mf) if title != "" || pageID != "" { t.Errorf("resolveTitlePageID = %q/%q, want empty/empty", title, pageID) @@ -52,9 +58,18 @@ func TestResolveTitlePageIDEmptyWhenAbsent(t *testing.T) { } func TestResolveWidth(t *testing.T) { - withFM := frontmatter.Parse("f.md", "---\ntitle: T\npage_width: wide\n---\nb\n") - noWidth := frontmatter.Parse("f.md", "---\ntitle: T\n---\nb\n") - noFM := frontmatter.Parse("f.md", "b\n") + withFM, err := frontmatter.Parse("f.md", "---\ntitle: T\npage_width: wide\n---\nb\n") + if err != nil { + t.Fatal(err) + } + noWidth, err := frontmatter.Parse("f.md", "---\ntitle: T\n---\nb\n") + if err != nil { + t.Fatal(err) + } + noFM, err := frontmatter.Parse("f.md", "b\n") + if err != nil { + t.Fatal(err) + } t.Run("flag overrides frontmatter", func(t *testing.T) { w, apply, err := resolveWidth("narrow", withFM) diff --git a/docs/guarantees.md b/docs/guarantees.md index 591fe72..016483f 100644 --- a/docs/guarantees.md +++ b/docs/guarantees.md @@ -205,17 +205,37 @@ nothing is computing a wrong answer. | | label | guarantee | status | |---|---|---|---| -| **R1** | `report-unresolved-references` | Every reference markfluence could not resolve is reported. | Partial | +| **R1** | `report-unresolved-references` | Every reference markfluence could not resolve is reported. | Holds | | **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. +resolve first landed in the same `warnings` list an unresolved image already +used (`_plans/026` commit 5) — Partial rather than Holds, because that was a +minimal warning reusing an existing mechanism rather than the dedicated +diagnostic (distinguishing *why* a reference failed, auditing a tree without +publishing) `_plans/025` gestured at and left for later. + +That dedicated diagnostic now exists (#42): a doc-link target that's missing +entirely or resolves outside the documentation root is Broken and replaces +the published element, exactly as a broken image already does; one that +exists but has no `page_id` yet, or whose `#fragment` matches no heading, +warns instead of publishing silently. Every message carries the source line +it came from when one is findable — a link/image with no visible text at all +has no `*ast.Text` to walk to, and reports the message unprefixed rather than +a wrong line (`nodeLine`'s documented `ok=false` case). `check` adds the +"auditing a tree without publishing" half — the same diagnostics without +ever touching Confluence or the filesystem outside reading. + +R1 is scoped to the two reference kinds markfluence actually attempts to +resolve: doc-links (`.md` siblings) and images. A relative link to a local +non-`.md`, non-image file (e.g. a PDF) gets no existence check at all, +before or after #42 — `rewriteDocLink` only ever attempts resolution for +hrefs ending in `.md`, so that case is never a resolution attempt in the +first place, by design, since only images are uploaded and a relative href +to anything else would be dead regardless. That sits outside R1's claim +rather than inside it unmet, so it does not block Holds. ## Non-goals diff --git a/internal/convert/convert.go b/internal/convert/convert.go index 7758f40..0720da0 100644 --- a/internal/convert/convert.go +++ b/internal/convert/convert.go @@ -77,6 +77,12 @@ func MdToConfluence( baseURL: baseURL, spaceKey: spaceKey, index: index, + // goldmark parses md.Body, not md.Content -- every position it + // reports is relative to the file with its frontmatter block already + // stripped. lineOffset is that block's own line count, added back so + // a reported line matches what a reader sees opening the file, not + // what the parser sees after Extract already removed the header. + lineOffset: strings.Count(md.Content[:len(md.Content)-len(md.Body)], "\n"), } var buf bytes.Buffer if err := newMarkdown(r).Convert([]byte(shielded), &buf); err != nil { diff --git a/internal/convert/images.go b/internal/convert/images.go index 9b8a03f..0c31e94 100644 --- a/internal/convert/images.go +++ b/internal/convert/images.go @@ -37,8 +37,9 @@ func (r *storageRenderer) renderImage( if src == "" { return ast.WalkSkipChildren, nil } + prefix := r.linePrefix(node, source) alt := nodeText(node, source) - attrs := r.parseImageTitle(string(n.Title), src) + attrs := r.parseImageTitle(prefix, string(n.Title), src) // Remote and unsupported-extension images are decided on src/fsPath alone // and never touch the filesystem -- checked and returned before any of the @@ -55,7 +56,7 @@ func (r *storageRenderer) renderImage( // 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) + msg := prefix + fmt.Sprintf("IMAGE BROKEN: %s (unsupported type)", src) r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) return ast.WalkSkipChildren, nil @@ -83,19 +84,19 @@ func (r *storageRenderer) renderImage( switch { case escapesRoot: - msg := fmt.Sprintf("IMAGE BROKEN: %s (outside the documentation root)", src) + msg := prefix + fmt.Sprintf("IMAGE BROKEN: %s (outside the documentation root)", src) r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) 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) + msg := prefix + 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) + msg := prefix + fmt.Sprintf("IMAGE BROKEN: %s (symlink, not a regular file)", src) r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) @@ -117,8 +118,9 @@ func (r *storageRenderer) renderImage( // parseImageTitle turns a markdown image title into extra attributes. // A JSON object supplies title/width/height/align; anything else becomes a plain -// tooltip (title). Invalid width/height/align values are dropped with a warning. -func (r *storageRenderer) parseImageTitle(titleRaw, src string) map[string]string { +// tooltip (title). Invalid width/height/align values are dropped with a warning, +// prefixed with linePrefix's line (computed by the caller, which holds the node). +func (r *storageRenderer) parseImageTitle(prefix, titleRaw, src string) map[string]string { if titleRaw == "" { return nil } @@ -144,7 +146,7 @@ func (r *storageRenderer) parseImageTitle(titleRaw, src string) map[string]strin attrs[dim] = s } else { r.warnings = append(r.warnings, - fmt.Sprintf("%s: ignoring %s=%s (must be a number)", src, dim, pyRepr(v))) + prefix+fmt.Sprintf("%s: ignoring %s=%s (must be a number)", src, dim, pyRepr(v))) } } if v, ok := data["align"]; ok { @@ -153,7 +155,7 @@ func (r *storageRenderer) parseImageTitle(titleRaw, src string) map[string]strin attrs["align"] = s } else { r.warnings = append(r.warnings, - fmt.Sprintf("%s: ignoring align=%s (must be left, center, or right)", src, pyRepr(v))) + prefix+fmt.Sprintf("%s: ignoring align=%s (must be left, center, or right)", src, pyRepr(v))) } } } diff --git a/internal/convert/links.go b/internal/convert/links.go index c2651fe..875a1a9 100644 --- a/internal/convert/links.go +++ b/internal/convert/links.go @@ -2,6 +2,7 @@ package convert import ( "fmt" + "html" "net/url" "path/filepath" "strings" @@ -12,17 +13,31 @@ import ( ) // 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. +// Confluence ids and sibling .md links to their published Confluence URLs. +// Links it does not rewrite fall back to the default goldmark rendering. A +// link whose target is Broken (not merely unresolved) replaces the whole +// element -- tags and visible text alike -- with literal "LINK BROKEN: ..." +// text, matching images.go's precedent for a missing image: this is a defect +// shipped to readers, not just a diagnostic. func (r *storageRenderer) renderLink( - w util.BufWriter, _ []byte, node ast.Node, entering bool, + w util.BufWriter, source []byte, node ast.Node, entering bool, ) (ast.WalkStatus, error) { n := node.(*ast.Link) if !entering { + if r.linkBrokenText != "" { + // entering wrote the replacement text and skipped children; no + // was opened, so there is nothing to close. + return ast.WalkContinue, nil + } _, _ = w.WriteString("") return ast.WalkContinue, nil } - href, rewritten := r.rewriteHref(string(n.Destination)) + href, rewritten, brokenText := r.rewriteHref(string(n.Destination), node, source) + r.linkBrokenText = brokenText + if brokenText != "" { + _, _ = w.WriteString(html.EscapeString(brokenText)) + return ast.WalkSkipChildren, nil + } _, _ = w.WriteString(`= 0 { path, fragment = href[:i], href[i:] } if !strings.HasSuffix(path, ".md") { - return "", false + return "", false, "" } if strings.Contains(path, "://") || strings.HasPrefix(path, "//") { - return "", false + return "", false, "" } - entry, ok := r.index.Page(r.resolveDocKey(path)) - if !ok { - r.warnings = append(r.warnings, fmt.Sprintf("link not resolved: %s", href)) - return "", false + key, escapes := r.resolveDocKey(path) + entry, found := r.index.Page(key) + if !found { + switch { + case escapes: + return "", false, r.reportLinkBroken(prefix, href, "outside the documentation root") + case !r.index.FileExists(key): + return "", false, r.reportLinkBroken(prefix, href, "not found") + default: + // The file exists but has no page_id yet -- the normal state of + // every page in a tree that hasn't been published, not an error. + r.warnings = append(r.warnings, prefix+fmt.Sprintf("link not resolved: %s", href)) + return "", false, "" + } } - var newHref string + var built string if r.spaceKey != "" { slug := "" if entry.Title != "" { slug = url.QueryEscape(entry.Title) } - newHref = fmt.Sprintf("%s/wiki/spaces/%s/pages/%s/%s", + built = fmt.Sprintf("%s/wiki/spaces/%s/pages/%s/%s", r.baseURL, r.spaceKey, entry.PageID, slug) } else { - newHref = fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", r.baseURL, entry.PageID) + built = fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", r.baseURL, entry.PageID) } - return newHref + fragment, true + return built + fragment, true, "" +} + +// warnAnchorNotFound records an anchor-miss warning, the one message shared +// by both branches in rewriteHref (same-page and cross-file). +func (r *storageRenderer) warnAnchorNotFound(prefix, href string) { + r.warnings = append(r.warnings, prefix+fmt.Sprintf("anchor not found: %s", href)) +} + +// reportLinkBroken records a Broken doc-link message -- reason is "not +// found" or "outside the documentation root", rewriteDocLink's two cases -- +// and returns it for the caller to use as brokenText. +func (r *storageRenderer) reportLinkBroken(prefix, href, reason string) string { + msg := prefix + fmt.Sprintf("LINK BROKEN: %s (%s)", href, reason) + r.broken = append(r.broken, msg) + return msg } // 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. +// root-relative, slash-separated path the link index is keyed by, and +// whether that path escapes the documentation root. // -// 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 { +// The index itself needs no clamp: it 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 (025's S2 +// discussion). escapes exists only so a caller can tell that guaranteed miss +// apart from a genuine "not found" for Broken-severity reporting -- a purely +// lexical check, since nothing here reads the filesystem the way images.go's +// os.Root-backed check does. +func (r *storageRenderer) resolveDocKey(dest string) (key string, escapes bool) { abs, err := filepath.Abs(filepath.Join(r.baseDir, decodeDestination(dest))) if err != nil { - return "" + return "", false } - key, _ := rootRelativeKey(r.root, abs) - return key + key, _ = rootRelativeKey(r.root, abs) + return key, key == ".." || strings.HasPrefix(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 58f8775..df9ad11 100644 --- a/internal/convert/links_test.go +++ b/internal/convert/links_test.go @@ -18,33 +18,38 @@ func TestResolveDocKeyDecodesBeforeResolving(t *testing.T) { r := &storageRenderer{baseDir: root, root: &project.Root{Dir: root}} cases := []struct { - dest string - key string + dest string + key string + escapes bool }{ - {"plain.md", "plain.md"}, - {"docs/plain.md", "docs/plain.md"}, - {"../plain.md", "../plain.md"}, // escapes root -- returned as-is, not refused + {"plain.md", "plain.md", false}, + {"docs/plain.md", "docs/plain.md", false}, + {"../plain.md", "../plain.md", true}, // 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", "docs/my doc.md"}, - {"./my%20doc.md", "my doc.md"}, + {"my%20doc.md", "my doc.md", false}, + {"docs/my%20doc.md", "docs/my doc.md", false}, + {"./my%20doc.md", "my doc.md", false}, // Non-ASCII filenames encode the same way. - {"caf%C3%A9.md", "café.md"}, + {"caf%C3%A9.md", "café.md", false}, // A literal "%" in a filename is not an escape sequence, so an // undecodable destination is a filename as written. - {"100%.md", "100%.md"}, - {"50%off.md", "50%off.md"}, + {"100%.md", "100%.md", false}, + {"50%off.md", "50%off.md", false}, // A destination that merely looks encoded resolves to the literal name. - {"my%2520doc.md", "my%20doc.md"}, + {"my%2520doc.md", "my%20doc.md", false}, } for _, c := range cases { - if got := r.resolveDocKey(c.dest); got != c.key { + got, escapes := r.resolveDocKey(c.dest) + if got != c.key { t.Errorf("resolveDocKey(%q) = %q, want %q", c.dest, got, c.key) } + if escapes != c.escapes { + t.Errorf("resolveDocKey(%q) escapes = %v, want %v", c.dest, escapes, c.escapes) + } } } @@ -61,7 +66,8 @@ func TestResolveDocKeyAgreesOnBothSpellings(t *testing.T) { {"docs/my%20doc.md", "docs/my doc.md"}, {"caf%C3%A9.md", "café.md"}, } { - encoded, literal := r.resolveDocKey(pair[0]), r.resolveDocKey(pair[1]) + encoded, _ := r.resolveDocKey(pair[0]) + literal, _ := r.resolveDocKey(pair[1]) if encoded != literal { t.Errorf("resolveDocKey(%q) = %q but resolveDocKey(%q) = %q; both spellings must agree", pair[0], encoded, pair[1], literal) @@ -80,8 +86,8 @@ func TestResolveDocKeyDistinguishesSameBasenameInDifferentDirectories(t *testing 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") + 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) } @@ -96,15 +102,17 @@ func TestResolveDocKeyDistinguishesSameBasenameInDifferentDirectories(t *testing // 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. +// would resolve to from its own directory, and neither is an escape: "../" +// syntax in the destination doesn't mean the resolved path leaves root, only +// that it climbs above the referencing file's 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, escapes := fromTeam.resolveDocKey("../index.md"); got != "index.md" || escapes { + t.Errorf("resolveDocKey(../index.md) = %q, escapes %v, want %q, false", got, escapes, "index.md") } - if got, want := fromTeam.resolveDocKey("../ops/runbook.md"), "ops/runbook.md"; got != want { - t.Errorf("resolveDocKey(../ops/runbook.md) = %q, want %q", got, want) + if got, escapes := fromTeam.resolveDocKey("../ops/runbook.md"); got != "ops/runbook.md" || escapes { + t.Errorf("resolveDocKey(../ops/runbook.md) = %q, escapes %v, want %q, false", got, escapes, "ops/runbook.md") } } diff --git a/internal/convert/nodeline_test.go b/internal/convert/nodeline_test.go new file mode 100644 index 0000000..5bea549 --- /dev/null +++ b/internal/convert/nodeline_test.go @@ -0,0 +1,110 @@ +package convert + +import ( + "testing" + + "github.com/yuin/goldmark" + "github.com/yuin/goldmark/ast" + "github.com/yuin/goldmark/text" +) + +// firstNode returns the first descendant of kind in source's parsed AST, or +// nil if there isn't one. +func firstNode(source []byte, kind ast.NodeKind) ast.Node { + doc := goldmark.DefaultParser().Parse(text.NewReader(source)) + var found ast.Node + _ = ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if entering && found == nil && n.Kind() == kind { + found = n + return ast.WalkStop, nil + } + return ast.WalkContinue, nil + }) + return found +} + +func TestNodeLineOnFirstLine(t *testing.T) { + source := []byte("[text](target.md)\n") + link := firstNode(source, ast.KindLink) + if link == nil { + t.Fatal("no link found") + } + r := &storageRenderer{} + if line, ok := r.nodeLine(link, source); !ok || line != 1 { + t.Errorf("nodeLine = %d, %v, want 1, true", line, ok) + } +} + +func TestNodeLineSeveralLinesDown(t *testing.T) { + source := []byte("para one\n\npara two\n\n[text](target.md)\n") + link := firstNode(source, ast.KindLink) + if link == nil { + t.Fatal("no link found") + } + r := &storageRenderer{} + if line, ok := r.nodeLine(link, source); !ok || line != 5 { + t.Errorf("nodeLine = %d, %v, want 5, true", line, ok) + } +} + +// TestNodeLineInsideNestedConstruct covers a link nested three levels deep +// (blockquote > list item > paragraph) -- nodeLine walks to the first +// descendant *ast.Text regardless of how many container levels sit between +// it and the node passed in. +func TestNodeLineInsideNestedConstruct(t *testing.T) { + source := []byte("> - [text](target.md)\n") + link := firstNode(source, ast.KindLink) + if link == nil { + t.Fatal("no link found") + } + r := &storageRenderer{} + if line, ok := r.nodeLine(link, source); !ok || line != 1 { + t.Errorf("nodeLine = %d, %v, want 1, true", line, ok) + } +} + +// TestNodeLineNoTextDescendantIsNotOK is the fallback path: a node with no +// *ast.Text descendant at all (constructed directly rather than parsed -- +// whether "[]( target.md )" itself ever produces a childless Link node is a +// goldmark implementation detail this test shouldn't depend on) must report +// ok=false rather than a wrong line. +func TestNodeLineNoTextDescendantIsNotOK(t *testing.T) { + link := ast.NewLink() + r := &storageRenderer{} + if line, ok := r.nodeLine(link, []byte("irrelevant")); ok { + t.Errorf("nodeLine = %d, true, want ok=false for a childless node", line) + } +} + +// TestNodeLineAppliesLineOffset is what makes a reported line match the file +// a reader opens, not the frontmatter-stripped body goldmark actually parses. +func TestNodeLineAppliesLineOffset(t *testing.T) { + source := []byte("[text](target.md)\n") + link := firstNode(source, ast.KindLink) + if link == nil { + t.Fatal("no link found") + } + r := &storageRenderer{lineOffset: 4} + if line, ok := r.nodeLine(link, source); !ok || line != 5 { + t.Errorf("nodeLine = %d, %v, want 5, true", line, ok) + } +} + +func TestLinePrefixEmptyWhenLineNotFound(t *testing.T) { + r := &storageRenderer{} + if got := r.linePrefix(ast.NewLink(), []byte("irrelevant")); got != "" { + t.Errorf("linePrefix = %q, want empty", got) + } +} + +func TestLinePrefixFormat(t *testing.T) { + source := []byte("[text](target.md)\n") + link := firstNode(source, ast.KindLink) + if link == nil { + t.Fatal("no link found") + } + r := &storageRenderer{} + if got, want := r.linePrefix(link, source), "line 1: "; got != want { + t.Errorf("linePrefix = %q, want %q", got, want) + } +} diff --git a/internal/convert/renderer.go b/internal/convert/renderer.go index e415323..45daf4f 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -47,6 +47,58 @@ type storageRenderer struct { broken []string warnings []string seen map[string]bool + + // linkBrokenText is the literal replacement text for the *ast.Link + // currently being rendered, set on entering when its target is Broken and + // cleared (empty) otherwise; renderLink's matching leaving call reads it + // once to decide whether a closing "" is due. Per-node transient + // state, the same shape as seen above -- safe because goldmark never + // renders two Link nodes concurrently (markdown has no nested links). + linkBrokenText string + + // lineOffset is the number of lines the frontmatter block consumed in the + // original file, added to every line nodeLine reports: goldmark parses + // md.Body, which Extract already stripped of that block, so every + // position it sees is relative to the body, not the file a reader would + // open and count lines in. + lineOffset int +} + +// nodeLine returns the 1-indexed source line n starts on -- in the original +// file, frontmatter included via lineOffset -- and whether one could be +// found. Neither *ast.Link nor *ast.Image carries its own position -- +// goldmark's parser never calls SetLines on either -- so this walks to the +// first descendant *ast.Text, which does, via the same Segment nodeText +// already reads. ok is false for a node with no text descendant at all (e.g. +// an empty link), in which case a caller must not report a line at all +// rather than a wrong one. +func (r *storageRenderer) nodeLine(n ast.Node, source []byte) (line int, ok bool) { + offset := -1 + _ = ast.Walk(n, func(c ast.Node, entering bool) (ast.WalkStatus, error) { + if !entering || offset >= 0 { + return ast.WalkContinue, nil + } + if t, isText := c.(*ast.Text); isText { + offset = t.Segment.Start + return ast.WalkStop, nil + } + return ast.WalkContinue, nil + }) + if offset < 0 { + return 0, false + } + return r.lineOffset + 1 + bytes.Count(source[:offset], []byte("\n")), true +} + +// linePrefix returns "line %d: " for a node whose source line nodeLine can +// find, or "" otherwise -- callers prepend the result to a diagnostic +// message unconditionally, so a message that can't be located reads exactly +// as it did before this existed. +func (r *storageRenderer) linePrefix(n ast.Node, source []byte) string { + if line, ok := r.nodeLine(n, source); ok { + return fmt.Sprintf("line %d: ", line) + } + return "" } // RegisterFuncs registers the node handlers this renderer overrides. diff --git a/internal/convert/storage_to_md_test.go b/internal/convert/storage_to_md_test.go index 313e1ff..3805f72 100644 --- a/internal/convert/storage_to_md_test.go +++ b/internal/convert/storage_to_md_test.go @@ -106,7 +106,10 @@ func TestRoundTripStableCallouts(t *testing.T) { "```", }, "\n") + "\n" - md := frontmatter.Parse("main.md", src) + md, err := frontmatter.Parse("main.md", src) + if err != nil { + t.Fatal(err) + } root := testRoot(t, "") page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { @@ -137,7 +140,10 @@ func TestRoundTripTableAlignment(t *testing.T) { "| a | b | c | d |", }, "\n") + "\n" - md := frontmatter.Parse("main.md", src) + md, err := frontmatter.Parse("main.md", src) + if err != nil { + t.Fatal(err) + } root := testRoot(t, "") page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { @@ -175,7 +181,10 @@ func TestRoundTripTableCellBG(t *testing.T) { "| unknown | |", }, "\n") + "\n" - md := frontmatter.Parse("main.md", src) + md, err := frontmatter.Parse("main.md", src) + if err != nil { + t.Fatal(err) + } root := testRoot(t, "") page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { @@ -225,7 +234,10 @@ func TestStorageToMarkdownJoinsMultilineCells(t *testing.T) { // The
form must itself be stable: publishing it back and exporting // again should reproduce the same markdown (L6, roundtrip-from-disk). - md := frontmatter.Parse("main.md", got) + md, err := frontmatter.Parse("main.md", got) + if err != nil { + t.Fatal(err) + } root := testRoot(t, "") page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { @@ -278,7 +290,10 @@ func TestStorageToMarkdownPassesThroughListsInCells(t *testing.T) { // The passthrough form must itself be stable end to end: publishing the // exported markdown must reproduce the exact storage read in above. - md := frontmatter.Parse("main.md", got) + md, err := frontmatter.Parse("main.md", got) + if err != nil { + t.Fatal(err) + } root := testRoot(t, "") page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { @@ -365,7 +380,10 @@ func TestRoundTripPassthrough(t *testing.T) { if err != nil { t.Fatalf("reading golden: %v", err) } - md := frontmatter.Parse("main.md", string(src)) + md, err := frontmatter.Parse("main.md", string(src)) + if err != nil { + t.Fatal(err) + } root := testRoot(t, "") page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", "vtest") if err != nil { diff --git a/internal/convert/symlink_test.go b/internal/convert/symlink_test.go index 2cf758f..39341fb 100644 --- a/internal/convert/symlink_test.go +++ b/internal/convert/symlink_test.go @@ -38,7 +38,10 @@ func TestRenderImageRefusesSymlinkedLeaf(t *testing.T) { t.Skipf("symlinks unavailable: %v", err) } - md := frontmatter.Parse(filepath.Join(root, "main.md"), "![logo](logo.png)\n") + md, err := frontmatter.Parse(filepath.Join(root, "main.md"), "![logo](logo.png)\n") + if err != nil { + t.Fatal(err) + } r, err := project.FromPath(root) if err != nil { t.Fatal(err) @@ -78,7 +81,10 @@ func TestRenderImageRefusesEscapeThroughSymlinkedDirectory(t *testing.T) { t.Skipf("symlinks unavailable: %v", err) } - md := frontmatter.Parse(filepath.Join(root, "main.md"), "![logo](assets/logo.png)\n") + md, err := frontmatter.Parse(filepath.Join(root, "main.md"), "![logo](assets/logo.png)\n") + if err != nil { + t.Fatal(err) + } r, err := project.FromPath(root) if err != nil { t.Fatal(err) diff --git a/internal/convert/tables.go b/internal/convert/tables.go index 7eeb9e8..1d8bec3 100644 --- a/internal/convert/tables.go +++ b/internal/convert/tables.go @@ -158,7 +158,7 @@ func (t tableCellBGTransformer) warn(cell ast.Node, source []byte, value, proble label = string(runes[:40]) + "..." } t.r.warnings = append(t.r.warnings, - fmt.Sprintf("table cell %q: ignoring bg:%s (%s)", label, value, problem)) + t.r.linePrefix(cell, source)+fmt.Sprintf("table cell %q: ignoring bg:%s (%s)", label, value, problem)) } // cellBGMarker reports whether an inline node is a background marker comment, diff --git a/internal/convert/testdata/regression/doc-links-encoded/main.md b/internal/convert/testdata/regression/doc-links-encoded/main.md index 45606de..c684060 100644 --- a/internal/convert/testdata/regression/doc-links-encoded/main.md +++ b/internal/convert/testdata/regression/doc-links-encoded/main.md @@ -35,8 +35,9 @@ Both halves at once -- encoded filename and encoded fragment: [both encoded](my%20sibling.md#caf%C3%A9-section) -A sibling that is not in this set is left exactly as written, still encoded, -because nothing was resolved and nothing should be rewritten: +A sibling that is not in this set is Broken; the reported message still +echoes the destination as written, encoding and all, rather than a resolved +answer: [unknown](nosuch%20file.md) diff --git a/internal/convert/testdata/regression/doc-links-encoded/test.output b/internal/convert/testdata/regression/doc-links-encoded/test.output index ed76934..8c733a2 100644 --- a/internal/convert/testdata/regression/doc-links-encoded/test.output +++ b/internal/convert/testdata/regression/doc-links-encoded/test.output @@ -1,8 +1,8 @@ { "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": [ - "link not resolved: nosuch%20file.md" - ] + "broken": [ + "line 42: LINK BROKEN: nosuch%20file.md (not found)" + ], + "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 Broken; the reported message still echoes the destination as written, encoding and all, rather than a resolved answer:

\n

line 42: LINK BROKEN: nosuch%20file.md (not found)

\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": [] } diff --git a/internal/convert/testdata/regression/image-properties/test.output b/internal/convert/testdata/regression/image-properties/test.output index af041dc..8a4b542 100644 --- a/internal/convert/testdata/regression/image-properties/test.output +++ b/internal/convert/testdata/regression/image-properties/test.output @@ -9,7 +9,7 @@ "broken": [], "html": "

Image Properties

\n

A JSON title sets title/width/height/align attributes:

\n

\n

A plain-string title becomes a tooltip (ac:title):

\n

\n

Invalid width/align values are dropped with warnings:

\n

\n", "warnings": [ - "assets/shot.png: ignoring width='wide' (must be a number)", - "assets/shot.png: ignoring align='middle' (must be left, center, or right)" + "line 13: assets/shot.png: ignoring width='wide' (must be a number)", + "line 13: assets/shot.png: ignoring align='middle' (must be left, center, or right)" ] } diff --git a/internal/convert/testdata/regression/images-broken/test.output b/internal/convert/testdata/regression/images-broken/test.output index b012a92..9513403 100644 --- a/internal/convert/testdata/regression/images-broken/test.output +++ b/internal/convert/testdata/regression/images-broken/test.output @@ -1,10 +1,10 @@ { "attachments": [], "broken": [ - "IMAGE BROKEN: assets/missing.png (not found)", - "IMAGE BROKEN: notes.pdf (unsupported type)", - "IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)" + "line 5: IMAGE BROKEN: assets/missing.png (not found)", + "line 9: IMAGE BROKEN: notes.pdf (unsupported type)", + "line 13: IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)" ], - "html": "

Broken Images

\n

A reference to a file that isn't there:

\n

IMAGE BROKEN: assets/missing.png (not found)

\n

A reference to a file with an unsupported extension:

\n

IMAGE BROKEN: notes.pdf (unsupported type)

\n

A reference to a file above the documentation root:

\n

IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)

\n", + "html": "

Broken Images

\n

A reference to a file that isn't there:

\n

line 5: IMAGE BROKEN: assets/missing.png (not found)

\n

A reference to a file with an unsupported extension:

\n

line 9: IMAGE BROKEN: notes.pdf (unsupported type)

\n

A reference to a file above the documentation root:

\n

line 13: IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)

\n", "warnings": [] } diff --git a/internal/convert/testdata/regression/images-encoded-src/test.output b/internal/convert/testdata/regression/images-encoded-src/test.output index 2deb314..079a198 100644 --- a/internal/convert/testdata/regression/images-encoded-src/test.output +++ b/internal/convert/testdata/regression/images-encoded-src/test.output @@ -22,8 +22,8 @@ } ], "broken": [ - "IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.png (outside the documentation root)" + "line 37: IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.png (outside the documentation root)" ], - "html": "

Encoded Image Sources

\n

A markdown image destination is a URL, not a path, so a filename with a space has to be percent-encoded to be referenced at all. This is the spelling most editors and previews produce, and the one this case exists for:

\n

\n

The angle-bracket form is the same image by another spelling, and resolves to the same attachment:

\n

\n

A bare space is not a valid destination, so this is not an image at all -- it stays literal text, exactly as GitHub and a local preview render it. Nothing is uploaded and nothing is reported broken, because no image was ever parsed:

\n

![bare space](assets/my image.png)

\n

Non-ASCII filenames encode the same way:

\n

\n

A literal "%" in a filename is not an escape sequence. It is left as written, so a file genuinely named "100%.png" still resolves:

\n

\n

An ordinary path is unaffected:

\n

\n

An encoded "../" is decoded before the documentation-root check runs, so the encoding cannot slip an escaping path past it -- compare the plain spelling in the images-broken case, which is refused for the same reason:

\n

IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.png (outside the documentation root)

\n", + "html": "

Encoded Image Sources

\n

A markdown image destination is a URL, not a path, so a filename with a space has to be percent-encoded to be referenced at all. This is the spelling most editors and previews produce, and the one this case exists for:

\n

\n

The angle-bracket form is the same image by another spelling, and resolves to the same attachment:

\n

\n

A bare space is not a valid destination, so this is not an image at all -- it stays literal text, exactly as GitHub and a local preview render it. Nothing is uploaded and nothing is reported broken, because no image was ever parsed:

\n

![bare space](assets/my image.png)

\n

Non-ASCII filenames encode the same way:

\n

\n

A literal "%" in a filename is not an escape sequence. It is left as written, so a file genuinely named "100%.png" still resolves:

\n

\n

An ordinary path is unaffected:

\n

\n

An encoded "../" is decoded before the documentation-root check runs, so the encoding cannot slip an escaping path past it -- compare the plain spelling in the images-broken case, which is refused for the same reason:

\n

line 37: IMAGE BROKEN: ..%2F..%2F..%2F..%2F..%2F..%2F..%2F..%2Fetc%2Fpasswd.png (outside the documentation root)

\n", "warnings": [] } diff --git a/internal/convert/testdata/regression/internal-doc-links/main.md b/internal/convert/testdata/regression/internal-doc-links/main.md index 072d12b..7a99f61 100644 --- a/internal/convert/testdata/regression/internal-doc-links/main.md +++ b/internal/convert/testdata/regression/internal-doc-links/main.md @@ -3,7 +3,7 @@ A link to a sibling doc is rewritten to its Confluence URL: [the sibling page](sibling.md). -A link to a doc that isn't in this set is left untouched: +A link to a doc that doesn't exist at all is Broken: [unknown](unknown.md). An absolute URL is never rewritten: [external](https://example.net/page.md). diff --git a/internal/convert/testdata/regression/internal-doc-links/test.output b/internal/convert/testdata/regression/internal-doc-links/test.output index 1a726e8..2aaa0c8 100644 --- a/internal/convert/testdata/regression/internal-doc-links/test.output +++ b/internal/convert/testdata/regression/internal-doc-links/test.output @@ -1,8 +1,8 @@ { "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": [ - "link not resolved: unknown.md" - ] + "broken": [ + "line 7: LINK BROKEN: unknown.md (not found)" + ], + "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 doesn't exist at all is Broken: line 7: LINK BROKEN: unknown.md (not found).

\n

An absolute URL is never rewritten: external.

\n", + "warnings": [] } diff --git a/internal/convert/testdata/regression/link-anchor-not-found/main.md b/internal/convert/testdata/regression/link-anchor-not-found/main.md new file mode 100644 index 0000000..bcd069d --- /dev/null +++ b/internal/convert/testdata/regression/link-anchor-not-found/main.md @@ -0,0 +1,20 @@ +--- +page_id: 9001 +title: Anchor Not Found +--- +# Anchor Not Found + +A same-page anchor that matches no heading is a Warning; the href renders +exactly as written since nothing was resolved: + +[bad same-page anchor](#no-such-heading) + +A cross-file anchor that matches no heading on an otherwise-resolvable +sibling warns the same way -- distinct from the sibling not existing at all, +which rewriteDocLink already reports separately: + +[bad cross-file anchor](sibling.md#no-such-heading) + +## Real Section + +The only real heading in this document. diff --git a/internal/convert/testdata/regression/link-anchor-not-found/sibling.md b/internal/convert/testdata/regression/link-anchor-not-found/sibling.md new file mode 100644 index 0000000..a74594c --- /dev/null +++ b/internal/convert/testdata/regression/link-anchor-not-found/sibling.md @@ -0,0 +1,9 @@ +--- +page_id: 9002 +title: Sibling +--- +# Sibling + +## Actual Heading + +The heading that exists; the link in main.md deliberately names a different one. diff --git a/internal/convert/testdata/regression/link-anchor-not-found/test.input b/internal/convert/testdata/regression/link-anchor-not-found/test.input new file mode 100644 index 0000000..0849cc2 --- /dev/null +++ b/internal/convert/testdata/regression/link-anchor-not-found/test.input @@ -0,0 +1,3 @@ +{ + "files": ["main.md", "sibling.md"] +} diff --git a/internal/convert/testdata/regression/link-anchor-not-found/test.output b/internal/convert/testdata/regression/link-anchor-not-found/test.output new file mode 100644 index 0000000..82e7a81 --- /dev/null +++ b/internal/convert/testdata/regression/link-anchor-not-found/test.output @@ -0,0 +1,9 @@ +{ + "attachments": [], + "broken": [], + "html": "

Anchor Not Found

\n

A same-page anchor that matches no heading is a Warning; the href renders exactly as written since nothing was resolved:

\n

bad same-page anchor

\n

A cross-file anchor that matches no heading on an otherwise-resolvable sibling warns the same way -- distinct from the sibling not existing at all, which rewriteDocLink already reports separately:

\n

bad cross-file anchor

\n

Real Section

\n

The only real heading in this document.

\n", + "warnings": [ + "line 10: anchor not found: #no-such-heading", + "line 16: anchor not found: sibling.md#no-such-heading" + ] +} diff --git a/internal/convert/testdata/regression/link-not-yet-published/draft.md b/internal/convert/testdata/regression/link-not-yet-published/draft.md new file mode 100644 index 0000000..b4bbe02 --- /dev/null +++ b/internal/convert/testdata/regression/link-not-yet-published/draft.md @@ -0,0 +1,3 @@ +# Draft + +Not yet published; no `page_id` in its frontmatter. diff --git a/internal/convert/testdata/regression/link-not-yet-published/main.md b/internal/convert/testdata/regression/link-not-yet-published/main.md new file mode 100644 index 0000000..24a792b --- /dev/null +++ b/internal/convert/testdata/regression/link-not-yet-published/main.md @@ -0,0 +1,8 @@ +# Link Not Yet Published + +A link to a sibling that exists on disk but has no `page_id` yet is a +Warning, not Broken -- the normal state of every page in a tree that hasn't +been published, not a defect. The href renders exactly as written, unlike a +genuinely missing or escaping target: + +[the draft](draft.md) diff --git a/internal/convert/testdata/regression/link-not-yet-published/test.input b/internal/convert/testdata/regression/link-not-yet-published/test.input new file mode 100644 index 0000000..8e060b9 --- /dev/null +++ b/internal/convert/testdata/regression/link-not-yet-published/test.input @@ -0,0 +1,3 @@ +{ + "files": ["main.md", "draft.md"] +} diff --git a/internal/convert/testdata/regression/link-not-yet-published/test.output b/internal/convert/testdata/regression/link-not-yet-published/test.output new file mode 100644 index 0000000..9d42b6d --- /dev/null +++ b/internal/convert/testdata/regression/link-not-yet-published/test.output @@ -0,0 +1,8 @@ +{ + "attachments": [], + "broken": [], + "html": "

Link Not Yet Published

\n

A link to a sibling that exists on disk but has no page_id yet is a Warning, not Broken -- the normal state of every page in a tree that hasn't been published, not a defect. The href renders exactly as written, unlike a genuinely missing or escaping target:

\n

the draft

\n", + "warnings": [ + "line 8: link not resolved: draft.md" + ] +} diff --git a/internal/convert/testdata/regression/link-outside-root/docs/link.md b/internal/convert/testdata/regression/link-outside-root/docs/link.md index c46cbe8..bb1597d 100644 --- a/internal/convert/testdata/regression/link-outside-root/docs/link.md +++ b/internal/convert/testdata/regression/link-outside-root/docs/link.md @@ -1,8 +1,9 @@ # 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. +Broken: it publishes as literal "LINK BROKEN: ... (outside the documentation +root)" text in place of the link element and its visible text, because the +target sits above root, so the link index never walked it. The query side +tells this apart from a genuine "not found" via a lexical escape check +(025's Scenario F); the index itself 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/test.output b/internal/convert/testdata/regression/link-outside-root/test.output index df1cdfa..c3a0d6e 100644 --- a/internal/convert/testdata/regression/link-outside-root/test.output +++ b/internal/convert/testdata/regression/link-outside-root/test.output @@ -1,8 +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" - ] + "broken": [ + "line 3: LINK BROKEN: ../outside/linked.md (outside the documentation root)" + ], + "html": "

Escaping Link

\n

A line 3: LINK BROKEN: ../outside/linked.md (outside the documentation root) is Broken: it publishes as literal "LINK BROKEN: ... (outside the documentation root)" text in place of the link element and its visible text, because the target sits above root, so the link index never walked it. The query side tells this apart from a genuine "not found" via a lexical escape check (025's Scenario F); the index itself still needs no clamp, since a path outside root is simply never in the index in the first place.

\n", + "warnings": [] } diff --git a/internal/convert/testdata/regression/table-cell-colors/test.output b/internal/convert/testdata/regression/table-cell-colors/test.output index 0243a68..5062ac4 100644 --- a/internal/convert/testdata/regression/table-cell-colors/test.output +++ b/internal/convert/testdata/regression/table-cell-colors/test.output @@ -3,7 +3,7 @@ "broken": [], "html": "

Table Cell Colors

\n

A leading bg: comment sets a cell background, by swatch name or hex, in body cells and header cells alike:

\n
...`, one element per line already); it + just never indents by depth. A per-line indent (count open/closed tags at + each line's start, prefix accordingly) gets full readability without a + whitespace-normalizing reformatter, which would risk altering meaningful + inline text mixed into a block. +- the attachment list (filename → source path), when non-empty. + +``` +$ markfluence check --show-html docs/table-example.md + +[docs/table-example.md] clean +[docs/table-example.md] --- storage HTML --- + + + + + + + +
ServiceStatus
+[docs/table-example.md] --- attachments --- +diagram.png -> assets/diagram.png +``` + +In `--json`, this is the `debug` object on `checkResult` above: `null` for a +`failed` file or when the flag wasn't passed; otherwise `{html, +attachments}`, present on every result either way per the schema's +no-`omitempty` rule. `debug.html` stays the compact, unindented string the +converter actually produced — indentation is a human-output display concern, +not a data one, and inserting it into the JSON value would mean the field no +longer matches what `update`/`create` would literally publish. + +### Human output + +Mirrors `fix`'s `renderHuman`: a `[file]`-prefixed line per broken item +(`ui.Error`) and warning (`ui.Warn`), and a plain "clean" line when there's +nothing to report. A `failed` file (never got a clean answer at all) prints +just its one error line, the same way `fix` short-circuits on failure. The +batch-level summary line only appears when at least one file has `ok: +false`, matching `fix`'s exact wording and its silence-on-success behavior. + +``` +$ markfluence check docs/intro.md docs/guide.md docs/broken-links.md docs/bad-frontmatter.md + +[docs/intro.md] clean +[docs/guide.md] line 8: link not resolved: sibling-draft.md +[docs/broken-links.md] line 5: LINK BROKEN: ../outside.md (outside the documentation root) +[docs/broken-links.md] line 12: LINK BROKEN: typo-target.md (not found) +[docs/broken-links.md] line 19: anchor not found: overview.md#nonexistent-heading +[docs/bad-frontmatter.md] invalid page_width "huge"; expected narrow, wide, or max + +2 of 4 file(s) failed. +``` + +`docs/guide.md` is `warnings`-status (`ok: true`, exits 0 — an unresolved +sibling link is expected in an unpublished tree). `docs/broken-links.md` is +`broken`. `docs/bad-frontmatter.md` is `failed` — it never reached the +converter at all, which is also why its message has no `line N:` prefix: +frontmatter diagnostics aren't goldmark-AST-based and don't get one (see +above). + +The matching `--json` result for `docs/broken-links.md`: + +```json +{ + "ok": false, + "status": "broken", + "file": "docs/broken-links.md", + "broken": [ + "line 5: LINK BROKEN: ../outside.md (outside the documentation root)", + "line 12: LINK BROKEN: typo-target.md (not found)" + ], + "warnings": [ + "line 19: anchor not found: overview.md#nonexistent-heading" + ], + "debug": null, + "error": null, + "code": null +} +``` + +(`debug` is `null` here because `--show-html` wasn't passed in this example.) + +### R1 (`docs/guarantees.md`) bumps from Partial to Holds + +Per interview: R1 says "every reference markfluence could not resolve is +reported", scoped to the two reference kinds markfluence actually attempts to +resolve — doc-links (`.md` siblings) and images. A relative link to a local +non-`.md`, non-image file (e.g. a PDF) gets zero existence checking, before +or after this PR: `rewriteDocLink` only ever attempts resolution for hrefs +ending in `.md`, so that case is never a resolution attempt at all — by +design, since only images are uploaded and a relative href to anything else +would be dead regardless. That sits outside R1's claim rather than inside it +unmet, so it doesn't block Holds. State this explicitly in the guarantees.md +update rather than letting it be noticed later — this is a status bump with +its own commit message per CLAUDE.md's rule on guarantee statuses, not folded +into another commit. + +## Out of scope (deliberately) + +- **A bare `markfluence check` project-wide scan.** +- **Style/lint rules** (heading levels, line length, prose linting). +- **Any network-based staleness check.** That's `update`'s mtime check. +- **Structured (non-string) broken/warning entries.** + `ConfluencePage.Broken`/`Warnings` stay `[]string`; a line number is a + text prefix (see above), not a `{line, column, message}` field. The + structured version is what a future LSP integration would want for a + `Range`, but nothing here needs it. +- **Existence-checking non-`.md`, non-image relative links** (e.g. a PDF). + Explicitly named in the R1 update as outside its claim, not silently + dropped. + +## Steps + +1. `docs(plans): plan the check command` — this file. +2. `feat(frontmatter): detect an unterminated frontmatter block` — `Parse` + gains an error return; update all six call sites; `linkindex.Build` skips + the file on this error the same way it skips an unreadable one. +3. `feat(linkindex): track file existence independent of page_id` — + `Index.FileExists`. +4. `feat(convert): reject an escaping link the way images.go already does` — + the `../`-prefix check on the query side, `LINK BROKEN: %s (outside the + documentation root)`. +5. `feat(convert): report a missing link target as Broken, not a warning` — + `LINK BROKEN: %s (not found)`, using `FileExists` to distinguish it from + "no `page_id` yet" (stays a warning); restructure `renderLink` to replace + the whole element on Broken (the per-node flag for the stray ``). +6. `feat(convert): warn on a fragment that matches no heading` — both + branches in `rewriteHref`, gated on `FileExists`. +7. `feat(convert): prefix Broken/Warning messages with a source line + number` — the `nodeLine` helper; wire it through `images.go`'s four + `Broken`/two `Warnings` sites and `tables.go`'s `warn`; thread `source` + through `renderLink`/`rewriteHref`/`rewriteDocLink` for the link sites + (new and pre-existing alike); regenerate every regression golden touched + (`make regen-regressions`) and update the README's example messages. +8. `feat(check): add the check command` — `cmd/check/{check,json}.go`, + registration in `root.go`, `completion.MarkdownFiles`, the schema's + `command` enum entry + `if/then` branch + `checkResult`/`checkSummary` + defs, including the `debug` object and the `--show-html` flag (storage + HTML plus the attachment list, with per-line indent-by-depth in human + output). +9. `docs(guarantees): bump R1 to Holds` — with the scope note above. +10. `docs(readme): document the check command and the new link-severity + messages`. +11. `docs: add check to the architecture notes` — CLAUDE.md gains a + `cmd/check/` bullet; the `internal/convert` writeup gains the + link-severity split and the line-number prefix (now affecting + `update`/`create` too, not just `check`); `internal/frontmatter`'s + writeup notes `Parse`'s new error return. + +## Testing + +**`internal/frontmatter`.** New detector: a file with `---\n` and no closing +line is flagged; a file with proper frontmatter, no frontmatter at all, or a +`---` appearing only in the body (e.g. inside a fenced code block) is not a +false positive. `Parse`'s new error propagates through `ParseFile` and all +call sites. + +**`internal/linkindex`.** `FileExists` true for an indexed page, true for a +`page_id`-less file, false for a genuinely absent path; unaffected by +`TestCacheBuildsOncePerRoot`'s memoization contract. `Build` skips (rather +than fails) a sibling with an unterminated frontmatter block. + +**`internal/convert`.** One regression case per severity bucket: link to a +nonexistent file (Broken/not found, output replaced), link that resolves +outside root via `../` (Broken/outside root, output replaced), link to a real +page-id-less sibling (Warning, message and output unchanged), link with a +`#fragment` matching no heading (new Warning, output unchanged). Confirm +`update`/`create` pick these up for free via the shared `ConfluencePage` +fields. `nodeLine`: correct line for a link/image on line 1, a link/image +several lines down, one inside a nested construct (a link inside a list +item inside a blockquote), and `ok=false` (no line prefix) for a node with +no text descendant (e.g. `[](target.md)`). One existing image-Broken and one +existing table-warning regression case confirm the line prefix lands on +already-existing messages too, not just the new link ones. + +**`cmd/check`.** One test per `status` value end-to-end (clean, +warnings-only, broken, failed), `--json` envelope shape via +`internal/schematest`, exit code 0 for clean/warnings, non-zero for +broken/failed, `roots` populated correctly for a batch spanning one and for a +batch spanning two `markfluence.yaml` roots, `ValidArgsFunction` wired +(`TestSubcommandsCompleteArgs`), confirmation that `run()` never constructs a +`client.ConfluenceClient`, and `--show-html`: `debug` populated (`html` plus +`attachments`) for a clean/warnings/broken file, `null` on a `failed` file, +`null` when the flag is omitted; the human-output indenter on a case with +nested tags (e.g. the `table-cell-colors` regression fixture) matches +expected depth and never alters text content. diff --git a/cmd/check/check.go b/cmd/check/check.go new file mode 100644 index 0000000..1c8e4ef --- /dev/null +++ b/cmd/check/check.go @@ -0,0 +1,164 @@ +// Package check implements the `markfluence check` command: validate one or +// more markdown files against the converter and frontmatter rules with no +// network access and no credentials. It writes nothing -- not to Confluence, +// not to disk. +package check + +import ( + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/mozilla/markfluence/internal/buildinfo" + "github.com/mozilla/markfluence/internal/completion" + "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" +) + +// checkBaseURL and checkSpaceKey are the regression suite's own defaults, +// used unconditionally rather than exposed as flags. Both are used only to +// build the *text* of a rewritten doc-link href (internal/convert/links.go); +// nothing in ConfluencePage.Broken/Warnings reads either, since resolution +// runs off the link index, not these strings. Hardcoding them makes check +// byte-identical across machines, which is what a CI gate wants. +const ( + checkBaseURL = "https://wiki.example.net" + checkSpaceKey = "ENG" +) + +var showHTML bool + +// Cmd is the check command. +var Cmd = &cobra.Command{ + Use: "check FILE...", + Short: "Validate markdown files against the converter and frontmatter rules, offline", + Long: "Validate one or more markdown FILEs against the converter and frontmatter\n" + + "rules, with no network access and no credentials -- fast, safe, and\n" + + "CI/agent-friendly. Reports conversion warnings and broken image/link\n" + + "references, and frontmatter sanity (parseable, page_width valid, page_id\n" + + "numeric when present). Each file is processed independently; the command\n" + + "exits non-zero if any file is broken or failed outright. Warnings alone do\n" + + "not fail.\n\n" + + "\"link not resolved: TARGET\" means TARGET is a sibling .md file that exists\n" + + "under the documentation root but has no page_id yet -- the normal state of\n" + + "a tree that hasn't been published, not a defect. A same-page anchor\n" + + "(#heading) hits the same warning when the current file itself has no\n" + + "page_id yet, which can read as though the file names itself as an\n" + + "unresolved target -- it doesn't; that's just this file, before its first\n" + + "publish.", + Args: cobra.MinimumNArgs(1), + ValidArgsFunction: completion.MarkdownFiles, + RunE: run, +} + +func init() { + Cmd.Flags().BoolVar(&showHTML, "show-html", false, + "Also print the converted storage HTML and attachment list, for debugging.") +} + +func run(cmd *cobra.Command, args []string) error { + rootOverride, _ := cmd.Flags().GetString("root") + roots := project.NewCache(rootOverride) + defer roots.Close() + indexes := linkindex.NewCache() + + failures := 0 + results := make([]*checkResult, 0, len(args)) + for _, filename := range args { + r := processFile(filename, roots, indexes) + results = append(results, r) + if !ui.IsJSON() { + r.renderHuman() + } + if !r.ok() { + failures++ + } + } + for _, dir := range roots.Roots() { + ui.Info("root: " + dir) + } + + if ui.IsJSON() { + items := make([]any, len(results)) + for i, r := range results { + items[i] = r.jsonResult() + } + env := jsonout.NewEnvelope("check", items, summarize(results)) + env.Roots = roots.Roots() + if err := jsonout.Emit(os.Stdout, env); err != nil { + return err + } + if failures > 0 { + return ui.SilentExit(1) + } + return nil + } + + if failures > 0 { + ui.Error(fmt.Sprintf("%d of %d file(s) failed.", failures, len(args))) + return ui.ErrSilent + } + return nil +} + +// processFile validates one file and returns a result describing the +// outcome. It performs no output itself; the caller renders the result. It +// never writes to Confluence or to disk, and never constructs a +// client.ConfluenceClient. +func processFile(filename string, roots *project.Cache, indexes *linkindex.Cache) *checkResult { + r := &checkResult{file: filename} + mf, err := frontmatter.ParseFile(filename) + if err != nil { + return r.fail(err, jsonout.CodeValidation) + } + + if _, err := pagewidth.Declared(mf.Frontmatter); err != nil { + return r.fail(err, jsonout.CodeValidation) + } + if pageID := mf.PageID(); pageID != "" && !pageref.IsDigits(pageID) { + return r.fail(errors.New(pageref.NotNumericMessage(pageID)), jsonout.CodeValidation) + } + + 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) + } + + page, err := convert.MdToConfluence(mf, root, index, checkBaseURL, checkSpaceKey, buildinfo.Stamp()) + if err != nil { + return r.fail(err, jsonout.CodeConvert) + } + r.broken = page.Broken + r.warnings = page.Warnings + if showHTML { + r.debugHTML = page.HTML + r.debugAttachments = page.Attachments + r.hasDebug = true + } + + switch { + case len(r.broken) > 0: + r.status = statusBroken + case len(r.warnings) > 0: + r.status = statusWarnings + default: + r.status = statusClean + } + return r +} diff --git a/cmd/check/check_test.go b/cmd/check/check_test.go new file mode 100644 index 0000000..f172a2f --- /dev/null +++ b/cmd/check/check_test.go @@ -0,0 +1,264 @@ +package check + +import ( + "encoding/json" + "go/parser" + "go/token" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/ui" + "github.com/spf13/cobra" +) + +// write creates path (and its parent directories) with body. +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) + } +} + +// testCmd builds a bare *cobra.Command carrying the one flag run() reads +// itself (--root); --show-html is a package-level var, toggled directly by +// tests that need it, the same way other commands' dry-run-style flags are. +func testCmd(t *testing.T, root string) *cobra.Command { + t.Helper() + c := &cobra.Command{} + c.Flags().String("root", root, "") + return c +} + +// captureOutput runs fn with both os.Stdout and os.Stderr redirected into one +// buffer, returning what it printed. check's human output splits Warn/Error +// (stderr) from Info/show-html (stdout), so an end-to-end test needs both. +func captureOutput(t *testing.T, fn func() error) (string, error) { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + oldOut, oldErr := os.Stdout, os.Stderr + os.Stdout, os.Stderr = w, w + runErr := fn() + os.Stdout, os.Stderr = oldOut, oldErr + if err := w.Close(); err != nil { + t.Fatal(err) + } + out, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + return string(out), runErr +} + +func TestRunClean(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "clean.md"), "# Clean\n\nNothing wrong here.\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "clean.md")}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "clean") { + t.Errorf("output = %q, want a clean line", out) + } +} + +func TestRunWarnings(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "draft.md"), "# Draft\n\nNo page_id yet.\n") + write(t, filepath.Join(dir, "main.md"), "# Main\n\n[the draft](draft.md)\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if err != nil { + t.Fatalf("run: %v (warnings alone must not fail)", err) + } + if !strings.Contains(out, "link not resolved") { + t.Errorf("output = %q, want the unresolved-link warning", out) + } +} + +func TestRunBroken(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "main.md"), "# Main\n\n![missing](nope.png)\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error for a broken file", err) + } + if !strings.Contains(out, "IMAGE BROKEN") { + t.Errorf("output = %q, want the broken-image message", out) + } +} + +func TestRunFailed(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "bad.md"), "---\npage_width: huge\n---\n# Bad\n") + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "bad.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error for a failed file", err) + } + if !strings.Contains(out, "page_width") { + t.Errorf("output = %q, want the page_width error", out) + } +} + +func TestRunUnterminatedFrontmatterIsFailed(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "bad.md"), "---\ntitle: T\nno closing delimiter\n") + + _, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "bad.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error for unterminated frontmatter", err) + } +} + +func TestRunNonNumericPageIDIsFailed(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "bad.md"), "---\npage_id: not-a-number\n---\n# Bad\n") + + _, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "bad.md")}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run = %v, want a silent exit-1 error for a non-numeric page_id", err) + } +} + +func TestRunExitsCleanlyWhenEverythingPasses(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "a.md"), "# A\n") + write(t, filepath.Join(dir, "b.md"), "# B\n") + + _, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "a.md"), filepath.Join(dir, "b.md")}) + }) + if err != nil { + t.Fatalf("run: %v, want nil when every file passes", err) + } +} + +func TestRunReportsOneRootPerBatch(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "markfluence.yaml"), "") + write(t, filepath.Join(dir, "a.md"), "# A\n") + write(t, filepath.Join(dir, "sub", "b.md"), "# B\n") + + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(dir, "a.md"), filepath.Join(dir, "sub", "b.md")}) + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if strings.Count(out, "root: "+dir) != 1 { + t.Errorf("output = %q, want exactly one root line for %s", out, dir) + } +} + +func TestRunReportsMultipleRootsInJSON(t *testing.T) { + one := t.TempDir() + two := t.TempDir() + write(t, filepath.Join(one, "markfluence.yaml"), "") + write(t, filepath.Join(one, "a.md"), "# A\n") + write(t, filepath.Join(two, "markfluence.yaml"), "") + write(t, filepath.Join(two, "b.md"), "# B\n") + + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + out, err := captureOutput(t, func() error { + return run(testCmd(t, ""), []string{filepath.Join(one, "a.md"), filepath.Join(two, "b.md")}) + }) + if err != nil { + t.Fatalf("run: %v", err) + } + var env struct { + Command string `json:"command"` + Roots []string `json:"roots"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if env.Command != "check" { + t.Errorf("command = %q, want check", env.Command) + } + if len(env.Roots) != 2 { + t.Errorf("roots = %v, want both %s and %s", env.Roots, one, two) + } +} + +func TestRunShowHTML(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "main.md"), "# Main\n\nHello.\n") + + showHTML = true + t.Cleanup(func() { showHTML = false }) + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "storage HTML") || !strings.Contains(out, "

Main

") { + t.Errorf("output = %q, want the storage HTML section", out) + } +} + +func TestRunJSONEnvelopeShowHTML(t *testing.T) { + dir := t.TempDir() + write(t, filepath.Join(dir, "main.md"), "# Main\n\nHello.\n") + + showHTML = true + t.Cleanup(func() { showHTML = false }) + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + out, err := captureOutput(t, func() error { return run(testCmd(t, ""), []string{filepath.Join(dir, "main.md")}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + var env struct { + Results []struct { + Debug *struct { + HTML string `json:"html"` + } `json:"debug"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if len(env.Results) != 1 || env.Results[0].Debug == nil || env.Results[0].Debug.HTML == "" { + t.Errorf("envelope = %+v, want a non-null debug.html", env) + } +} + +// TestNeverImportsClient guards the architectural point of the whole command: +// check must stay offline and credential-free. This can't regress silently -- +// importing internal/client would be caught here even before any test that +// exercises behavior would notice. +func TestNeverImportsClient(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, f, nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parsing %s: %v", f, err) + } + for _, imp := range file.Imports { + if strings.Trim(imp.Path.Value, `"`) == "github.com/mozilla/markfluence/internal/client" { + t.Errorf("%s imports internal/client; check must stay offline/credential-free", f) + } + } + } +} diff --git a/cmd/check/json.go b/cmd/check/json.go new file mode 100644 index 0000000..0857ee1 --- /dev/null +++ b/cmd/check/json.go @@ -0,0 +1,211 @@ +package check + +import ( + "fmt" + "regexp" + "strings" + + "github.com/mozilla/markfluence/internal/convert" + "github.com/mozilla/markfluence/internal/jsonout" + "github.com/mozilla/markfluence/internal/ui" +) + +// Per-file status verbs for check. +const ( + statusClean = "clean" + statusWarnings = "warnings" + statusBroken = "broken" + statusFailed = "failed" +) + +// checkResult captures the outcome of validating one file. +type checkResult struct { + file string + status string + broken []string + warnings []string + // hasDebug is true only when --show-html was passed and the file reached + // the converter -- never on a failed file, which has no HTML to show. + hasDebug bool + debugHTML string + debugAttachments []convert.Attachment + errMsg string + code jsonout.Code +} + +// fail marks the result failed with an error and code, and returns it for a +// tidy `return r.fail(...)`. +func (r *checkResult) fail(err error, code jsonout.Code) *checkResult { + r.status = statusFailed + r.errMsg = err.Error() + r.code = code + return r +} + +// ok reports whether the result counts as a success. Computed from status +// rather than stored alongside it, so there is exactly one thing four +// different call sites (fail and the three switch branches in processFile) +// have to get right, not two that could silently disagree -- a broken result +// is ok:false too, but status is what a caller actually branches on. +func (r *checkResult) ok() bool { + return r.status != statusBroken && r.status != statusFailed +} + +// renderHuman prints one file's diagnostics: a [file]-prefixed line per +// broken item (ui.Error) and warning (ui.Warn), a plain "clean" line when +// there's nothing else to report, and -- with --show-html -- the storage +// HTML (indented by nesting depth) and attachment list. +func (r *checkResult) renderHuman() { + prefix := "[" + r.file + "]" + if r.status == statusFailed { + ui.Error(prefix + " " + r.errMsg) + return + } + for _, b := range r.broken { + ui.Error(prefix + " " + b) + } + for _, w := range r.warnings { + ui.Warn(prefix + " " + w) + } + if r.status == statusClean { + ui.Info(prefix + " clean") + } + if r.hasDebug { + ui.Info(prefix + " --- storage HTML ---") + fmt.Println(indentHTML(r.debugHTML)) + if len(r.debugAttachments) > 0 { + ui.Info(prefix + " --- attachments ---") + for _, a := range r.debugAttachments { + fmt.Printf("%s -> %s\n", a.Filename, a.Source) + } + } + } +} + +// htmlTagRE matches one HTML/XML tag, used only to track nesting depth -- +// never to reformat inside a line. Storage HTML is XHTML with every +// attribute value already entity-escaped (html.EscapeString escapes ">"), +// so an unescaped ">" inside a quoted attribute value never reaches here. +var htmlTagRE = regexp.MustCompile(`<[^>]+>`) + +// indentHTML indents storage HTML by nesting depth for human-readable +// display. The renderer already emits one tag per line at every structural +// boundary (confirmed against the regression goldens); this only adds +// leading whitespace per line based on the net depth change its tags cause, +// never reformatting within a line -- so it cannot alter meaningful inline +// text mixed into a block the way a whitespace-normalizing pretty-printer +// could. +func indentHTML(html string) string { + lines := strings.Split(strings.TrimRight(html, "\n"), "\n") + var b strings.Builder + depth := 0 + for i, line := range lines { + trimmed := strings.TrimSpace(line) + d := depth + if strings.HasPrefix(trimmed, " 0 { + d-- + } + b.WriteString(strings.Repeat(" ", d)) + b.WriteString(trimmed) + if i < len(lines)-1 { + b.WriteString("\n") + } + depth += tagDepthDelta(trimmed) + if depth < 0 { + depth = 0 + } + } + return b.String() +} + +// tagDepthDelta returns how a line's tags change nesting depth: +1 per +// opening tag, -1 per closing tag, 0 for a self-closing tag (XHTML's own +// "") or an HTML comment, net over every tag found on the line. +func tagDepthDelta(line string) int { + delta := 0 + for _, tag := range htmlTagRE.FindAllString(line, -1) { + switch { + case strings.HasPrefix(tag, "\n

after

\n" + want := "		trailing
\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
ServiceStatus

Notes

authok

steady

billingdown

paging

search

decommissioned

\n

An unknown color name is dropped with a warning, and so is a marker that isn't first in its cell:

\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
CellResult
nopeno background
trailing no background
\n

The keyword and the color are case-insensitive; other comments pass through untouched:

\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
Cell
shouty
shouty hex
text
\n", "warnings": [ - "table cell \"nope\": ignoring bg:chartreuse (unknown color: use a swatch name or a #rrggbb hex)", - "table cell \"trailing\": ignoring bg:green (a bg marker must come first in the cell)" + "line 17: table cell \"nope\": ignoring bg:chartreuse (unknown color: use a swatch name or a #rrggbb hex)", + "line 18: table cell \"trailing\": ignoring bg:green (a bg marker must come first in the cell)" ] } diff --git a/internal/convert/version_test.go b/internal/convert/version_test.go index 19d5c06..5963d6c 100644 --- a/internal/convert/version_test.go +++ b/internal/convert/version_test.go @@ -10,10 +10,13 @@ import ( ) func TestVersionTokenReplaced(t *testing.T) { - md := frontmatter.Parse( + md, err := frontmatter.Parse( filepath.Join(t.TempDir(), "main.md"), "# Title\n\n\n", ) + if err != nil { + t.Fatal(err) + } const stamp = "markfluence v1.2.3 2020-01-01T00:00:00Z" root := testRoot(t, filepath.Dir(md.Filename)) page, err := convert.MdToConfluence(md, root, testIndex(t, root), "https://wiki.example.net", "ENG", stamp) diff --git a/internal/frontmatter/frontmatter.go b/internal/frontmatter/frontmatter.go index 4944718..4f92319 100644 --- a/internal/frontmatter/frontmatter.go +++ b/internal/frontmatter/frontmatter.go @@ -7,6 +7,7 @@ package frontmatter import ( + "errors" "os" "regexp" "sort" @@ -28,7 +29,14 @@ var inlineCommentRE = regexp.MustCompile(`\s#`) // Full-line `#` comments are skipped; a trailing inline `#` comment is stripped // from each unquoted value; quoted values are read via ParseValue. func Extract(content string) (map[string]string, string) { - loc := frontmatterRE.FindStringSubmatchIndex(content) + return extractFrom(frontmatterRE.FindStringSubmatchIndex(content), content) +} + +// extractFrom is Extract's body, taking an already-computed match location so +// Parse can share the one frontmatterRE evaluation it also needs for +// ErrUnterminatedFrontmatter, rather than running the same regex over content +// a second time. +func extractFrom(loc []int, content string) (map[string]string, string) { if loc == nil { return map[string]string{}, content } @@ -232,10 +240,30 @@ type MarkdownFile struct { Body string } -// Parse builds a MarkdownFile from an in-memory content string tagged with filename. -func Parse(filename, content string) *MarkdownFile { - fm, body := Extract(content) - return &MarkdownFile{Filename: filename, Content: content, Frontmatter: fm, Body: body} +// ErrUnterminatedFrontmatter is returned by Parse/ParseFile when content opens +// with a "---\n" delimiter that never closes. Extract is deliberately lenient +// about this shape -- a regex miss just falls back to "no frontmatter, whole +// file is body" -- which would otherwise hide a common paste mistake +// completely, including from every command that calls Parse. +// +// This is a lexical check, not a parse: a document whose very first line is a +// bare thematic break (a markdown horizontal rule, "---" with nothing after it +// that closes with a second "---\n") is indistinguishable from unterminated +// frontmatter and is flagged the same way. Accepted deliberately -- a document +// opening cold with a horizontal rule and no heading is unusual, and detecting +// the difference would need real parsing, not a shape this small. +var ErrUnterminatedFrontmatter = errors.New( + `unterminated frontmatter block: starts with "---" but has no closing "---" line`) + +// Parse builds a MarkdownFile from an in-memory content string tagged with +// filename, or reports ErrUnterminatedFrontmatter. +func Parse(filename, content string) (*MarkdownFile, error) { + loc := frontmatterRE.FindStringSubmatchIndex(content) + if loc == nil && strings.HasPrefix(content, "---\n") { + return nil, ErrUnterminatedFrontmatter + } + fm, body := extractFrom(loc, content) + return &MarkdownFile{Filename: filename, Content: content, Frontmatter: fm, Body: body}, nil } // ParseFile reads filename from disk and parses it. @@ -244,7 +272,7 @@ func ParseFile(filename string) (*MarkdownFile, error) { if err != nil { return nil, err } - return Parse(filename, string(data)), nil + return Parse(filename, string(data)) } // coordinate reads a page-coordinate field, mapping the no-value sentinels to "". diff --git a/internal/frontmatter/frontmatter_test.go b/internal/frontmatter/frontmatter_test.go index f300c7c..f61ae5c 100644 --- a/internal/frontmatter/frontmatter_test.go +++ b/internal/frontmatter/frontmatter_test.go @@ -138,8 +138,11 @@ func TestUpdateFieldPreservesCommentsDropsBlanks(t *testing.T) { // --- MarkdownFile accessors -------------------------------------------------- func TestMarkdownFileAccessors(t *testing.T) { - md := frontmatter.Parse("doc.md", + md, err := frontmatter.Parse("doc.md", "---\ntitle: My Page\npage_id: 123\nspace: ENG\nparent: 456\n---\nbody\n") + if err != nil { + t.Fatal(err) + } if md.Title() != "My Page" || md.PageID() != "123" || md.Space() != "ENG" || md.Parent() != "456" { t.Errorf("accessors = %q/%q/%q/%q", md.Title(), md.PageID(), md.Space(), md.Parent()) } @@ -155,7 +158,10 @@ func TestCoordinateSentinelsAreUnset(t *testing.T) { "---\ntitle: X\npage_id:\n---\nb\n", // blank "---\ntitle: X\npage_id: null\n---\nb\n", // literal null } { - md := frontmatter.Parse("doc.md", doc) + md, err := frontmatter.Parse("doc.md", doc) + if err != nil { + t.Fatal(err) + } if md.PageID() != "" { t.Errorf("PageID() = %q for %q, want empty", md.PageID(), doc) } @@ -163,10 +169,50 @@ func TestCoordinateSentinelsAreUnset(t *testing.T) { } func TestTitleKeepsLiteralNullButBlankIsEmpty(t *testing.T) { - if md := frontmatter.Parse("d.md", "---\ntitle: null\n---\nb\n"); md.Title() != "null" { + md, err := frontmatter.Parse("d.md", "---\ntitle: null\n---\nb\n") + if err != nil { + t.Fatal(err) + } + if md.Title() != "null" { t.Errorf("Title() = %q, want %q (a title is free text)", md.Title(), "null") } - if md := frontmatter.Parse("d.md", "---\ntitle:\n---\nb\n"); md.Title() != "" { + md, err = frontmatter.Parse("d.md", "---\ntitle:\n---\nb\n") + if err != nil { + t.Fatal(err) + } + if md.Title() != "" { t.Errorf("Title() = %q, want empty for blank", md.Title()) } } + +func TestParseUnterminatedFrontmatter(t *testing.T) { + _, err := frontmatter.Parse("d.md", "---\ntitle: T\nno closing delimiter\n") + if err != frontmatter.ErrUnterminatedFrontmatter { + t.Errorf("err = %v, want ErrUnterminatedFrontmatter", err) + } +} + +// TestParseFlagsALeadingThematicBreakToo pins a known, accepted tradeoff +// (see ErrUnterminatedFrontmatter's doc comment): a document opening with a +// bare "---" horizontal rule and nothing that closes it is indistinguishable +// from unterminated frontmatter using this lexical check, and is flagged the +// same way rather than silently read as "no frontmatter". This is not a bug +// to fix here -- it's what the shape of the check can and cannot tell apart. +func TestParseFlagsALeadingThematicBreakToo(t *testing.T) { + _, err := frontmatter.Parse("d.md", "---\n\nSome body with no frontmatter at all.\n") + if err != frontmatter.ErrUnterminatedFrontmatter { + t.Errorf("err = %v, want ErrUnterminatedFrontmatter (a known false positive, not a regression)", err) + } +} + +func TestParseUnterminatedFrontmatterNoFalsePositives(t *testing.T) { + for name, doc := range map[string]string{ + "proper frontmatter": "---\ntitle: T\n---\nbody\n", + "no frontmatter": "just a document\nwith no frontmatter at all\n", + "--- in a fenced code block, not at the top": "intro\n\n```\n---\nnot frontmatter\n---\n```\n", + } { + if _, err := frontmatter.Parse("d.md", doc); err != nil { + t.Errorf("%s: err = %v, want nil", name, err) + } + } +} diff --git a/internal/linkindex/linkindex.go b/internal/linkindex/linkindex.go index dde89e5..93d94b6 100644 --- a/internal/linkindex/linkindex.go +++ b/internal/linkindex/linkindex.go @@ -65,7 +65,13 @@ func Build(root *project.Root) (*Index, error) { if err != nil { return nil } - mf := frontmatter.Parse(path, string(data)) + mf, err := frontmatter.Parse(path, string(data)) + if err != nil { + // A malformed sibling is skipped exactly like an unreadable one -- + // one broken file elsewhere in the tree must not block checking or + // converting an unrelated one. + return nil + } if id := mf.PageID(); id != "" { idx.pages[path] = PageEntry{PageID: id, Title: mf.Title()} } @@ -91,6 +97,18 @@ func (idx *Index) Page(path string) (PageEntry, bool) { return e, ok } +// FileExists reports whether path (root-relative, slash-separated) is a +// walked `.md` file under the index's root, regardless of whether it has a +// page_id yet. It answers "does this file exist at all" independent of +// Page's "is this file published" -- Build records an anchors entry (even an +// empty one) for every walked file, unlike pages, which only gets one when +// the file has a page_id, so this reuses that map rather than adding new +// bookkeeping. +func (idx *Index) FileExists(path string) bool { + _, ok := idx.anchors[path] + return 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) { diff --git a/internal/linkindex/linkindex_test.go b/internal/linkindex/linkindex_test.go index c5996f8..a486675 100644 --- a/internal/linkindex/linkindex_test.go +++ b/internal/linkindex/linkindex_test.go @@ -88,6 +88,30 @@ func TestBuildSkipsPageWithNoPageID(t *testing.T) { } } +// TestFileExistsIndependentOfPageID is FileExists's whole point: "exists" and +// "published" are genuinely distinct questions, and a page_id-less file must +// answer true to the first while still answering false to Page. +func TestFileExistsIndependentOfPageID(t *testing.T) { + root := t.TempDir() + write(t, filepath.Join(root, "published.md"), "---\npage_id: 1\n---\nbody\n") + 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 !idx.FileExists("published.md") { + t.Error(`FileExists("published.md") = false, want true`) + } + if !idx.FileExists("draft.md") { + t.Error(`FileExists("draft.md") = false, want true (a page_id-less file still exists)`) + } + if idx.FileExists("nope.md") { + t.Error(`FileExists("nope.md") = true, want false`) + } +} + // 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 diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index b46080a..1e5e63b 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -9,7 +9,7 @@ "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"] }, + "command": { "enum": ["info", "read", "update", "create", "fix", "check", "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" } @@ -60,6 +60,15 @@ } } }, + { + "if": { "properties": { "command": { "const": "check" } }, "required": ["command"] }, + "then": { + "properties": { + "results": { "items": { "$ref": "#/$defs/checkResult" } }, + "summary": { "$ref": "#/$defs/checkSummary" } + } + } + }, { "if": { "properties": { "command": { "const": "attachment-upload" } }, "required": ["command"] }, "then": { @@ -423,6 +432,46 @@ "code": { "$ref": "#/$defs/codeOrNull" } } }, + "checkResult": { + "description": "One checked file. broken/warnings are always [] (never null), matching ConfluencePage's own convention. status=broken means broken is non-empty (frontmatter or converter); status=failed means the file never reached a clean answer at all (unreadable, unterminated frontmatter, bad page_width, non-numeric page_id) -- code is VALIDATION in that case. debug is non-null only when --show-html was passed and the file reached the converter (never on a failed file).", + "type": "object", + "additionalProperties": false, + "required": ["ok", "status", "file", "broken", "warnings", "debug", "error", "code"], + "properties": { + "ok": { "type": "boolean" }, + "status": { "enum": ["clean", "warnings", "broken", "failed"] }, + "file": { "type": "string" }, + "broken": { "type": "array", "items": { "type": "string" } }, + "warnings": { "type": "array", "items": { "type": "string" } }, + "debug": { + "oneOf": [ + { "type": "null" }, + { + "type": "object", + "additionalProperties": false, + "required": ["html", "attachments"], + "properties": { + "html": { "type": "string" }, + "attachments": { "type": "array", "items": { "$ref": "#/$defs/checkAttachment" } } + } + } + ] + }, + "error": { "$ref": "#/$defs/stringOrNull" }, + "code": { "$ref": "#/$defs/codeOrNull" } + } + }, + "checkAttachment": { + "description": "ConfluencePage.Attachments verbatim: a local image check's --show-html surfaces, not an upload outcome (contrast update/create's attachments array, which reports an action).", + "type": "object", + "additionalProperties": false, + "required": ["filename", "path", "source"], + "properties": { + "filename": { "type": "string" }, + "path": { "type": "string" }, + "source": { "type": "string" } + } + }, "attachmentUploadResult": { "description": "One uploaded file. status uses the same verbs as the attachments array on update/create. dest_path is always null: upload has no local destination to report, and only exists here so upload and download share one result shape.", "type": "object", @@ -566,6 +615,19 @@ "consistent": { "type": "integer" } } }, + "checkSummary": { + "description": "clean/warnings count files on the ok:true side (clean has neither broken nor warnings; warnings has only warnings); failed already covers both the broken and failed statuses on the ok:false side, the same granularity fixSummary uses.", + "type": "object", + "additionalProperties": false, + "required": ["total", "succeeded", "failed", "clean", "warnings"], + "properties": { + "total": { "type": "integer" }, + "succeeded": { "type": "integer" }, + "failed": { "type": "integer" }, + "clean": { "type": "integer" }, + "warnings": { "type": "integer" } + } + }, "errorObject": { "description": "The typed error object written to stderr on a fatal/pre-flight failure. command may be empty for a pre-parse (bad-flag) error.", "type": "object",