From dae36d84f3592b85ebdb8095b09ebad78dc69497 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 11:16:29 -0400 Subject: [PATCH 01/14] docs(plans): plan the check command --- _plans/027_check-subcommand.md | 475 +++++++++++++++++++++++++++++++++ 1 file changed, 475 insertions(+) create mode 100644 _plans/027_check-subcommand.md diff --git a/_plans/027_check-subcommand.md b/_plans/027_check-subcommand.md new file mode 100644 index 0000000..725671e --- /dev/null +++ b/_plans/027_check-subcommand.md @@ -0,0 +1,475 @@ +# Plan: the `check` command + +Add an offline validator for markdown files. Closes #42. + +## Current state of the codebase + +#42 asks for a `check` subcommand that runs the converter over one or more +markdown FILEs and reports problems without requiring network access or +credentials. That is not the thin wrapper the issue first assumed. + +Current state: + +- `internal/convert`'s whole diagnostic surface today is three `Broken` + messages (unsupported image extension, image outside root, image + not-found/not-regular), three `Warnings` (two image-property, one table), + and exactly one link diagnostic — `links.go:94`, + `"link not resolved: %s"` — fired whenever a sibling `.md` link doesn't + resolve in the index. Nothing else in `links.go` touches + `r.broken`/`r.warnings`. +- That single link warning doesn't distinguish *why* a link failed: a target + that's missing entirely, one that resolves outside the documentation root, + and one that exists but has no `page_id` yet all produce the identical + message. A `#fragment` that matches no heading produces no diagnostic at + all — both anchor branches in `rewriteHref` silently no-op on a miss. +- `MdToConfluence`'s signature is `(md *frontmatter.MarkdownFile, root + *project.Root, index *linkindex.Index, baseURL, spaceKey, version string)`. + `root` comes from `internal/project` (walks up from a file looking for + `markfluence.yaml`; falling back to the starting directory when none is + found is not an error). `index` comes from `internal/linkindex.Build`, + which walks the whole tree under `root` once, collecting every `.md` + file's `page_id`/title (when present) and heading anchors. +- Both `root` and `index` are filesystem-only — `internal/linkindex` never + imports `internal/client` — so nothing in the conversion path requires + network access or credentials. +- `update`/`create` already build this `root`/`index` pair once per batch via + `internal/project.Cache`/`internal/linkindex.Cache`, resolved per file and + shared across every file under the same root; `check` needs the identical + plumbing just to call `MdToConfluence` at all. +- `idx.anchors[path]` is populated (even to an empty map) for every walked + `.md` file regardless of whether it has a `page_id` — enough to answer + "does this path exist under root" almost for free. +- `resolveDocKey`/`rootRelativeKey` (`convert.go`) never reject a + `../`-prefixed result: an escaping link currently lands in "not found" + only because the index can't contain anything outside root by + construction, not because anything explicitly checks for it. +- `docs/guarantees.md`'s R1 (`report-unresolved-references`) already reads + **Partial**, not **Aspirational** — the existing link warning is why. +- `root.go`'s `PersistentPreRunE` doesn't construct an + `internal/client.ConfluenceClient`; every existing command's + `client.Resolve` call happens inside its own `run()`, so nothing upstream + forces a client into existence. + +Per interview: the diagnostic gaps above are close enough to #42's own goal +that this plan folds fixing them in now rather than shipping `check` narrowly +and revisiting the diagnostic surface a second time later. It's still +sequenced as separate, atomic commits within one PR/branch — converter +changes first, then the command that surfaces them. + +## Decisions locked + +### Command: `check FILE...`, required, exactly like `fix` + +`cobra.MinimumNArgs(1)` + `ValidArgsFunction: completion.MarkdownFiles`. No +bare `markfluence check` project-wide scan — no other command does that, the +issue's own wording is "one or more markdown FILEs", and the stated use case +(a pre-commit/CI hook) passes its own file list (e.g. `git diff +--name-only`) rather than wanting markfluence to walk anything. + +### No `client.Resolve`, no HTTP, no file writes, ever + +`check`'s `run()` never imports `internal/client`. `root.go`'s +`PersistentPreRunE` doesn't construct a client either, so nothing upstream +forces one into existence — `check` is simply the first command whose `run()` +doesn't call `client.Resolve`. It is read-only on disk too: no frontmatter +write-back, unlike `fix`/`create`. + +### `project.Cache` / `linkindex.Cache`, exactly like `create`/`update` + +```go +rootOverride, _ := cmd.Flags().GetString("root") +roots := project.NewCache(rootOverride) +defer roots.Close() +indexes := linkindex.NewCache() +``` + +Per file: `root, err := roots.Resolve(filepath.Dir(filename))`, then `index, +err := indexes.Get(root)`. The envelope's top-level `roots` field is +populated from `roots.Roots()`, matching `update`. + +### `baseURL`/`spaceKey` hardcoded, no flags + +`https://wiki.example.net` / `ENG` — the regression suite's own defaults — +passed straight to `MdToConfluence`. Both are used only to build the *text* +of a rewritten href; nothing in `Broken`/`Warnings` reads either, since +resolution runs off `index`. Hardcoding makes `check` byte-identical across +machines, which is what a CI gate wants, and drops two flags with no effect +on what's reported. + +### Link severity: four buckets, and a broken link now rewrites the output + +Per interview, a broken link is treated the same way `images.go` already +treats a broken image: `Broken` doesn't just add a diagnostic string, it +**replaces the published element** with literal text (`LINK BROKEN: ...`), +not just the plain href passthrough that happens today. This is a real +behavior change to what `update`/`create` publish, not merely a `check`-only +diagnostic; it also affects any already-published page containing a link to +a genuinely missing/escaping target. + +| case | severity | message (line-number prefix per below, omitted here for brevity) | output change | +|---|---|---|---| +| target `.md` not found anywhere under root | **Broken** | `LINK BROKEN: %s (not found)` | `` + text replaced with literal message | +| target `.md` resolves outside the documentation root | **Broken** | `LINK BROKEN: %s (outside the documentation root)` | same | +| target `.md` exists, has no `page_id` yet | Warning (unchanged) | `link not resolved: %s` | none — href renders as-is, same as today | +| target has a `page_id`, but `#fragment` matches no heading | Warning (new) | e.g. `anchor not found: %s` | none | + +A fragment-miss warning only fires when the target file itself exists — if +the whole target is missing/escaping, that's reported once as `LINK BROKEN`, +not doubled up with a redundant anchor warning. Applies to both anchor +branches in `rewriteHref` (same-page `#frag` and cross-file `path.md#frag`). + +**Renderer restructuring** (`links.go`/the `ast.Link` node renderer): today +`renderLink` writes `` on `entering` and `` on `!entering` +as two independent writes, with child text nodes rendered by goldmark's +walker in between. Replacing the whole element means `entering` must detect +"broken", write the literal message, and return `ast.WalkSkipChildren` — but +goldmark still invokes the renderer a second time on `!entering` regardless +of that skip (`WalkSkipChildren` only suppresses descending into children, +not the node's own second visit). Needs a small per-node flag on +`storageRenderer` (same pattern as `r.seen` for image dedup) to suppress the +stray `` on the matching leave call. + +**New plumbing needed:** + +1. **`Index.FileExists(path) bool`.** No `Build` changes needed — + `idx.anchors[path]` is already populated (even to an empty map) for every + walked `.md` file regardless of `page_id`, so existence is + `_, ok := idx.anchors[path]`. +2. **An explicit escape check on the query side.** `resolveDocKey` → + `rootRelativeKey` (`convert.go:135`) returns whatever `filepath.Rel` + produces with `ok=true` unconditionally; it never rejects a `../`-prefixed + result the way `images.go`'s `rootRelative` does for image paths. Today an + escaping link happens to land in "not found" only because the index can + never contain anything outside root by construction (`DocKeyFor`'s own doc + comment, and `linkindex`'s package doc, both already explain why *that* + side needs no clamp) — but the query side still needs a lexical check + (`r == ".."` or `strings.HasPrefix(r, "../")`) to tell "outside root" apart + from "not found" for the two distinct Broken messages. +3. **A fragment-miss warning.** Both anchor branches in `rewriteHref` + currently no-op silently on a miss; add it there, gated on `FileExists`. + +Because this is a converter change, `update`/`create` inherit these +diagnostics (and the output change) for free via the shared +`ConfluencePage.Broken`/`Warnings` fields — no command-specific plumbing +needed there beyond confirming the existing pass-through still works. + +### Every Broken/Warning message gains a source line number + +`"LINK BROKEN: ../outside.md (outside the documentation root)"` doesn't say +*where* in the document that link is — a real gap for anyone acting on +`check`'s output, not just a nicety. Originally scoped out of this plan as +"bigger than this issue" on the assumption that it needed AST rewiring; it +doesn't, for the common case: + +- The `NodeRenderer` interface already passes the raw source bytes to every + renderer function (`renderImage` already uses this for alt text via + `nodeText`; `tableCellBGTransformer.warn` (`tables.go:154`) already has + both `cell ast.Node` and `source []byte` in scope; only `renderLink` + currently discards it — the parameter is literally named `_`). +- Neither `ast.Link` nor `ast.Image` carries its own position (goldmark's + parser never calls `SetLines` on either), but their child `*ast.Text` + nodes do, via `Segment.Start` — an already-established pattern (`nodeText` + in `images.go:188` walks exactly these segments today). A byte offset + converts to a 1-indexed line by counting newlines in `source[:offset]`. +- A new shared helper, `nodeLine(n ast.Node, source []byte) (int, bool)` + (next to `nodeText` in `images.go`), walks to the first descendant + `*ast.Text` and returns its line; `ok=false` when a node has no text + descendant at all (e.g. an empty link), in which case the message stays + unprefixed rather than showing a wrong line. + +**Format**: prefix the existing message text, `"line %d: "` — e.g. `"line +12: LINK BROKEN: typo-target.md (not found)"` — rather than changing +`Broken`/`Warnings` to structured entries. This is deliberately the light +version: a fully structured `{line, column, message}` entry (what an LSP +integration would eventually want for a `Range`) stays out of scope, per +below. + +**Call sites, and the one real threading cost**: `images.go`'s four +`Broken` and two `Warnings` sites, and `tables.go`'s `warn`, already have a +node and `source` in scope — one-line additions each. `links.go` needs real +threading: `renderLink` must stop discarding `source`, and `rewriteHref`/ +`rewriteDocLink` (currently `href string` only) need the line/`ok` pair +threaded through to their warning-append sites (the not-resolved case and +both anchor-miss branches). Contained to `links.go`'s existing private +functions — no AST changes, no changes outside `internal/convert`. + +**Ripple**: this changes the text of every *existing* `Broken`/`Warning` +message, not just the new link ones — every regression fixture with an +image warning/broken case needs its golden `test.output` regenerated +(`make regen-regressions`), and the README's example messages need +updating to match. + +Frontmatter-sourced diagnostics (`page_width`, `page_id`, the unterminated +block) do **not** get a line number here — they're not goldmark-AST-based +at all, and unlike a link buried in a long body, a bad frontmatter key is +already trivially locatable (a handful of lines at the top of the file). + +### Frontmatter validation: three checks, one of them shared across every command + +- **`page_width`**: reuse `pagewidth.Declared(frontmatter)` verbatim. +- **`page_id`**: reuse `pageref.IsDigits` — flag only a *present* non-numeric + value, never require one. +- **Unterminated frontmatter block**: new, and per interview wired into + `internal/frontmatter.Parse` itself rather than kept `check`-only, since + `update`/`create`/`fix` all hit this exact silent misread today (a file + starting with `---\n` that never closes it isn't an error to `Extract` — it + falls back to "no frontmatter, whole file is body", `frontmatter.go:30-50`, + which for `create` surfaces as a confusing downstream "missing title" error + instead of "your frontmatter is malformed"). `Parse`'s signature changes to + `(*MarkdownFile, error)`; `ParseFile` propagates it. Six call sites need + updating: `cmd/update`, `cmd/fix`, `cmd/create` (×2 — the file itself and + the parent `.md` lookup), `internal/pageref`, `internal/linkindex.Build`. + `Build` walks every sibling in the tree, not just the file under test — on + this new error it skips that file's entry exactly the way it already skips + an unreadable one today (silently, via `return nil` from the walk + callback), so one malformed file elsewhere never blocks checking or + converting an unrelated one. +- **Explicitly not checked**: "required fields for the intended operation" + (`page_id`/`space`/`parent` presence) — dropped per the issue's own + comment, because `check` cannot know intent and a false positive here is + worse than a miss. + +### `--json`: `checkResult` and `checkSummary` + +Modeled on `fixResult`/`fixSummary` (`cmd/fix/json.go`), plus a `broken` +field `fix` has no analog for (fix never fails on content, only on I/O/API +errors): + +``` +checkResult: { + ok: bool + status: "clean" | "warnings" | "broken" | "failed" + file: string + broken: []string // always present, [] not null + warnings: []string // always present, [] not null + debug: { // null unless --show-html, or on a failed file + html: string // the converted storage HTML, unindented/compact + attachments: [{filename: string, path: string, source: string}] // ConfluencePage.Attachments verbatim + } | null + error: string | null + code: Code | null +} +``` + +- `clean`: no broken, no warnings. `ok: true`. +- `warnings`: warnings only. `ok: true` — warnings don't fail. +- `broken`: `broken` non-empty (frontmatter or converter). `ok: false`, + contributes to the batch's non-zero exit. +- `failed`: the file never got a clean answer at all (unreadable, + unterminated frontmatter block, bad `page_width`, non-numeric `page_id`). + `ok: false`, `code: VALIDATION`. + +`checkSummary` mirrors `fixSummary`'s shape: `{ total, succeeded, failed, +clean, warnings }`. + +Schema wiring (`cmd`'s `TestCommandEnumMatchesRegisteredCommands` is +bidirectional): add `"check"` to `schema/json-output/v1.json`'s `command` +enum, an `if/then` branch pinning `checkResult`/`checkSummary`, and the two +`$defs`. `check` is **not** added to `noJSONEnvelope` — it emits a real +envelope, unlike `schema`. + +### `--show-html` + +There is no existing command that prints the converted storage-format HTML +for inspection: `ConfluencePage.HTML` is only ever consumed internally by +`update`/`create` to publish it, never written to stdout or a file. `check` +is a natural place for a debugging escape hatch, since it already runs +`MdToConfluence` and holds the result — and `ConfluencePage.Attachments` +(the local-image → upload-name mapping) is just as relevant to debugging a +conversion as the HTML body is, so `--show-html` surfaces both, not just the +body. + +`--show-html` prints, for a file that reached the converter (not `failed`), +in addition to its diagnostic lines (not instead of them — the point is +seeing "what's wrong" and "what would actually publish" together): + +- the storage HTML, **indented by nesting depth** in human mode. The + renderer already emits one tag per line at every structural boundary + (confirmed against `testdata/regression/table-cell-colors/test.output` — + `\n\n\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. From cfc0058772237d40ddae73b97cb3585e70dcfde3 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 11:21:01 -0400 Subject: [PATCH 02/14] feat(frontmatter): detect an unterminated frontmatter block Extract's leniency for a "---" that never closes -- falling back to "no frontmatter, whole file is body" -- hid a common paste mistake completely, including from every command that calls Parse (create surfaced it as a confusing "missing title" error instead). Parse now reports ErrUnterminatedFrontmatter; linkindex.Build skips the offending file the same way it already skips an unreadable one, so one malformed sibling never blocks checking or converting an unrelated file. --- cmd/create/create.go | 5 ++- cmd/create/create_test.go | 10 ++++-- cmd/update/update_test.go | 25 ++++++++++++--- internal/convert/storage_to_md_test.go | 30 ++++++++++++++---- internal/convert/symlink_test.go | 10 ++++-- internal/convert/version_test.go | 5 ++- internal/frontmatter/frontmatter.go | 21 ++++++++++--- internal/frontmatter/frontmatter_test.go | 40 +++++++++++++++++++++--- internal/linkindex/linkindex.go | 8 ++++- 9 files changed, 128 insertions(+), 26 deletions(-) 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/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/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/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..427f73a 100644 --- a/internal/frontmatter/frontmatter.go +++ b/internal/frontmatter/frontmatter.go @@ -7,6 +7,7 @@ package frontmatter import ( + "errors" "os" "regexp" "sort" @@ -232,10 +233,22 @@ type MarkdownFile struct { Body string } -// Parse builds a MarkdownFile from an in-memory content string tagged with filename. -func Parse(filename, content string) *MarkdownFile { +// 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. +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) { + if strings.HasPrefix(content, "---\n") && !frontmatterRE.MatchString(content) { + return nil, ErrUnterminatedFrontmatter + } fm, body := Extract(content) - return &MarkdownFile{Filename: filename, Content: content, Frontmatter: fm, Body: body} + return &MarkdownFile{Filename: filename, Content: content, Frontmatter: fm, Body: body}, nil } // ParseFile reads filename from disk and parses it. @@ -244,7 +257,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..235d07c 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,36 @@ 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) { + if _, err := frontmatter.Parse("d.md", "---\ntitle: T\nno closing delimiter\n"); err != frontmatter.ErrUnterminatedFrontmatter { + t.Errorf("err = %v, want ErrUnterminatedFrontmatter", 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..1141c87 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()} } From 6f3aafb659be04020072bc0175f7186b687dd346 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 11:21:55 -0400 Subject: [PATCH 03/14] feat(linkindex): track file existence independent of page_id Index.FileExists answers "does this file exist under root" without regard to whether it has a page_id yet -- reusing the anchors map, which Build already populates (even to an empty map) for every walked .md file, unlike pages, which only gets an entry once a file is published. This is what lets convert's upcoming link-severity split tell "missing entirely" apart from "exists but not published yet". --- internal/linkindex/linkindex.go | 12 ++++++++++++ internal/linkindex/linkindex_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/internal/linkindex/linkindex.go b/internal/linkindex/linkindex.go index 1141c87..93d94b6 100644 --- a/internal/linkindex/linkindex.go +++ b/internal/linkindex/linkindex.go @@ -97,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 From 127e334f4bfa1bf925c5afe6d715665cc7661f7a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 11:26:54 -0400 Subject: [PATCH 04/14] fix(frontmatter): satisfy lll on the unterminated-frontmatter test --- internal/frontmatter/frontmatter_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/frontmatter/frontmatter_test.go b/internal/frontmatter/frontmatter_test.go index 235d07c..0c1486c 100644 --- a/internal/frontmatter/frontmatter_test.go +++ b/internal/frontmatter/frontmatter_test.go @@ -186,7 +186,8 @@ func TestTitleKeepsLiteralNullButBlankIsEmpty(t *testing.T) { } func TestParseUnterminatedFrontmatter(t *testing.T) { - if _, err := frontmatter.Parse("d.md", "---\ntitle: T\nno closing delimiter\n"); err != frontmatter.ErrUnterminatedFrontmatter { + _, err := frontmatter.Parse("d.md", "---\ntitle: T\nno closing delimiter\n") + if err != frontmatter.ErrUnterminatedFrontmatter { t.Errorf("err = %v, want ErrUnterminatedFrontmatter", err) } } From 5de566f332acafcf9fc178941dc8c9fcfe2bbd7e Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 11:27:07 -0400 Subject: [PATCH 05/14] feat(convert): reject an escaping link the way images.go already does resolveDocKey now reports whether a link/anchor destination's resolved path escapes the documentation root -- a lexical check, since the index build side needs none (an escaping key can never be in it) but the query side had no way to tell that guaranteed miss apart from a genuine "not found". A link that escapes root is now Broken: rewriteDocLink reports "LINK BROKEN: ... (outside the documentation root)" and renderLink replaces the whole element -- tags and visible text alike -- with that literal message, the same way images.go already does for a missing image. This needed a small per-node flag on storageRenderer (linkBrokenText) since goldmark still invokes a container node's renderer on the matching leaving call regardless of WalkSkipChildren on entering, and there is no "" to write in that case. --- internal/convert/links.go | 107 +++++++++++------- internal/convert/links_test.go | 50 ++++---- internal/convert/renderer.go | 8 ++ .../regression/link-outside-root/docs/link.md | 11 +- .../regression/link-outside-root/test.output | 10 +- 5 files changed, 116 insertions(+), 70 deletions(-) diff --git a/internal/convert/links.go b/internal/convert/links.go index c2651fe..aa6fedb 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, ) (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)) + 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 { + key, escapes := r.resolveDocKey(path) + entry, found := r.index.Page(key) + if !found { + if escapes { + msg := fmt.Sprintf("LINK BROKEN: %s (outside the documentation root)", href) + r.broken = append(r.broken, msg) + return "", false, msg + } r.warnings = append(r.warnings, fmt.Sprintf("link not resolved: %s", href)) - return "", false + 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, "" } // 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/renderer.go b/internal/convert/renderer.go index e415323..3c2a23a 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -47,6 +47,14 @@ 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 } // RegisterFuncs registers the node handlers this renderer overrides. 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..961ccc7 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": [ + "LINK BROKEN: ../outside/linked.md (outside the documentation root)" + ], + "html": "

Escaping Link

\n

A 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": [] } From 64ad1de73b4b67d3aafb5c3ee73324b45afb3b03 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 11:44:30 -0400 Subject: [PATCH 06/14] feat(convert): report a missing link target as Broken, not a warning A doc link whose target genuinely doesn't exist under root now reports "LINK BROKEN: ... (not found)" and replaces the published element the same way an escaping or missing image already does, rather than the generic "link not resolved" warning every unresolved link used to get regardless of why. FileExists is what makes this distinguishable from "exists but has no page_id yet", which stays a warning -- that's the normal state of every page in a tree that hasn't been published, not a defect. Updated the two existing regression fixtures whose "unknown" link pointed at a genuinely nonexistent file, and added a new one (link-not-yet-published) so the still-a-warning path -- an existing, unpublished sibling -- has coverage of its own; nothing previously exercised it directly. --- internal/convert/links.go | 30 ++++++++++++------- .../regression/doc-links-encoded/main.md | 5 ++-- .../regression/doc-links-encoded/test.output | 10 +++---- .../regression/internal-doc-links/main.md | 2 +- .../regression/internal-doc-links/test.output | 10 +++---- .../link-not-yet-published/draft.md | 3 ++ .../regression/link-not-yet-published/main.md | 8 +++++ .../link-not-yet-published/test.input | 3 ++ .../link-not-yet-published/test.output | 8 +++++ 9 files changed, 56 insertions(+), 23 deletions(-) create mode 100644 internal/convert/testdata/regression/link-not-yet-published/draft.md create mode 100644 internal/convert/testdata/regression/link-not-yet-published/main.md create mode 100644 internal/convert/testdata/regression/link-not-yet-published/test.input create mode 100644 internal/convert/testdata/regression/link-not-yet-published/test.output diff --git a/internal/convert/links.go b/internal/convert/links.go index aa6fedb..dc743c1 100644 --- a/internal/convert/links.go +++ b/internal/convert/links.go @@ -93,13 +93,15 @@ func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bo // rewriteDocLink rewrites a sibling .md href (with optional fragment) to its // Confluence URL. ok=false with brokenText=="" means an absolute URL, a -// non-.md href, or a target with no page_id yet -- none of these are errors; -// the last one warns (minimal R1: reported, not silently dead) via the same -// r.warnings list images.go already populates on a broken reference. A -// non-empty brokenText means the target is Broken -- missing entirely, or -// resolving outside the documentation root -- and the caller must render -// that text in place of any link element, the way images.go already does -// for a missing image. +// non-.md href, or a target that exists but has no page_id yet -- none of +// these are errors; the last one warns (minimal R1: reported, not silently +// dead) via the same r.warnings list images.go already populates on a broken +// reference. A non-empty brokenText means the target is Broken -- missing +// entirely, or resolving outside the documentation root -- and the caller +// must render that text in place of any link element, the way images.go +// already does for a missing image. FileExists is what tells "missing +// entirely" apart from "not published yet": both look identical to +// index.Page (a miss), but only the first is a defect. func (r *storageRenderer) rewriteDocLink(href string) (newHref string, ok bool, brokenText string) { path, fragment := href, "" if i := strings.Index(href, "#"); i >= 0 { @@ -114,13 +116,21 @@ func (r *storageRenderer) rewriteDocLink(href string) (newHref string, ok bool, key, escapes := r.resolveDocKey(path) entry, found := r.index.Page(key) if !found { - if escapes { + switch { + case escapes: msg := fmt.Sprintf("LINK BROKEN: %s (outside the documentation root)", href) r.broken = append(r.broken, msg) return "", false, msg + case !r.index.FileExists(key): + msg := fmt.Sprintf("LINK BROKEN: %s (not found)", href) + r.broken = append(r.broken, msg) + return "", false, msg + 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, fmt.Sprintf("link not resolved: %s", href)) + return "", false, "" } - r.warnings = append(r.warnings, fmt.Sprintf("link not resolved: %s", href)) - return "", false, "" } var built string 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..5cad900 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": [ + "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

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/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..f38c6d2 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": [ + "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: LINK BROKEN: unknown.md (not found).

\n

An absolute URL is never rewritten: external.

\n", + "warnings": [] } 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..aab745a --- /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": [ + "link not resolved: draft.md" + ] +} From f3e10091c61f399ba5b5f953c727413f829b3745 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 12:17:30 -0400 Subject: [PATCH 07/14] feat(convert): warn on a fragment that matches no heading Both anchor branches in rewriteHref used to no-op silently on a miss -- a same-page #fragment or a cross-file path.md#fragment that named no real heading published with no word about it at all. Now both warn "anchor not found: ...". The cross-file branch gates on FileExists so a target that doesn't exist at all isn't double-reported: rewriteDocLink already covers that case as Broken on its own. Added a regression fixture covering both branches; nothing existing exercised an anchor miss at all. --- internal/convert/links.go | 10 ++++++++++ .../regression/link-anchor-not-found/main.md | 20 +++++++++++++++++++ .../link-anchor-not-found/sibling.md | 9 +++++++++ .../link-anchor-not-found/test.input | 3 +++ .../link-anchor-not-found/test.output | 9 +++++++++ 5 files changed, 51 insertions(+) create mode 100644 internal/convert/testdata/regression/link-anchor-not-found/main.md create mode 100644 internal/convert/testdata/regression/link-anchor-not-found/sibling.md create mode 100644 internal/convert/testdata/regression/link-anchor-not-found/test.input create mode 100644 internal/convert/testdata/regression/link-anchor-not-found/test.output diff --git a/internal/convert/links.go b/internal/convert/links.go index dc743c1..858cf7d 100644 --- a/internal/convert/links.go +++ b/internal/convert/links.go @@ -70,6 +70,11 @@ func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bo // doc-link step cannot resolve it (a file with no page_id yet). href = encodeDestination(r.currentBasename) + "#" + escapeFragment(nf) rewritten = true + } else { + // currentDocKey is the file being converted right now, so it + // unconditionally exists -- a miss here is always a genuine + // fragment that matches no heading, never a missing target. + r.warnings = append(r.warnings, fmt.Sprintf("anchor not found: %s", href)) } } else if path, frag, ok := splitMarkdownAnchor(href); ok { key, _ := r.resolveDocKey(path) @@ -78,6 +83,11 @@ func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bo // and the doc-link step decodes it again for its own lookup. href = path + "#" + escapeFragment(nf) rewritten = true + } else if r.index.FileExists(key) { + // Only warn here when the target file itself exists: otherwise + // rewriteDocLink below reports the missing/escaping target once, + // as Broken, and a second "anchor not found" would be redundant. + r.warnings = append(r.warnings, fmt.Sprintf("anchor not found: %s", href)) } } 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..be38454 --- /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": [ + "anchor not found: #no-such-heading", + "anchor not found: sibling.md#no-such-heading" + ] +} From f6e15c8ca388e4ef20bde0197f0da85045196674 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 13:02:50 -0400 Subject: [PATCH 08/14] feat(convert): prefix Broken/Warning messages with a source line number nodeLine walks a node to its first descendant *ast.Text (the same Segment substrate nodeText already reads) and converts that byte offset to a 1-indexed line, since neither *ast.Link nor *ast.Image carries its own position -- goldmark's parser never calls SetLines on either. Wired into every existing message site (images.go's four Broken and two Warnings, tables.go's cell-background warning) and the link sites added over the last few commits, via a new storageRenderer.linePrefix helper. lineOffset on storageRenderer corrects for the fact that goldmark parses md.Body, which Extract has already stripped of its frontmatter block -- every position it reports is relative to the body, not the file a reader would open and count lines in. Without it, a document with frontmatter reports a line number that is wrong by exactly the frontmatter's own line count. Regenerated every regression golden this touches (every one of them, since the change lands on pre-existing messages too, not just the ones from the last few commits) and updated the README's example messages to match. --- README.md | 31 +++-- internal/convert/convert.go | 6 + internal/convert/images.go | 20 ++-- internal/convert/links.go | 30 +++-- internal/convert/nodeline_test.go | 110 ++++++++++++++++++ internal/convert/renderer.go | 44 +++++++ internal/convert/tables.go | 2 +- .../regression/doc-links-encoded/test.output | 4 +- .../regression/image-properties/test.output | 4 +- .../regression/images-broken/test.output | 8 +- .../regression/images-encoded-src/test.output | 4 +- .../regression/internal-doc-links/test.output | 4 +- .../link-anchor-not-found/test.output | 4 +- .../link-not-yet-published/test.output | 2 +- .../regression/link-outside-root/test.output | 4 +- .../regression/table-cell-colors/test.output | 4 +- 16 files changed, 231 insertions(+), 50 deletions(-) create mode 100644 internal/convert/nodeline_test.go diff --git a/README.md b/README.md index 486adfc..4f3c247 100644 --- a/README.md +++ b/README.md @@ -1040,8 +1040,8 @@ Example: ``` **Images** — `![alt](./path.png)` uploads a local file as an attachment (or -references a remote URL); a missing/unsupported image becomes `IMAGE BROKEN: …` -text. +references a remote URL); a missing/unsupported image becomes +`line N: IMAGE BROKEN: …` text (`N` is the line it's on in the file). Image paths resolve relative to the Markdown file, the same way they do when you view the file on GitHub, so a page in a subdirectory can share an asset @@ -1073,8 +1073,8 @@ That layout needs a [documentation root](#the-documentation-root) declared at Every image is bounded by the [documentation root](#the-documentation-root): one resolving outside it (`../../secrets/x.png`) is reported as -`IMAGE BROKEN: … (outside the documentation root)` rather than uploaded, and a -symlink is refused even when it resolves inside the root. +`line N: IMAGE BROKEN: … (outside the documentation root)` rather than +uploaded, and a symlink is refused even when it resolves inside the root. Confluence attachment names cannot contain `/`, so the path — relative to the root, not to the page — is percent-encoded into the attachment name: @@ -1117,11 +1117,24 @@ space is written `[see](my%20doc.md)` (or `[see]()`), and a bare `[see](my doc.md)` is not a link at all. The same applies to the fragment, so a non-ASCII heading anchor may arrive as `#caf%C3%A9-section`. Both are decoded before markfluence matches them against files and headings on disk, so either -spelling resolves. A link it cannot resolve — a target with no `page_id`, or a -file that isn't there — is left exactly as written and published as-is, which on -Confluence is a dead relative link. A `.md` link shaped like a same-tree -reference gets a warning when this happens; a mention, an attachment link, or -an external URL was never meant to resolve here and stays silent. +spelling resolves. + +Whether an unresolved link is reported — and how badly — depends on why: + +* A target that **doesn't exist at all**, or **resolves outside the + documentation root**, is Broken: the whole link element is replaced with + literal `line N: LINK BROKEN: … (not found)` or + `line N: LINK BROKEN: … (outside the documentation root)` text, the same + way a broken image already is. +* A target that **exists but has no `page_id` yet** — the normal state of + every page in a tree that hasn't been published — is a Warning + (`link not resolved: …`); the href still renders exactly as written. +* A `#fragment` that **matches no heading** on an otherwise-resolvable target + is also a Warning (`anchor not found: …`); the link still works, it just + lands at the top of the page instead of the named heading. + +A mention, an attachment link, or an external URL was never meant to resolve +here and stays silent either way. **Comment directives:** - `` — replaced with Confluence table-of-contents macro. 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 858cf7d..d172391 100644 --- a/internal/convert/links.go +++ b/internal/convert/links.go @@ -20,7 +20,7 @@ import ( // 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 { @@ -32,7 +32,7 @@ func (r *storageRenderer) renderLink( _, _ = w.WriteString("") return ast.WalkContinue, nil } - href, rewritten, brokenText := 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)) @@ -59,8 +59,13 @@ func (r *storageRenderer) renderLink( // rewritten href is already fully composed and escaped, so it must be written // verbatim rather than re-escaped) -- or, when the link resolves to a Broken // target, the literal replacement text to render instead of any element, -// in which case href/rewritten are meaningless and must not be used. -func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bool, brokenText string) { +// in which case href/rewritten are meaningless and must not be used. node and +// source are the link node being rendered and the document's raw bytes, +// passed through only to prefix a reported line number onto any message. +func (r *storageRenderer) rewriteHref( + href string, node ast.Node, source []byte, +) (newHref string, rewritten bool, brokenText string) { + prefix := r.linePrefix(node, source) if strings.HasPrefix(href, "#") { if nf, ok := r.index.Anchor(r.currentDocKey, decodeDestination(href[1:])); ok { // Same-page anchors become fake cross-file links to the current @@ -74,7 +79,7 @@ func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bo // currentDocKey is the file being converted right now, so it // unconditionally exists -- a miss here is always a genuine // fragment that matches no heading, never a missing target. - r.warnings = append(r.warnings, fmt.Sprintf("anchor not found: %s", href)) + r.warnings = append(r.warnings, prefix+fmt.Sprintf("anchor not found: %s", href)) } } else if path, frag, ok := splitMarkdownAnchor(href); ok { key, _ := r.resolveDocKey(path) @@ -87,11 +92,11 @@ func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bo // Only warn here when the target file itself exists: otherwise // rewriteDocLink below reports the missing/escaping target once, // as Broken, and a second "anchor not found" would be redundant. - r.warnings = append(r.warnings, fmt.Sprintf("anchor not found: %s", href)) + r.warnings = append(r.warnings, prefix+fmt.Sprintf("anchor not found: %s", href)) } } - newHref, ok, brokenText := r.rewriteDocLink(href) + newHref, ok, brokenText := r.rewriteDocLink(href, prefix) if brokenText != "" { return "", false, brokenText } @@ -111,8 +116,9 @@ func (r *storageRenderer) rewriteHref(href string) (newHref string, rewritten bo // must render that text in place of any link element, the way images.go // already does for a missing image. FileExists is what tells "missing // entirely" apart from "not published yet": both look identical to -// index.Page (a miss), but only the first is a defect. -func (r *storageRenderer) rewriteDocLink(href string) (newHref string, ok bool, brokenText string) { +// index.Page (a miss), but only the first is a defect. prefix is rewriteHref's +// already-computed line prefix, threaded through rather than recomputed. +func (r *storageRenderer) rewriteDocLink(href, prefix string) (newHref string, ok bool, brokenText string) { path, fragment := href, "" if i := strings.Index(href, "#"); i >= 0 { path, fragment = href[:i], href[i:] @@ -128,17 +134,17 @@ func (r *storageRenderer) rewriteDocLink(href string) (newHref string, ok bool, if !found { switch { case escapes: - msg := fmt.Sprintf("LINK BROKEN: %s (outside the documentation root)", href) + msg := prefix + fmt.Sprintf("LINK BROKEN: %s (outside the documentation root)", href) r.broken = append(r.broken, msg) return "", false, msg case !r.index.FileExists(key): - msg := fmt.Sprintf("LINK BROKEN: %s (not found)", href) + msg := prefix + fmt.Sprintf("LINK BROKEN: %s (not found)", href) r.broken = append(r.broken, msg) return "", false, msg 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, fmt.Sprintf("link not resolved: %s", href)) + r.warnings = append(r.warnings, prefix+fmt.Sprintf("link not resolved: %s", href)) return "", false, "" } } 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 3c2a23a..45daf4f 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -55,6 +55,50 @@ type storageRenderer struct { // 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/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/test.output b/internal/convert/testdata/regression/doc-links-encoded/test.output index 5cad900..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": [ - "LINK BROKEN: nosuch%20file.md (not found)" + "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

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", + "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/test.output b/internal/convert/testdata/regression/internal-doc-links/test.output index f38c6d2..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": [ - "LINK BROKEN: unknown.md (not found)" + "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: LINK BROKEN: unknown.md (not found).

\n

An absolute URL is never rewritten: external.

\n", + "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/test.output b/internal/convert/testdata/regression/link-anchor-not-found/test.output index be38454..82e7a81 100644 --- a/internal/convert/testdata/regression/link-anchor-not-found/test.output +++ b/internal/convert/testdata/regression/link-anchor-not-found/test.output @@ -3,7 +3,7 @@ "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": [ - "anchor not found: #no-such-heading", - "anchor not found: sibling.md#no-such-heading" + "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/test.output b/internal/convert/testdata/regression/link-not-yet-published/test.output index aab745a..9d42b6d 100644 --- a/internal/convert/testdata/regression/link-not-yet-published/test.output +++ b/internal/convert/testdata/regression/link-not-yet-published/test.output @@ -3,6 +3,6 @@ "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": [ - "link not resolved: draft.md" + "line 8: link not resolved: draft.md" ] } diff --git a/internal/convert/testdata/regression/link-outside-root/test.output b/internal/convert/testdata/regression/link-outside-root/test.output index 961ccc7..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": [ - "LINK BROKEN: ../outside/linked.md (outside the documentation root)" + "line 3: LINK BROKEN: ../outside/linked.md (outside the documentation root)" ], - "html": "

Escaping Link

\n

A 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", + "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\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)" ] } From 4f82a2856e05a1c227ed44579c077ed4d57c18d0 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 13:11:51 -0400 Subject: [PATCH 09/14] feat(check): add the check command Closes #42. Validates one or more markdown FILEs against the converter and frontmatter rules with no network access and no credentials: it never imports internal/client (guarded by a static import check in check_test.go), never writes to disk, and never contacts Confluence. Frontmatter validation: an unparseable/unterminated block (via frontmatter.Parse's new error), an invalid page_width, a present-but- non-numeric page_id. Deliberately not checked: whether page_id/space/parent are present at all -- check cannot know the caller's intended operation, and a file legitimately has none of these before its first create. Reuses update/create's project.Cache/linkindex.Cache pattern to build the root/index pair MdToConfluence needs, against hardcoded baseURL/spaceKey (the regression suite's own defaults) rather than flags, since neither affects what Broken/Warnings report. --show-html adds the converted storage HTML and attachment list to a result that reached the converter: indented by nesting depth in human output (a per-line indent, not a whitespace-normalizing reformat, since the renderer already breaks lines at every structural boundary) and as a `debug: {html, attachments} | null` field in --json, where html stays compact/unindented to match what update/create would literally publish. Schema: added "check" to the command enum, an if/then branch, and the checkResult/checkSummary/checkAttachment $defs. --- cmd/check/check.go | 161 ++++++++++++++++++++++ cmd/check/check_test.go | 264 +++++++++++++++++++++++++++++++++++++ cmd/check/json.go | 204 ++++++++++++++++++++++++++++ cmd/check/json_test.go | 110 ++++++++++++++++ cmd/root.go | 2 + schema/json-output/v1.json | 64 ++++++++- 6 files changed, 804 insertions(+), 1 deletion(-) create mode 100644 cmd/check/check.go create mode 100644 cmd/check/check_test.go create mode 100644 cmd/check/json.go create mode 100644 cmd/check/json_test.go diff --git a/cmd/check/check.go b/cmd/check/check.go new file mode 100644 index 0000000..b74dacb --- /dev/null +++ b/cmd/check/check.go @@ -0,0 +1,161 @@ +// 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: an unpublished sibling link (no page_id yet) is the normal state\n" + + "of a tree that hasn't been published, not a defect.", + 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.ok = false + r.status = statusBroken + case len(r.warnings) > 0: + r.ok = true + r.status = statusWarnings + default: + r.ok = true + 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..e20f804 --- /dev/null +++ b/cmd/check/json.go @@ -0,0 +1,204 @@ +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 + ok bool + 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.ok = false + r.status = statusFailed + r.errMsg = err.Error() + r.code = code + return r +} + +// 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

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/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/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", From f7a0efc215d09b1f9836773343eaafa7df7d9668 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 13:29:12 -0400 Subject: [PATCH 10/14] docs(guarantees): bump R1 to Holds The dedicated diagnostic R1's own Partial note named as missing now exists: a doc-link that's missing entirely or escapes the documentation root is Broken and replaces the published element; one that exists but isn't published yet, or whose fragment matches no heading, warns; every message carries its source line; and check adds the "audit without publishing" half with no network access or writes. Scoped explicitly to the two reference kinds markfluence actually attempts to resolve -- doc-links and images. A relative link to a non-.md, non-image local file (e.g. a PDF) is never a resolution attempt at all, so it sits outside R1's claim rather than inside it unmet. --- docs/guarantees.md | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/docs/guarantees.md b/docs/guarantees.md index 591fe72..a0b4d04 100644 --- a/docs/guarantees.md +++ b/docs/guarantees.md @@ -205,17 +205,35 @@ 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. `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 From b7a747cb7e0fe39a070fc8de218528ca16d0c2db Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 13:35:37 -0400 Subject: [PATCH 11/14] docs(readme): document the check command New "Validating markdown locally" usage group, a ### check section (what it checks, the Broken-fails/Warnings-don't rule, --show-html), and updates to the --json section's status-verb and per-target notes plus a new bullet on check's broken-status ok:false-with-no-error/code shape and the debug field. --- README.md | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4f3c247..395c0fd 100644 --- a/README.md +++ b/README.md @@ -201,6 +201,12 @@ markfluence children --help markfluence export --help ``` +Validating markdown locally (no network, no credentials): + +```sh +markfluence check --help +``` + Manipulating Confluence page attachments: ```sh @@ -338,6 +344,47 @@ markfluence fix docs/*.md markfluence fix docs/foo.md --dry-run ``` +### `check` + +``` +Usage: markfluence check FILE... [flags] +``` + +Validate one or more Markdown files against the converter and frontmatter +rules — offline: no network access, no credentials, and no writes to +Confluence or to disk. This is the primitive a CI job, a pre-commit hook, or +an agent editing docs wants: validate every change instantly, with no risk of +publishing anything. Each file is processed independently; the command exits +non-zero if any file is broken or fails outright. + +It reports the same `Broken`/`Warnings` a real `update`/`create` would +produce — a missing or escaping image/link, an unpublished sibling link, a +`#fragment` matching no heading — each prefixed with the source line it came +from, plus three frontmatter checks: an unparseable/unterminated frontmatter +block, an invalid `page_width`, and a present-but-non-numeric `page_id`. +Deliberately not checked: whether `page_id`/`space`/`parent` are set at all — +`check` can't know whether you're about to `create` or `update`, and a false +positive there would be worse than a miss. A **Broken** result fails +(`update`/`create` would publish literal `LINK BROKEN: …`/`IMAGE BROKEN: …` +text); a **Warning** alone does not — an unpublished sibling link is the +normal state of a tree that hasn't been created yet, not a defect. + +```console +$ markfluence check docs/*.md + ✗ [docs/broken-links.md] line 12: LINK BROKEN: typo-target.md (not found) + [docs/guide.md] clean + ✗ 1 of 2 file(s) failed. +``` + +`--show-html` additionally prints the converted storage HTML (indented by +nesting depth) and the attachment list, for debugging what a file would +actually publish without publishing it: + +```sh +markfluence check docs/*.md +markfluence check --show-html docs/one-page.md +``` + ### `info` ``` @@ -763,15 +810,25 @@ Notes on the schema: to speak of. - **Status verbs** are per-command: `published`/`skipped` (`update`), `created`/`not_created` (`create`), `changed`/`consistent` (`fix`), + `clean`/`warnings`/`broken` (`check`), `created`/`updated`/`skipped` (`attachment-upload`), `downloaded`/`skipped` (`attachment-download`), plus `failed`. `info`, `read`, and `attachment-list` results carry data only (no status verb). - **One result per target**, and the target is per-command: the page for - `info`/`read`/`export` (always one), the file for `update`/`create`/`fix`, and - the attachment for the three `attachment-*` commands — so + `info`/`read`/`export` (always one), the file for `update`/`create`/`fix`/`check`, + and the attachment for the three `attachment-*` commands — so `.results[] | .filename` works and `summary.total` is the attachment count. `export` nests the files it wrote in an `attachments` array on its page result, the way `update`/`create` do. +- **`check`'s `broken` status is `ok: false` with no `error`/`code`** — unlike + every other failure, its `broken`/`warnings` arrays already say everything + there is to say, so there's no separate operational error to attach. Only + its `failed` status (a file that never reached the converter at all) sets + them, the same as every other command's failure. `check --show-html` adds a + `debug: { html, attachments } | null` field, populated only for a file that + reached the converter; `html` stays exactly what the converter produced + (unindented), since it's meant to match what `update`/`create` would + literally publish. - **Compound values are objects**, never display strings — `version`, `page_width`, and the `created`/`updated` author stamps on `info`. - **`create`'s two-phase abort** (a validation failure means nothing is created) From a1b48136f36d743d7795cc331212904ac6421fa1 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 30 Aug 2026 13:37:58 -0400 Subject: [PATCH 12/14] docs: add check to the architecture notes CLAUDE.md gains a cmd/check/ bullet; the internal/convert writeup gains the link-severity split, the linkBrokenText output-replacement mechanism, and the nodeLine/linePrefix/lineOffset line-number machinery; internal/frontmatter's writeup notes Parse's new error return. Also corrected two pieces of pre-existing drift the same paragraph already carried: MdToConfluence's signature was still shown pre-026 (missing root/index), and links.go's description still referenced a "docKey" that no longer exists and claimed unresolved-link failure is silent, which #42 is what makes untrue. --- CLAUDE.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 `