From 06134cf892dd626b8c8b7f383985589f1033de86 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 17:59:24 -0400 Subject: [PATCH 1/6] docs: plan for converting in create's preflight phase --- _plans/034_create-preflight-conversion.md | 208 ++++++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 _plans/034_create-preflight-conversion.md diff --git a/_plans/034_create-preflight-conversion.md b/_plans/034_create-preflight-conversion.md new file mode 100644 index 0000000..d054186 --- /dev/null +++ b/_plans/034_create-preflight-conversion.md @@ -0,0 +1,208 @@ +# Plan: convert in create's preflight phase + +Make `create` convert every file in phase 1, so a document defect the converter +finds aborts the batch instead of surfacing after phase 2 has already created a +page and written its id into the file. Fixes #127. + +## The bug + +`create` is three-phase (`cmd/create/create.go`): + +1. **Preflight** -- per file: parse frontmatter, resolve root/index, title, + width, then `checkPageID`, space resolution, `resolveParent`, and + `checkTitleFree`. If any file fails, `abort()` runs and nothing is created. +2. **Reserve** -- `reserveOne` creates a content-less page for each file, + parents-first, and immediately writes `page_id` back into the frontmatter. +3. **Publish** -- `publishOne` calls `MdToConfluence` and `UpdatePage`s the real + content in. + +Nothing in phase 1 converts, so a conversion failure lands in phase 3. Concretely, +with #59's base-name attachment naming: `doc.md` referencing both +`arch/diagram.png` and `deploy/diagram.png` wants one attachment named +`diagram.png` for two assets, which `images.go:121` refuses with a +`NameCollisionError`. By then the page exists and `doc.md` carries its id, so the +user has a content-less page, an id they did not ask for, and a re-run that fails +with "a page already exists at page_id" from `checkPageID`. Recovering means +deleting the page or hand-editing the frontmatter. + +Phase-3 failures have always left stubs -- `publishOne`'s comment says so, and +`_plans/026` accepted it -- because the usual one is a network or server +condition nothing local could predict. The collision is the first that is purely +a property of files on disk: `markfluence check` diagnoses it with no client, no +credentials, and no network, and everything it needs (the images, the root) is +resolved by the end of phase 1. So this one is knowable before anything is +created, and phase 1 simply is not asking. + +## Decisions + +**Phase 3 cannot reuse phase 1's conversion; convert twice.** This was #127's +open question and the answer is no. `createAll` seeds the shared link index +between the phases (`r.index.SetPage(...)`, create.go:389) and `rewriteDocLink` +puts `entry.PageID` straight into the built URL (links.go:180-182), so a phase-1 +conversion runs against an unseeded index and renders "link not resolved" for +every in-set sibling link. Publishing that result would be a correctness +regression -- exactly the ordering dependency `_plans/026` removed. The extra +cost is one local conversion per file over bytes already in memory: no network, +no I/O. + +**The converse is what makes the preflight sound: no error path reads the +index.** `MdToConfluence` returns exactly two errors -- `NameCollisionError` +(images.go:121) and goldmark's own `Convert` failure (convert.go:103) -- and +neither consults `index`. So phase 1's verdict is neither a false positive nor a +false negative against phase 3's, and the error text is identical. That is the +invariant the whole design rests on, so it gets a test rather than only a +comment. + +**Phase 1's `Broken` and `Warnings` are discarded.** They are *wrong* for any +file linking to an in-set sibling, for the same unseeded-index reason: phase 1 +warns "link not resolved" where phase 3 emits a real URL. Only the error return +is read. Nothing about a successful run's output changes. This looks like an +oversight, so the doc comment says plainly that they are thrown away and why. + +Filling them into an aborted result was considered and rejected: they would be +unreliable in exactly the multi-file case `abort()` exists for, and `broken` on +an aborted result reads as though a page went up with dead links. + +**The conversion runs last in `resolveFile`, after `checkTitleFree`.** Existing +error precedence is then untouched -- the page_id-first ordering that +`resolveFile`'s comment justifies ("the most specific thing wrong with the file", +three fewer API calls) and that tests pin stays exactly as it is. The cost, +stated plainly: a file that cannot convert still makes ~4 API calls before being +told. Running it first would report a document defect with zero API calls, +matching `check`, but it re-orders precedence for a file that is wrong in two +ways at once, and that reasoning is load-bearing enough not to churn for a +saving that only applies to broken input. + +It is called with the real `c.SiteURL()`, `spaceKey`, and `buildinfo.Stamp()` -- +all in hand at that point in `resolveFile` -- rather than `check`-style +placeholders. The error does not depend on any of them, but passing the truth +costs nothing and does not invite the question. + +**A preflight conversion failure reports `CONVERT`, not `VALIDATION`.** The same +defect reports `CodeConvert` from phase 3 today, so keeping it means a `--json` +consumer's distinction between "the document did not convert" and "the server or +the frontmatter said no" survives the move. `abort()` currently hardcodes +`jsonout.CodeValidation` (json.go:203, json.go:210); `failure` gains a `code` +field instead. The schema needs no change: its `code` enum is global (v1.json:159) +and already contains both, and nothing in the `create` branch constrains which +appears. + +**No special case for `NameCollisionError`.** `check` distinguishes it +(check.go:173) because there it is a document defect like a dead link rather than +the converter having failed. As the type's own doc comment says, "publishing +commands need no such distinction: either way the file does not go up." One +`if err != nil` branch, and a future third converter error is covered for free. + +**`--dry-run` changes behavior for a defective file, and must.** A dry-run +converts in `publishOne` today, so a colliding file currently prints a per-file +`CONVERT` failure under the `DRY RUN` banner. After this it aborts the batch -- +which is right, because a dry-run's job is to predict the real run, and the real +run now aborts. This is a behavior change to note, not a regression. + +**Scope is the conversion only.** Two adjacent things stay out, below. + +## Implementation + +### `cmd/create/create.go` + +- `convertFailure` -- a typed wrapper for a preflight conversion error, mirroring + the existing `pageIDFailure` idiom in this file (a typed phase-1 error carrying + what the result needs beyond the message). Holds the wrapped error and + implements `Error()`/`Unwrap()`. +- `resolveFile` -- after `checkTitleFree`, before building the `record`: + + ```go + // Convert now, discarding everything but the error. A defect the converter + // finds is a property of the file on disk, so asking here -- before phase 2 + // creates anything -- is what keeps #127's stub from being made at all. + // + // The page is thrown away rather than reused by publishOne, and its Broken + // and Warnings with it: this index has not been seeded with the batch's own + // page ids yet, so an in-set link renders unresolved here and resolves + // there. The *error* is the same either way -- neither NameCollisionError + // nor goldmark's Convert failure reads the index at all. + if _, err := convert.MdToConfluence( + mf, root, index, c.SiteURL(), spaceKey, buildinfo.Stamp(), + ); err != nil { + return record{}, &convertFailure{err: err} + } + ``` + +- `failure` gains `code jsonout.Code`. +- `newFailure` sets `code: jsonout.CodeValidation` by default and + `jsonout.CodeConvert` when `errors.As(err, &cf)` matches a `*convertFailure`, + alongside the `pageIDFailure` field-carrying it already does. +- The two bare `failure{...}` literals in `run` -- "parent page is not in the + target space" and the `"(hierarchy)"` topo-sort failure -- set + `code: jsonout.CodeValidation` explicitly. They must: `abortedResult` only + emits `code` when `message` is non-empty, so a zero-value `code` would put + `""` into a field whose enum does not contain it. +- `publishOne`'s doc comment: keep the stub-is-permanent paragraph, and add that + a conversion failure no longer reaches it -- what survives here is a server or + network condition no local check could have predicted (S7). + +### `cmd/create/json.go` + +- `abort()` -- both `abortedResult(..., jsonout.CodeValidation)` calls become + `abortedResult(..., f.code)`. + +### `docs/guarantees.md` + +Add **S7** `no-partial-create`, status **Partial**: a file that fails leaves no +page behind. The prose states the limit rather than burying it -- every failure +knowable from the files on disk is caught in preflight, so nothing a document +defect can do creates a page; a phase-3 server or network failure still leaves a +content-less stub, with its id already persisted, which a plain `markfluence +update` finishes. Note that this is why the guarantee is Partial and not Holds, +and that `_plans/026` accepted the stub deliberately as the price of removing +`create`'s ordering dependency. + +The "How each kind is verified" table at the foot of the file is per-kind rather +than per-guarantee; Safety's row ("adversarial tests: traversal attempts, +pre-existing files") gains S7's shape -- a failing preflight asserted to have +created nothing. + +## Tests + +- **End-to-end, `cmd/create/run_test.go`**: a fixture referencing + `arch/diagram.png` and `deploy/diagram.png` through the existing + `fakeConfluence`. Assert the batch aborted, `f.pages` is empty, and the file's + frontmatter still has no `page_id`. The fake's own constraint helps here -- + it has no attachment support and `default`s to `t.Errorf("unexpected + request")`, so a fixture that reached phase 3 fails loudly on its own. +- **The batch is refused, not just the bad file**: two files, one colliding. + The clean one reports `not_created`, and no page exists for it either. This + pins the actual behavioral change -- a conversion failure goes through + `abort()` rather than failing one file in place. +- **`--json` code is `CONVERT`**: an aborted result for the colliding file + reports `CONVERT` while a page_id or title failure in the same batch still + reports `VALIDATION`. Pins `failure.code` against a refactor collapsing it + back to a constant. +- **Phase 1 and phase 3 agree on the error** (`internal/convert`): the same file + converted against an unseeded index and against one seeded with a page entry + returns the identical error. This is the invariant the design rests on -- that + no error path reads the index -- and its failure mode is someone making the + index matter to an error, which is exactly what should break a test. + +## Docs + +- `README.md` -- the `create` section: preflight converts, so a document defect + aborts the batch instead of leaving a stub. +- `CLAUDE.md` -- the `cmd/create` bullet: phase 1 converts and discards the + result, with the reuse-is-unsound reason in one clause. + +## Out of scope + +- **Deleting the stub on a phase-3 failure.** It would make S4 + (`no-removal-as-side-effect`) and S5 (`remove-only-ours`) non-vacuous for the + first time, and S4 says removal is a command's stated purpose or it does not + happen. That needs its own argument and its own status change, not a + ride-along. +- **Pre-flighting attachment readability.** `SyncAttachments` opens each file to + upload it, and an image that passed `Lstat` in `images.go` can still fail to + open. Checking it in phase 1 duplicates work the upload does anyway and races + the filesystem -- the check passes and the upload still fails -- and unlike + the converter it is not network-free, since it needs the page's existing + attachments. +- **`update`.** It has no reserve phase, so a conversion failure already fails + the file with nothing created. Nothing to fix. From 80b827205a0fd3bc834262c19dbca129f8687797 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 18:01:04 -0400 Subject: [PATCH 2/6] test(convert): the error does not depend on the link index The invariant create's preflight conversion will rest on. Its verdict is only as good as phase 3's because neither NameCollisionError nor goldmark's own failure reads the index -- and the page emphatically does depend on it, which is why a preflight result cannot be reused. Both halves are asserted, since "the errors match" proves nothing if the seeding never mattered. --- internal/convert/errorindex_test.go | 119 +++++++++++++++++++++++++ internal/convert/namecollision_test.go | 11 +-- internal/convert/testroot_test.go | 16 ++++ 3 files changed, 136 insertions(+), 10 deletions(-) create mode 100644 internal/convert/errorindex_test.go diff --git a/internal/convert/errorindex_test.go b/internal/convert/errorindex_test.go new file mode 100644 index 0000000..8b02855 --- /dev/null +++ b/internal/convert/errorindex_test.go @@ -0,0 +1,119 @@ +package convert_test + +// The invariant cmd/create's preflight conversion rests on: MdToConfluence's +// *error* does not depend on the link index, even though its rendered page +// emphatically does. +// +// create is three-phase (_plans/034). Preflight validates every file, reserve +// creates a content-less page for each and seeds its id into the shared index, +// publish converts and fills each page in. Preflight converts too, purely to +// learn whether the file can convert at all -- which is only sound because +// neither NameCollisionError (images.go) nor goldmark's own failure reads the +// index, so the verdict cannot differ between the two phases. It is also why +// the preflight result is *discarded* rather than reused by publish: the HTML +// does differ, since an in-set link resolves only once the id is there. +// +// Both halves are asserted together on purpose. Either alone can pass +// vacuously -- "the errors match" proves nothing if the seeding never mattered. + +import ( + "os" + "path/filepath" + "testing" + + "github.com/mozilla/markfluence/internal/convert" + "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/linkindex" + "github.com/mozilla/markfluence/internal/project" +) + +// convertSeeded renders body as root/main.md against the link index for root +// with seed applied to it, mirroring what create's reserve phase does to the +// index it shares with publish. A nil seed is the unseeded (preflight) case. +func convertSeeded( + t *testing.T, root, body string, seed map[string]linkindex.PageEntry, images ...string, +) (*convert.ConfluencePage, error) { + t.Helper() + writeImages(t, root, images...) + md, err := frontmatter.Parse(filepath.Join(root, "main.md"), body) + if err != nil { + t.Fatal(err) + } + r, err := project.FromPath(root) + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.FS.Close() }() + idx := testIndex(t, r) + for key, entry := range seed { + idx.SetPage(key, entry) + } + return convert.MdToConfluence(md, r, idx, "https://wiki.example.net", "ENG", "vtest") +} + +// siblingEntry is what create's reserve phase seeds for an in-set file: the id +// of the stub it just created, under the file's root-relative key. +var siblingEntry = map[string]linkindex.PageEntry{ + "sibling.md": {PageID: "4242", Title: "Sibling"}, +} + +// writeSibling creates an unpublished sibling.md -- a title and no page_id, +// which is what makes a link to it unresolvable until an id is seeded. +func writeSibling(t *testing.T, root string) { + t.Helper() + path := filepath.Join(root, "sibling.md") + if err := os.WriteFile(path, []byte("---\ntitle: Sibling\n---\n\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } +} + +// TestErrorDoesNotDependOnTheIndex is the invariant itself: a file that cannot +// convert returns the identical error whether or not the index knows the +// batch's own page ids. The link comes before the images because a collision +// stops the walk -- put it after and the case would never resolve a link at +// all, and would pass for the wrong reason. +func TestErrorDoesNotDependOnTheIndex(t *testing.T) { + root := t.TempDir() + writeSibling(t, root) + body := "see [sibling](sibling.md)\n\n![a](a/diagram.png)\n\n![b](b/diagram.png)\n" + images := []string{"a/diagram.png", "b/diagram.png"} + + _, unseeded := convertSeeded(t, root, body, nil, images...) + _, seeded := convertSeeded(t, root, body, siblingEntry, images...) + + if unseeded == nil || seeded == nil { + t.Fatalf("want a refusal from both: unseeded=%v seeded=%v", unseeded, seeded) + } + if unseeded.Error() != seeded.Error() { + t.Errorf("the error depends on the index:\n unseeded: %s\n seeded: %s", unseeded, seeded) + } +} + +// TestPageDoesDependOnTheIndex is the other half, and the reason preflight's +// page is thrown away instead of reused: seeding an id turns "link not +// resolved" into a real URL, so a page converted before the reserve phase is +// not the page that should be published. +func TestPageDoesDependOnTheIndex(t *testing.T) { + root := t.TempDir() + writeSibling(t, root) + body := "see [sibling](sibling.md)\n" + + before, err := convertSeeded(t, root, body, nil) + if err != nil { + t.Fatalf("unseeded: %v", err) + } + after, err := convertSeeded(t, root, body, siblingEntry) + if err != nil { + t.Fatalf("seeded: %v", err) + } + + if before.HTML == after.HTML { + t.Fatalf("seeding an id changed nothing; this test proves nothing:\n%s", before.HTML) + } + if len(before.Warnings) != 1 { + t.Errorf("unseeded warnings = %v, want one \"link not resolved\"", before.Warnings) + } + if len(after.Warnings) != 0 { + t.Errorf("seeded warnings = %v, want none", after.Warnings) + } +} diff --git a/internal/convert/namecollision_test.go b/internal/convert/namecollision_test.go index fe802d4..9bc20fb 100644 --- a/internal/convert/namecollision_test.go +++ b/internal/convert/namecollision_test.go @@ -6,7 +6,6 @@ package convert_test // no correct way to publish it. import ( - "os" "path/filepath" "strings" "testing" @@ -19,15 +18,7 @@ import ( // convertBody renders body as root/main.md, returning the page or the error. func convertBody(t *testing.T, root, body string, images ...string) (*convert.ConfluencePage, error) { t.Helper() - for _, img := range images { - path := filepath.Join(root, filepath.FromSlash(img)) - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(path, []byte("PNG"), 0o644); err != nil { - t.Fatal(err) - } - } + writeImages(t, root, images...) md, err := frontmatter.Parse(filepath.Join(root, "main.md"), body) if err != nil { t.Fatal(err) diff --git a/internal/convert/testroot_test.go b/internal/convert/testroot_test.go index df0fe87..ba77b08 100644 --- a/internal/convert/testroot_test.go +++ b/internal/convert/testroot_test.go @@ -1,6 +1,7 @@ package convert_test import ( + "os" "path/filepath" "testing" @@ -39,3 +40,18 @@ func testIndex(t *testing.T, root *project.Root) *linkindex.Index { } return idx } + +// writeImages creates a stub image file at root for each slash-separated +// relative path, making the intermediate directories. +func writeImages(t *testing.T, root string, images ...string) { + t.Helper() + for _, img := range images { + path := filepath.Join(root, filepath.FromSlash(img)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("PNG"), 0o644); err != nil { + t.Fatal(err) + } + } +} From b543b6eb54d358ab08819dce6802207d58ac2e83 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 18:05:30 -0400 Subject: [PATCH 3/6] fix(create): convert in preflight, so a document defect creates nothing A conversion failure surfaced in the publish phase, after reserve had already created a page and written its id into the file -- leaving a content-less page, a page_id nobody asked for, and a re-run that failed with "a page already exists at page_id" instead. Fixes #127. Preflight converts every file and keeps nothing but the error. The result cannot be reused by publish: reserve seeds the batch's page ids into the shared link index in between, so an in-set link renders unresolved in preflight and resolves in publish. The error is the same either way, because no error path reads the index. Phase 1 now rejects a file for more than one reason, so the code travels on the failure instead of being hardcoded by abort(): a conversion failure reports CONVERT, the code it already reported from publish. The abort line says "failed preflight" rather than "failed validation" to match. --- README.md | 2 +- cmd/create/create.go | 74 ++++++++++++++++- cmd/create/json.go | 17 ++-- cmd/create/json_test.go | 21 ++--- cmd/create/run_test.go | 176 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 268 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 0cd15d4..6a3b840 100644 --- a/README.md +++ b/README.md @@ -247,7 +247,7 @@ of clash name the page in the way, so you can go look at it: ```console $ markfluence create docs/runbook.md ✗ [docs/runbook.md] a page already exists at page_id 123 ("Deploy Runbook"): https://wiki.example.net/wiki/spaces/ENG/pages/123/Deploy+Runbook - ✗ Aborting: 1 file(s) failed validation; nothing was created. + ✗ Aborting: 1 file(s) failed preflight; nothing was created. ``` A file whose `page_id` doesn't resolve is also a failure, not a fresh page: diff --git a/cmd/create/create.go b/cmd/create/create.go index 6a6f9fc..25aa750 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -9,6 +9,13 @@ // too. A parent cycle among the given files is a different graph -- the // reserve phase needs a real topological order for it and rejects the batch // outright when there isn't one. +// +// Preflight converts every file too, and throws the result away. Only the error +// is wanted: a document the converter refuses is refused before anything is +// created, rather than after the reserve phase has already made a page and put +// its id in the file (#127). The result cannot be reused by publish, because +// reserve seeds the batch's page ids into the shared link index in between -- +// see resolveFile. package create import ( @@ -126,9 +133,17 @@ type record struct { // failure is a phase-1 validation error against a file (or "(hierarchy)"). // pageID and url are set only for a frontmatter page_id failure, which is the one // validation error that can name a page: see pageIDFailure. +// +// code travels with the failure rather than being supplied by abort(), which +// used to hardcode VALIDATION for everything phase 1 rejected. It stopped being +// true once phase 1 started converting: a conversion failure reports CONVERT, +// the same code it reported from phase 3 before the check moved (see +// convertFailure). Every literal building one of these must set code, since +// abortedResult emits whatever is here and "" is not in the schema's enum. type failure struct { filename, message string pageID, url string + code jsonout.Code } // pageIDFailure is a phase-1 failure about a file's frontmatter page_id. create @@ -147,14 +162,34 @@ type pageIDFailure struct { func (e *pageIDFailure) Error() string { return e.message } +// convertFailure marks a phase-1 error as having come from the converter rather +// than from validation, so the failure reports CONVERT. Typed for the same +// reason pageIDFailure is: resolveFile hands back a plain error, and newFailure +// is the one place that turns one into a reportable failure -- so what a caller +// needs to know beyond the message travels in the error's type. +// +// It carries no message of its own. The converter's own error text is already +// the whole diagnosis (a NameCollisionError names both paths, with lines, and +// says which one to rename), and wrapping it in "converting: ..." would only +// push that further from the start of the line. +type convertFailure struct{ err error } + +func (e *convertFailure) Error() string { return e.err.Error() } +func (e *convertFailure) Unwrap() error { return e.err } + // newFailure records a phase-1 error against a file, carrying over the fields of -// a page_id failure so abort() can report them without re-fetching anything. +// a page_id failure so abort() can report them without re-fetching anything, +// and the code the error's type implies. func newFailure(filename string, err error) failure { - f := failure{filename: filename, message: err.Error()} + f := failure{filename: filename, message: err.Error(), code: jsonout.CodeValidation} var pf *pageIDFailure if errors.As(err, &pf) { f.pageID, f.url = pf.pageID, pf.url } + var cf *convertFailure + if errors.As(err, &cf) { + f.code = jsonout.CodeConvert + } return f } @@ -288,13 +323,17 @@ func run(cmd *cobra.Command, args []string) error { } for _, r := range records { if r.parent.kind == parentInSet && byAbs[r.parent.abs].spaceID != r.spaceID { - errs = append(errs, failure{filename: r.filename, message: "parent page is not in the target space"}) + errs = append(errs, failure{ + filename: r.filename, + message: "parent page is not in the target space", + code: jsonout.CodeValidation, + }) } } if len(errs) == 0 { ordered, err = topoSort(records, byAbs) if err != nil { - errs = append(errs, failure{filename: "(hierarchy)", message: err.Error()}) + errs = append(errs, failure{filename: "(hierarchy)", message: err.Error(), code: jsonout.CodeValidation}) } } } @@ -470,6 +509,11 @@ func reserveOne( // interrupted run leaves stubs where the old single-pass create left pages // missing entirely -- uglier, but every id is already persisted, so a plain // `markfluence update` finishes the job). +// +// What can still fail here is a server or network condition -- and only that. +// The conversion runs a second time, but it already ran in preflight against +// the same file, so a document defect never reaches this point; that is the +// residual S7 (no-partial-create) stays Partial for. func publishOne(r record, res *createResult, pageID string, version int, c *client.ConfluenceClient) *createResult { // SiteURL, not BaseURL: rewritten links are published into the page, so they // must point at the site even when requests go through the gateway. @@ -597,6 +641,28 @@ func resolveFile( return record{}, err } + // Convert, and keep nothing but the error. A defect the converter refuses -- + // two assets wanting one attachment name, say -- is a property of the file on + // disk, so asking here, before the reserve phase creates anything, is what + // keeps it from leaving a content-less page and a page_id the author has to + // undo by hand (#127). + // + // Last, after every server check, so the error precedence above is untouched: + // a page_id that is taken or broken stays the first thing reported about a + // file, which is what its own comment argues for. The cost is that a file + // that cannot convert still makes those requests first. + // + // The page is discarded rather than handed to publishOne, and its Broken and + // Warnings with it. This index has not been seeded with the batch's own page + // ids yet, so an in-set link renders unresolved here and resolves there -- + // reusing this result would publish the unresolved one, which is the ordering + // dependency _plans/026 removed. The *error* is the same in both phases, + // because no error path reads the index at all; that is pinned by + // TestErrorDoesNotDependOnTheIndex in internal/convert. + if _, err := convert.MdToConfluence(mf, root, index, c.SiteURL(), spaceKey, buildinfo.Stamp()); err != nil { + return record{}, &convertFailure{err: err} + } + return record{filename, abs, mf, title, spaceKey, spaceID, parent, width, root, index}, nil } diff --git a/cmd/create/json.go b/cmd/create/json.go index 1eb74fc..6db93a7 100644 --- a/cmd/create/json.go +++ b/cmd/create/json.go @@ -178,7 +178,7 @@ func abort(args []string, errs []failure, roots *project.Cache) error { for _, e := range errs { ui.Error(fmt.Sprintf("[%s] %s", e.filename, e.message)) } - ui.Error(fmt.Sprintf("Aborting: %d file(s) failed validation; nothing was created.", len(errs))) + ui.Error(fmt.Sprintf("Aborting: %d file(s) failed preflight; nothing was created.", len(errs))) return ui.ErrSilent } @@ -200,14 +200,14 @@ func abort(args []string, errs []failure, roots *project.Cache) error { failed := 0 for _, a := range args { if f, bad := errMap[a]; bad { - items = append(items, abortedResult(a, statusFailed, f, jsonout.CodeValidation)) + items = append(items, abortedResult(a, statusFailed, f)) failed++ } else { - items = append(items, abortedResult(a, statusNotCreated, failure{}, "")) + items = append(items, abortedResult(a, statusNotCreated, failure{})) } } for _, e := range extra { - items = append(items, abortedResult(e.filename, statusFailed, e, jsonout.CodeValidation)) + items = append(items, abortedResult(e.filename, statusFailed, e)) failed++ } @@ -225,8 +225,11 @@ func abort(args []string, errs []failure, roots *project.Cache) error { // // f is this file's failure, or the zero value for a file that was never reached // (status not_created). A page_id failure fills page_id -- the id in the file, the -// thing to go fix -- and, when a page is really at that id, url. -func abortedResult(file, status string, f failure, code jsonout.Code) jsonCreateResult { +// thing to go fix -- and, when a page is really at that id, url. The code comes +// from f rather than from a parameter, so it cannot disagree with the failure it +// describes: phase 1 rejects a file for more than one reason now, and only the +// failure knows which. +func abortedResult(file, status string, f failure) jsonCreateResult { res := jsonCreateResult{ OK: false, Status: status, @@ -241,7 +244,7 @@ func abortedResult(file, status string, f failure, code jsonout.Code) jsonCreate if f.message != "" { msg := f.message res.Error = &msg - c := code + c := f.code res.Code = &c } return res diff --git a/cmd/create/json_test.go b/cmd/create/json_test.go index 8ccac83..065f4b0 100644 --- a/cmd/create/json_test.go +++ b/cmd/create/json_test.go @@ -51,11 +51,11 @@ func TestSchemaConformance(t *testing.T) { // The phase-1 abort envelope, including the one failure that names a page: // a page_id already taken reports page_id and url on a result that is not ok. abortItems := []any{ - abortedResult("bad.md", statusFailed, failure{message: "no title given"}, jsonout.CodeValidation), + abortedResult("bad.md", statusFailed, + failure{message: "no title given", code: jsonout.CodeValidation}), abortedResult("taken.md", statusFailed, newFailure("taken.md", - pageIDFailureFor(testClient(), "123", &client.Page{ID: "123", Title: "Runbook"})), - jsonout.CodeValidation), - abortedResult("ok.md", statusNotCreated, failure{}, ""), + pageIDFailureFor(testClient(), "123", &client.Page{ID: "123", Title: "Runbook"}))), + abortedResult("ok.md", statusNotCreated, failure{}), } abortEnv := jsonout.NewEnvelope("create", abortItems, createSummary{Total: 3, Succeeded: 0, Failed: 2, Aborted: true}) @@ -173,13 +173,14 @@ func TestJSONResultDryRunInSetParent(t *testing.T) { func TestAbortedResultShapes(t *testing.T) { // A validation-failed file. - failed := abortedResult("bad.md", statusFailed, failure{message: "no title given"}, jsonout.CodeValidation) + failed := abortedResult("bad.md", statusFailed, + failure{message: "no title given", code: jsonout.CodeValidation}) if failed.OK || failed.Status != "failed" || failed.Error == nil || failed.Code == nil || *failed.Code != jsonout.CodeValidation { t.Errorf("failed abort result unexpected: %+v", failed) } // A file that simply wasn't created (batch aborted). - nc := abortedResult("ok.md", statusNotCreated, failure{}, "") + nc := abortedResult("ok.md", statusNotCreated, failure{}) if nc.OK || nc.Status != "not_created" || nc.Error != nil || nc.Code != nil { t.Errorf("not_created abort result unexpected: %+v", nc) } @@ -192,8 +193,7 @@ func TestAbortedResultShapes(t *testing.T) { // A page_id already taken: the result names the page in fields, not just prose. taken := abortedResult("taken.md", statusFailed, newFailure("taken.md", - pageIDFailureFor(testClient(), "123", &client.Page{ID: "123", Title: "Runbook"})), - jsonout.CodeValidation) + pageIDFailureFor(testClient(), "123", &client.Page{ID: "123", Title: "Runbook"}))) if taken.PageID == nil || *taken.PageID != "123" { t.Errorf("page_id = %v, want 123", taken.PageID) } @@ -204,7 +204,7 @@ func TestAbortedResultShapes(t *testing.T) { // A page_id that resolves to nothing: the id is reported, but there is no page // to link, so url stays null. missing := abortedResult("gone.md", statusFailed, newFailure("gone.md", - pageIDFailureFor(testClient(), "999", nil)), jsonout.CodeValidation) + pageIDFailureFor(testClient(), "999", nil))) if missing.PageID == nil || *missing.PageID != "999" { t.Errorf("page_id = %v, want 999", missing.PageID) } @@ -258,7 +258,8 @@ func TestAbortReportsRoots(t *testing.T) { } old := os.Stdout os.Stdout = w - err = abort([]string{"bad.md"}, []failure{{filename: "bad.md", message: "no title given"}}, roots) + bad := failure{filename: "bad.md", message: "no title given", code: jsonout.CodeValidation} + err = abort([]string{"bad.md"}, []failure{bad}, roots) os.Stdout = old if err := w.Close(); err != nil { t.Fatalf("Close: %v", err) diff --git a/cmd/create/run_test.go b/cmd/create/run_test.go index d62478d..38d97c6 100644 --- a/cmd/create/run_test.go +++ b/cmd/create/run_test.go @@ -3,6 +3,7 @@ package create import ( "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "os" @@ -13,8 +14,12 @@ import ( "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/project" + "github.com/mozilla/markfluence/internal/schematest" + "github.com/mozilla/markfluence/internal/ui" + "github.com/spf13/cobra" ) // fakeConfluence is a minimal in-memory double covering exactly what create's @@ -484,3 +489,174 @@ func TestCreateBatchIgnoresDirectoryNesting(t *testing.T) { } } } + +// testCmd builds a bare *cobra.Command carrying the flags run() reads itself, +// pointed at url. It doesn't go through the real root command tree, and +// CONFLUENCE_TOKEN (never a flag) comes from the environment instead, as it +// would in a real invocation. --root is pinned to the fixture directory so a +// fixture's images resolve inside a root the test chose, rather than whatever +// Discover's fallback happens to pick. +func testCmd(t *testing.T, url, root string) *cobra.Command { + t.Helper() + t.Setenv("CONFLUENCE_TOKEN", "t") + c := &cobra.Command{} + c.Flags().String("url", url, "") + c.Flags().String("username", "u", "") + c.Flags().String("cloud-id", "", "") + c.Flags().String("env-file", "", "") + c.Flags().String("root", root, "") + return c +} + +// writeCollidingImages plants the two assets whose base names agree, which is +// what MdToConfluence refuses: an attachment name is unique per page, so one +// upload would overwrite the other. +func writeCollidingImages(t *testing.T, dir string) { + t.Helper() + for _, rel := range []string{"arch/diagram.png", "deploy/diagram.png"} { + path := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("PNG"), 0o644); err != nil { + t.Fatal(err) + } + } +} + +const collidingBody = "---\ntitle: A\n---\n![arch](arch/diagram.png)\n\n![deploy](deploy/diagram.png)\n" + +// TestRunRefusesADocumentDefectBeforeCreatingAnything is #127: a defect the +// converter refuses used to surface in the publish phase, after the reserve +// phase had already created a page and written its id into the file -- leaving +// a content-less page, a page_id nobody asked for, and a re-run that failed +// with "a page already exists at page_id" instead. Preflight converts now, so +// the file is refused with nothing created and nothing written. +// +// The fake has no attachment support and errors on an unexpected request, so a +// fixture that reached the publish phase would fail here twice over. +func TestRunRefusesADocumentDefectBeforeCreatingAnything(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + writeCollidingImages(t, dir) + path := write(t, dir, "a.md", collidingBody) + + c, fake := newFakeConfluence(t) + err := run(testCmd(t, c.SiteURL(), dir), []string{path}) + + if err == nil { + t.Fatal("run should have failed: the file cannot convert") + } + if len(fake.pages) != 0 { + t.Errorf("pages created = %d, want 0 -- the defect is knowable before the reserve phase", len(fake.pages)) + } + raw, readErr := os.ReadFile(path) + if readErr != nil { + t.Fatal(readErr) + } + if strings.Contains(string(raw), "page_id") { + t.Errorf("a refused file must not gain a page_id:\n%s", raw) + } +} + +// TestRunConversionFailureAbortsTheWholeBatch is the behavioral change, not +// just the absence of a stub: a conversion failure goes through abort(), so a +// clean file sharing the batch is not created either. Reporting the bad file +// alone would leave the batch half-published, which is the state phase 1 +// exists to prevent. +func TestRunConversionFailureAbortsTheWholeBatch(t *testing.T) { + resetOpts(t) + dir := t.TempDir() + spaceOpt = "ENG" + writeCollidingImages(t, dir) + bad := write(t, dir, "bad.md", collidingBody) + good := write(t, dir, "good.md", "---\ntitle: Good\n---\nbody\n") + + c, fake := newFakeConfluence(t) + if err := run(testCmd(t, c.SiteURL(), dir), []string{bad, good}); err == nil { + t.Fatal("run should have failed") + } + + if len(fake.pages) != 0 { + t.Errorf("pages created = %d, want 0 -- one bad file aborts the batch", len(fake.pages)) + } + raw, err := os.ReadFile(good) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "page_id") { + t.Errorf("the clean file must be left alone when the batch aborts:\n%s", raw) + } +} + +// TestRunConversionFailureReportsCONVERT pins the code the failure carries. +// abort() used to hardcode VALIDATION for everything phase 1 rejected; this +// defect reported CONVERT from the publish phase before the check moved, and a +// --json consumer's distinction between "the document did not convert" and +// "the server or the frontmatter said no" has to survive the move. Asserted +// against a VALIDATION failure in the same envelope, since a code that is +// simply always CONVERT would pass the first half alone. +func TestRunConversionFailureReportsCONVERT(t *testing.T) { + resetOpts(t) + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + dir := t.TempDir() + spaceOpt = "ENG" + writeCollidingImages(t, dir) + bad := write(t, dir, "bad.md", collidingBody) + untitled := write(t, dir, "untitled.md", "---\ntitle: \"\"\n---\nbody\n") + + c, _ := newFakeConfluence(t) + out, runErr := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{bad, untitled}) + }) + if runErr == nil { + t.Fatal("run should have failed") + } + schematest.ValidateEnvelope(t, []byte(out)) + + var env struct { + Results []struct { + File string `json:"file"` + Code *string `json:"code"` + Error *string `json:"error"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("unmarshal %q: %v", out, err) + } + codes := map[string]string{} + for _, r := range env.Results { + if r.Code != nil { + codes[filepath.Base(r.File)] = *r.Code + } + } + if got := codes["bad.md"]; got != string(jsonout.CodeConvert) { + t.Errorf("bad.md code = %q, want CONVERT", got) + } + if got := codes["untitled.md"]; got != string(jsonout.CodeValidation) { + t.Errorf("untitled.md code = %q, want VALIDATION", got) + } +} + +// captureStdout runs fn with os.Stdout redirected, returning what it printed. +func captureStdout(t *testing.T, fn func() error) (string, error) { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + old := os.Stdout + os.Stdout = w + runErr := fn() + os.Stdout = old + if err := w.Close(); err != nil { + t.Fatal(err) + } + out, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + return string(out), runErr +} From 6f3f0fb4232dcc6154f431dc715ddf255935a6fc Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 18:06:41 -0400 Subject: [PATCH 4/6] docs: add S7 (no-partial-create), Partial The strongest safety property create has, written down now that #127 closed the part of it that was knowable from disk. Partial rather than Holds: a publish-phase server or network failure still leaves the reserved stub behind, which _plans/026 accepted deliberately, and closing that would mean deleting a page -- a change S4 does not authorise on its own. --- CLAUDE.md | 4 ++-- README.md | 16 ++++++++++++---- docs/guarantees.md | 31 ++++++++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 246fb14..547ee6a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,12 +45,12 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd **Before changing anything that talks to Confluence, read [docs/confluence/](docs/confluence/)** — what we established by experiment, since Atlassian documents little of it. Two traps recorded there have each already produced a confident wrong conclusion: `body-format=view` is not what the browser renders, and `body.storage` proves only what was stored, never what takes effect. -**[docs/guarantees.md](docs/guarantees.md) holds the properties markfluence holds itself to** — safety (S1-S6), laws (L1-L8), conformance (C1), reporting (R1-R2). Each carries a status, because several are aspirational rather than true today: a spec or PR cites them by id to say what it changes. The ids are permanent and never reused, and a change that downgrades a status says so in the commit message and in that file rather than letting it be noticed later. +**[docs/guarantees.md](docs/guarantees.md) holds the properties markfluence holds itself to** — safety (S1-S7), laws (L1-L8), conformance (C1), reporting (R1-R2). Each carries a status, because several are aspirational rather than true today: a spec or PR cites them by id to say what it changes. The ids are permanent and never reused, and a change that downgrades a status says so in the commit message and in that file rather than letting it be noticed later. ### Layout - `cmd/root.go` — the cobra root: `--url`/`--username`/`--debug`/`--no-color` persistent flags, version from `internal/buildinfo`, and registration of every subcommand. `Execute()` prints cobra-generated errors (bad args/flags) but not `ui.ErrSilent`, which marks a failure a command already reported. -- `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/{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 three-phase and transactional (preflight all, reserve parents-first in topological order, then publish); `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. Preflight also **converts every file and throws the page away**, keeping only the error (#127/S7): a defect the converter refuses is a property of the file on disk, so asking before the reserve phase is what keeps it from leaving a content-less page and a `page_id` the author has to undo by hand. The result cannot be reused by `publishOne` — reserve seeds the batch's ids into the shared link index in between, so an in-set link renders unresolved in preflight and resolves in publish — but the *error* is identical, because no error path in `MdToConfluence` reads the index (pinned by `TestErrorDoesNotDependOnTheIndex`); phase 1's `Broken`/`Warnings` are discarded for that same reason. It is called **last**, after every server check, so `page_id`-first precedence is untouched, and its failure carries `CodeConvert` on the `failure` struct rather than `abort()`'s old hardcoded `VALIDATION`. - `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`, plus everything real YAML now refuses — a nested value, a `|` block, a duplicate key, a tab indent, a reserved indicator), an invalid `page_width` (`pagewidth.Declared`), a present-but-non-numeric `page_id` (`pageref.IsDigits`), and a **present-but-empty `title`** — 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. `title` is the one exception to that reasoning and only in its present-but-empty form: `create` and `update` both reject it, so no verb makes it valid and there is no false positive to have. An *absent* title stays unreported, since `update` accepts one and keeps the live page's title. 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. **`--depth` exports a subtree** (`0` default / a number / `all`), **`--space KEY` a whole space** (requiring an explicit `--depth`, since the default would export nothing and defaulting to `all` would make a typo walk a whole space), and a **folder** may be the target — a folder and a space have no file of their own, so their children become the top level, which is why `layout`'s `rootRef` carries the id children hang off *separately* from whether anything is written for it: the walk's top-level nodes report a folder as their parent but report nothing for a space, and conflating the two placed every page at the destination root. `layout` owns every path an export writes (mirrored hierarchy: `.md` plus a `/` for children and unrecorded attachments, a folder as a bare directory) and the `-` suffix for a group of siblings that slug the same — applied to *every* member so a filename never depends on walk order, and disambiguating rather than refusing because a space nobody can retitle would otherwise be unexportable over a punctuation variant (an exported filename is ergonomic; identity is `page_id`, per L8). `parent:` is a relative path to the parent's own `.md` so the tree publishes into fresh pages, except for the export root and a page whose parent is a folder, which keep an id. Two things the layout buys that are easy to miss: page directories are unique, which is what makes page-scoped attachment placement collision-free — so `pagedoc.Placement` must carry the *disambiguated* directory (`AttachmentDirFor`), or two colliding siblings silently share one attachment file, which no checksum catches because a native attachment has none. `destClaims` reserves every page's destination before any attachment is written, since a recorded `path=` is server data that can name a page's own file and a parent's attachments are written before its children exist — otherwise the attachment lands first and the page is reported `skipped (exists)`. A page already on disk skips its *render* but not its attachment pass, so a retry resumes a run that died mid-download. `markfluence.yaml` is planted at `dest` for a multi-page export **before the first page**, because a partial tree with no marker republishes every shared asset as `IMAGE BROKEN`. Markdown only. An attachment with a recorded `path=` lands there; one without is page-scoped. Still no `--attachments-dir`, but for a different reason than before the naming change: moving an asset no longer renames its attachment, so it is no longer unsafe — it would simply reintroduce the collision a base name has to refuse, since two pages' `diagram.png` cannot share a directory. 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. **`--space KEY` lists a whole space instead of a page** (#98), which makes `PAGE` optional — exactly one of the two, checked before credentials. Depth 1 is then the space's **root pages**, not the homepage's children: a space can have several roots (`create` with a null parent makes one), so seeding the walk from `homepageId` would drop a root and its whole subtree, and there is no root-level *folder* to miss because a folder created with no parent lands under the homepage ([docs/confluence/spaces.md](docs/confluence/spaces.md)). A root row's `parent_id` is `null` — the one place `childrenResult` needs `stringOrNull` for it — since a space is not a node. The key is resolved through `ResolveSpaceID` before the walk even though the v1 route it feeds takes a key: an unknown key must fail as a typo (exit 2) the way it does for `find`/`search`, and the v1 route reports one as a 404, which is also what a rejected credential looks like. Because a space's top level is usually one row, human output adds a `--depth` reminder when `--depth` was left at its default — on **stderr**, via `ui.Hint`, so the table stays pipeable; `--json` never sees it. A failing space walk likewise reports an `errorObject` on stderr rather than a `results[0]` failure, since `SingleOpFailure.page_id` would otherwise carry a space key. diff --git a/README.md b/README.md index 6a3b840..4eaa687 100644 --- a/README.md +++ b/README.md @@ -240,9 +240,10 @@ content type — in which case give its id the same way you would a page's. Page width defaults to `max`; set it with `--page-width narrow|wide|max` (which overrides the frontmatter `page_width` and may apply across a batch). -All files are validated first — if any would fail (a problem with its `page_id`, a -title clash in the space, an unresolvable parent), nothing is created. Both kinds -of clash name the page in the way, so you can go look at it: +All files are checked first — if any would fail (a problem with its `page_id`, a +title clash in the space, an unresolvable parent, or markdown the converter +refuses), nothing is created. Both kinds of clash name the page in the way, so +you can go look at it: ```console $ markfluence create docs/runbook.md @@ -270,6 +271,13 @@ other. A run interrupted after this point leaves a permanent, empty page version behind rather than no page at all; every id is already persisted (unless `--no-persist`), so a plain `update` finishes the job. +The preflight phase converts each file too, keeping only the answer to "can this +convert at all?" — so markdown the converter refuses (two images in one document +whose file names match, say) aborts the batch instead of leaving an empty page +and a `page_id` behind. What can still leave a stub is a server or network +failure while publishing, which no local check could have predicted; see S7 in +[docs/guarantees.md](docs/guarantees.md). + `--dry-run` validates every file (the same checks a real run makes, so it exits non-zero on the same failures) and previews what would be created — pages, attachment uploads, page widths, and frontmatter write-backs — without writing to @@ -919,7 +927,7 @@ Notes on the schema: 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) +- **`create`'s preflight abort** (any file failing means nothing is created) lists every input file — failed ones with an `error`, the rest as `not_created` — and sets `summary.aborted: true`. - **Warnings and broken image/link notices** are data (`warnings`/`broken` diff --git a/docs/guarantees.md b/docs/guarantees.md index e158364..4f42bdc 100644 --- a/docs/guarantees.md +++ b/docs/guarantees.md @@ -52,6 +52,7 @@ A violation here does damage, rather than producing a wrong answer. | **S4** | `no-removal-as-side-effect` | Nothing is removed as a side effect. Removal is a command's stated purpose or it does not happen. | Vacuous | | **S5** | `remove-only-ours` | markfluence removes only what markfluence created. | Vacuous | | **S6** | `removal-is-previewable` | A command that removes says what it will remove before doing it, and honours `--dry-run`. | Vacuous | +| **S7** | `no-partial-create` | A file `create` fails leaves no page behind. | Partial | **S1** is enforced by `attachfile.Resolve`, which refuses a traversing path rather than clipping it. @@ -115,6 +116,34 @@ is already visible on the horizon in two places: only reported, by `attachment-list`. It is what lets a prune remove stranded markfluence attachments while never touching a file someone attached by hand. +### S7 and the stub create leaves behind + +**S7** is **Partial**, and the boundary is worth stating exactly, because the +gap is not the part that looks alarming. + +`create` is three-phase: preflight validates every file, reserve creates a +content-less page for each and persists its `page_id`, publish converts and +fills each page in. Anything preflight rejects aborts the batch with nothing +created, and since #127 that includes **every failure knowable from the files on +disk** — preflight converts each file and keeps the error, so a document the +converter refuses (two assets wanting one attachment name, say) never reaches +the reserve phase. It used to, which is what made the guarantee worth writing: +the author was left with a content-less page, a `page_id` they did not ask for, +and a re-run that refused because a page was already at that id. + +What remains is a **server or network failure in the publish phase**, which +leaves the reserved stub behind. That is deliberate rather than unaddressed: +`_plans/026` accepted it as the price of reserving every id before converting +anything, which is what stopped link resolution depending on creation order. +The stub is not lost work — its id is already in the frontmatter, so a plain +`markfluence update` finishes publishing it — but a page exists that the +command reported as failed, so the guarantee does not hold as written. + +Closing it would mean deleting the stub, and that is not a change this +guarantee can authorise on its own: it would make **S4** and **S5** +non-vacuous, and S4 says removal is a command's stated purpose or it does not +happen. + ## Laws Algebraic properties of the three mappings markfluence performs — **Resolve** (a @@ -388,7 +417,7 @@ would write *something*, under a name nobody chose. | kind | verified by | |---|---| -| Safety | adversarial tests: traversal attempts, pre-existing files | +| Safety | adversarial tests: traversal attempts, pre-existing files, a failing preflight asserted to have created nothing | | Laws | property tests: generate trees, assert the equation | | Conformance | C1: fixtures checked against what a Markdown preview renders. C2: the writer verifies its own output at runtime, plus a round-trip test and fuzz target; agreement with *other* YAML implementations is review judgement | | Reporting | example tests asserting a specific message appears | From 34fe0b59fb109ed55b9fd096068113c9e0cbef8f Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 18:17:09 -0400 Subject: [PATCH 5/6] docs: correct three overstated claims from review - publishOne can fail locally, not only remotely: SyncAttachments opens every asset to checksum and upload it, which the converter never does. So can reserveOne's frontmatter write, which runs after CreatePage and leaves a stub whose id is *not* persisted -- the one case update cannot pick up. S7 now names all three residuals instead of one. - "No error path reads the index" was stronger than what holds. Whether renderImage runs does depend on it (renderLink skips a broken link's children, and Broken comes from FileExists). What holds is that reserve only calls SetPage, which writes idx.pages alone, while FileExists/Anchor read idx.anchors, fixed at Build time. - The end-to-end test's comment credited the fake's unexpected-request guard, which cannot fire: a revert fails at MdToConfluence, before SyncAttachments. Also: validationFailure replaces the two bare failure literals so nothing sets code by hand; --help says "checked" to match the abort line; the README notes that one bad file aborts a --dry-run preview of the whole batch. --- CLAUDE.md | 2 +- README.md | 13 +++-- _plans/034_create-preflight-conversion.md | 66 ++++++++++++++--------- cmd/create/create.go | 47 ++++++++++------ cmd/create/run_test.go | 6 ++- docs/guarantees.md | 44 ++++++++++----- internal/convert/errorindex_test.go | 14 +++-- 7 files changed, 123 insertions(+), 69 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 547ee6a..9df7f67 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -50,7 +50,7 @@ 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,check,info,read,export,children,find,search}/` — one package per command (each exports `Cmd`), orchestrating the `internal` packages and `internal/ui` output. `create` is three-phase and transactional (preflight all, reserve parents-first in topological order, then publish); `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. Preflight also **converts every file and throws the page away**, keeping only the error (#127/S7): a defect the converter refuses is a property of the file on disk, so asking before the reserve phase is what keeps it from leaving a content-less page and a `page_id` the author has to undo by hand. The result cannot be reused by `publishOne` — reserve seeds the batch's ids into the shared link index in between, so an in-set link renders unresolved in preflight and resolves in publish — but the *error* is identical, because no error path in `MdToConfluence` reads the index (pinned by `TestErrorDoesNotDependOnTheIndex`); phase 1's `Broken`/`Warnings` are discarded for that same reason. It is called **last**, after every server check, so `page_id`-first precedence is untouched, and its failure carries `CodeConvert` on the `failure` struct rather than `abort()`'s old hardcoded `VALIDATION`. +- `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 three-phase and transactional (preflight all, reserve parents-first in topological order, then publish); `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. Preflight also **converts every file and throws the page away**, keeping only the error (#127/S7): a defect the converter refuses is a property of the file on disk, so asking before the reserve phase is what keeps it from leaving a content-less page and a `page_id` the author has to undo by hand. The result cannot be reused by `publishOne` — reserve seeds the batch's ids into the shared link index in between, so an in-set link renders unresolved in preflight and resolves in publish, and phase 1's `Broken`/`Warnings` are discarded for that reason. The *error* is identical across the two, for a narrower reason than "the converter ignores the index": whether `renderImage` runs at all does depend on it (`renderLink` skips a broken link's children, and `Broken` is decided by `FileExists`), but reserve only calls `SetPage`, which writes `idx.pages` alone — nothing there can raise an error or change one's text, and `FileExists`/`Anchor` read `idx.anchors`, fixed at `Build` time. Making `SetPage` also mark a file as existing would break it; pinned by `TestErrorDoesNotDependOnTheIndex`. It is called **last**, after every server check, so `page_id`-first precedence is untouched, and its failure carries `CodeConvert` on the `failure` struct rather than `abort()`'s old hardcoded `VALIDATION`. - `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`, plus everything real YAML now refuses — a nested value, a `|` block, a duplicate key, a tab indent, a reserved indicator), an invalid `page_width` (`pagewidth.Declared`), a present-but-non-numeric `page_id` (`pageref.IsDigits`), and a **present-but-empty `title`** — 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. `title` is the one exception to that reasoning and only in its present-but-empty form: `create` and `update` both reject it, so no verb makes it valid and there is no false positive to have. An *absent* title stays unreported, since `update` accepts one and keeps the live page's title. 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. **`--depth` exports a subtree** (`0` default / a number / `all`), **`--space KEY` a whole space** (requiring an explicit `--depth`, since the default would export nothing and defaulting to `all` would make a typo walk a whole space), and a **folder** may be the target — a folder and a space have no file of their own, so their children become the top level, which is why `layout`'s `rootRef` carries the id children hang off *separately* from whether anything is written for it: the walk's top-level nodes report a folder as their parent but report nothing for a space, and conflating the two placed every page at the destination root. `layout` owns every path an export writes (mirrored hierarchy: `.md` plus a `/` for children and unrecorded attachments, a folder as a bare directory) and the `-` suffix for a group of siblings that slug the same — applied to *every* member so a filename never depends on walk order, and disambiguating rather than refusing because a space nobody can retitle would otherwise be unexportable over a punctuation variant (an exported filename is ergonomic; identity is `page_id`, per L8). `parent:` is a relative path to the parent's own `.md` so the tree publishes into fresh pages, except for the export root and a page whose parent is a folder, which keep an id. Two things the layout buys that are easy to miss: page directories are unique, which is what makes page-scoped attachment placement collision-free — so `pagedoc.Placement` must carry the *disambiguated* directory (`AttachmentDirFor`), or two colliding siblings silently share one attachment file, which no checksum catches because a native attachment has none. `destClaims` reserves every page's destination before any attachment is written, since a recorded `path=` is server data that can name a page's own file and a parent's attachments are written before its children exist — otherwise the attachment lands first and the page is reported `skipped (exists)`. A page already on disk skips its *render* but not its attachment pass, so a retry resumes a run that died mid-download. `markfluence.yaml` is planted at `dest` for a multi-page export **before the first page**, because a partial tree with no marker republishes every shared asset as `IMAGE BROKEN`. Markdown only. An attachment with a recorded `path=` lands there; one without is page-scoped. Still no `--attachments-dir`, but for a different reason than before the naming change: moving an asset no longer renames its attachment, so it is no longer unsafe — it would simply reintroduce the collision a base name has to refuse, since two pages' `diagram.png` cannot share a directory. 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. **`--space KEY` lists a whole space instead of a page** (#98), which makes `PAGE` optional — exactly one of the two, checked before credentials. Depth 1 is then the space's **root pages**, not the homepage's children: a space can have several roots (`create` with a null parent makes one), so seeding the walk from `homepageId` would drop a root and its whole subtree, and there is no root-level *folder* to miss because a folder created with no parent lands under the homepage ([docs/confluence/spaces.md](docs/confluence/spaces.md)). A root row's `parent_id` is `null` — the one place `childrenResult` needs `stringOrNull` for it — since a space is not a node. The key is resolved through `ResolveSpaceID` before the walk even though the v1 route it feeds takes a key: an unknown key must fail as a typo (exit 2) the way it does for `find`/`search`, and the v1 route reports one as a 404, which is also what a rejected credential looks like. Because a space's top level is usually one row, human output adds a `--depth` reminder when `--depth` was left at its default — on **stderr**, via `ui.Hint`, so the table stays pipeable; `--json` never sees it. A failing space walk likewise reports an `errorObject` on stderr rather than a `results[0]` failure, since `SingleOpFailure.page_id` would otherwise carry a space key. diff --git a/README.md b/README.md index 4eaa687..35f80de 100644 --- a/README.md +++ b/README.md @@ -274,14 +274,17 @@ behind rather than no page at all; every id is already persisted (unless The preflight phase converts each file too, keeping only the answer to "can this convert at all?" — so markdown the converter refuses (two images in one document whose file names match, say) aborts the batch instead of leaving an empty page -and a `page_id` behind. What can still leave a stub is a server or network -failure while publishing, which no local check could have predicted; see S7 in -[docs/guarantees.md](docs/guarantees.md). +and a `page_id` behind. A stub can still be left by a failure while publishing: +a server or network error, an attachment that turns out to be unreadable, or a +frontmatter file that can't be written. See S7 in +[docs/guarantees.md](docs/guarantees.md), which names all three. -`--dry-run` validates every file (the same checks a real run makes, so it exits +`--dry-run` checks every file (the same checks a real run makes, so it exits non-zero on the same failures) and previews what would be created — pages, attachment uploads, page widths, and frontmatter write-backs — without writing to -Confluence or to any file. Because nothing is created, a previewed page has no id +Confluence or to any file. Because it makes the same checks, one unpublishable +file aborts the preview for the whole batch rather than previewing the rest; to +lint several files independently, use [`check`](#check) instead. Because nothing is created, a previewed page has no id or URL yet; an in-set child's `parent` is unresolved, but its source file is reported in the `parent_file` output field (present in every run, in `--json`). diff --git a/_plans/034_create-preflight-conversion.md b/_plans/034_create-preflight-conversion.md index d054186..a0871dc 100644 --- a/_plans/034_create-preflight-conversion.md +++ b/_plans/034_create-preflight-conversion.md @@ -45,12 +45,19 @@ regression -- exactly the ordering dependency `_plans/026` removed. The extra cost is one local conversion per file over bytes already in memory: no network, no I/O. -**The converse is what makes the preflight sound: no error path reads the -index.** `MdToConfluence` returns exactly two errors -- `NameCollisionError` -(images.go:121) and goldmark's own `Convert` failure (convert.go:103) -- and -neither consults `index`. So phase 1's verdict is neither a false positive nor a -false negative against phase 3's, and the error text is identical. That is the -invariant the whole design rests on, so it gets a test rather than only a +**The converse is what makes the preflight sound, and the reason is narrower +than "the converter ignores the index".** `MdToConfluence` returns exactly two +errors -- `NameCollisionError` (images.go:121) and goldmark's own `Convert` +failure (convert.go:103). Neither reads `index` directly, but whether +`renderImage` is *reached* does depend on it: `renderLink` returns +`WalkSkipChildren` for a Broken target, and Broken is decided by +`index.FileExists`. What actually holds is that reserve only ever calls +`SetPage`, which writes `idx.pages` alone -- nothing in `idx.pages` can raise +an error or change one's text -- while `FileExists` and `Anchor` read +`idx.anchors`, fixed at `Build` time and identical in both phases. So phase 1's +verdict is neither a false positive nor a false negative against phase 3's, and +the error text is identical. A change making `SetPage` also mark a file as +existing would break this, which is why it gets a test rather than only a comment. **Phase 1's `Broken` and `Warnings` are discarded.** They are *wrong* for any @@ -83,7 +90,10 @@ defect reports `CodeConvert` from phase 3 today, so keeping it means a `--json` consumer's distinction between "the document did not convert" and "the server or the frontmatter said no" survives the move. `abort()` currently hardcodes `jsonout.CodeValidation` (json.go:203, json.go:210); `failure` gains a `code` -field instead. The schema needs no change: its `code` enum is global (v1.json:159) +field instead. Note what this does *not* buy: `newFailure` still defaults an +HTTP error from the server checks to VALIDATION, so the distinction is between +"did not convert" and "everything else", not a full code taxonomy -- see Out of +scope. The schema needs no change: its `code` enum is global (v1.json:159) and already contains both, and nothing in the `create` branch constrains which appears. @@ -99,7 +109,7 @@ converts in `publishOne` today, so a colliding file currently prints a per-file which is right, because a dry-run's job is to predict the real run, and the real run now aborts. This is a behavior change to note, not a regression. -**Scope is the conversion only.** Two adjacent things stay out, below. +**Scope is the conversion only.** Three adjacent things stay out, below. ## Implementation @@ -112,15 +122,6 @@ run now aborts. This is a behavior change to note, not a regression. - `resolveFile` -- after `checkTitleFree`, before building the `record`: ```go - // Convert now, discarding everything but the error. A defect the converter - // finds is a property of the file on disk, so asking here -- before phase 2 - // creates anything -- is what keeps #127's stub from being made at all. - // - // The page is thrown away rather than reused by publishOne, and its Broken - // and Warnings with it: this index has not been seeded with the batch's own - // page ids yet, so an in-set link renders unresolved here and resolves - // there. The *error* is the same either way -- neither NameCollisionError - // nor goldmark's Convert failure reads the index at all. if _, err := convert.MdToConfluence( mf, root, index, c.SiteURL(), spaceKey, buildinfo.Stamp(), ); err != nil { @@ -128,15 +129,20 @@ run now aborts. This is a behavior change to note, not a regression. } ``` -- `failure` gains `code jsonout.Code`. + carrying a comment that states why the page is discarded and why the error + survives the phase boundary -- the two paragraphs under Decisions above. + +- `failure` gains `code jsonout.Code`, and `validationFailure(filename, message)` + is the constructor for a failure with no error value behind it (the two run() + diagnoses itself). Between it and `newFailure`, nothing sets `code` by hand, + which is what keeps a forgotten one from emitting `""` into a field whose enum + does not contain it. - `newFailure` sets `code: jsonout.CodeValidation` by default and `jsonout.CodeConvert` when `errors.As(err, &cf)` matches a `*convertFailure`, alongside the `pageIDFailure` field-carrying it already does. - The two bare `failure{...}` literals in `run` -- "parent page is not in the - target space" and the `"(hierarchy)"` topo-sort failure -- set - `code: jsonout.CodeValidation` explicitly. They must: `abortedResult` only - emits `code` when `message` is non-empty, so a zero-value `code` would put - `""` into a field whose enum does not contain it. + target space" and the `"(hierarchy)"` topo-sort failure -- go through + `validationFailure`. - `publishOne`'s doc comment: keep the stub-is-permanent paragraph, and add that a conversion failure no longer reaches it -- what survives here is a server or network condition no local check could have predicted (S7). @@ -167,9 +173,9 @@ created nothing. - **End-to-end, `cmd/create/run_test.go`**: a fixture referencing `arch/diagram.png` and `deploy/diagram.png` through the existing `fakeConfluence`. Assert the batch aborted, `f.pages` is empty, and the file's - frontmatter still has no `page_id`. The fake's own constraint helps here -- - it has no attachment support and `default`s to `t.Errorf("unexpected - request")`, so a fixture that reached phase 3 fails loudly on its own. + frontmatter still has no `page_id`. Not asserted on the fake refusing an + attachment request: reverting the fix fails the file at `MdToConfluence` + inside phase 3, which is before `SyncAttachments`, so the fake never sees one. - **The batch is refused, not just the bad file**: two files, one colliding. The clean one reports `not_created`, and no page exists for it either. This pins the actual behavioral change -- a conversion failure goes through @@ -203,6 +209,14 @@ created nothing. open. Checking it in phase 1 duplicates work the upload does anyway and races the filesystem -- the check passes and the upload still fails -- and unlike the converter it is not network-free, since it needs the page's existing - attachments. + attachments. It is therefore one of S7's three named residuals, not something + this plan closes; anything claiming publish can only fail remotely is wrong. + +- **Routing a preflight HTTP error through `jsonout.CodeFor`.** `newFailure` + defaults to VALIDATION, so a rejected credential or a 5xx from `checkPageID`, + `ResolveSpaceID`, `checkTitleFree` or `checkParentInSpace` still reports + VALIDATION rather than AUTH/NETWORK/API. Pre-existing, and a real improvement + now that `failure` carries a code at all -- but it changes the code on + failures this issue is not about, so it wants its own decision. - **`update`.** It has no reserve phase, so a conversion failure already fails the file with nothing created. Nothing to fix. diff --git a/cmd/create/create.go b/cmd/create/create.go index 25aa750..85926ab 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -14,8 +14,8 @@ // is wanted: a document the converter refuses is refused before anything is // created, rather than after the reserve phase has already made a page and put // its id in the file (#127). The result cannot be reused by publish, because -// reserve seeds the batch's page ids into the shared link index in between -- -// see resolveFile. +// reserve seeds the batch's page ids into the shared link index in between; why +// the error survives that and the page does not is in resolveFile. package create import ( @@ -54,7 +54,8 @@ var Cmd = &cobra.Command{ Use: "create FILE...", Short: "Create new Confluence pages from markdown files", Long: "Create new Confluence pages from markdown FILEs.\n\n" + - "All files are validated first; if any would fail, nothing is created.\n" + + "Every file is checked first -- including converting it -- and if any would\n" + + "fail, nothing is created.\n" + "Otherwise a content-less stub is reserved for each, parents-first, before\n" + "any of them is converted -- so a link between two files in the same batch\n" + "resolves regardless of which direction it points, or whether the two link\n" + @@ -138,8 +139,9 @@ type record struct { // used to hardcode VALIDATION for everything phase 1 rejected. It stopped being // true once phase 1 started converting: a conversion failure reports CONVERT, // the same code it reported from phase 3 before the check moved (see -// convertFailure). Every literal building one of these must set code, since -// abortedResult emits whatever is here and "" is not in the schema's enum. +// convertFailure). Build one only through newFailure or validationFailure, both +// of which set code -- abortedResult emits whatever is here whenever message is +// non-empty, and "" is not in the schema's enum. type failure struct { filename, message string pageID, url string @@ -177,6 +179,13 @@ type convertFailure struct{ err error } func (e *convertFailure) Error() string { return e.err.Error() } func (e *convertFailure) Unwrap() error { return e.err } +// validationFailure records a phase-1 failure that has no error value behind it +// -- one run() diagnoses itself, over the batch rather than over a single file. +// It exists so nothing has to remember to set code by hand. +func validationFailure(filename, message string) failure { + return failure{filename: filename, message: message, code: jsonout.CodeValidation} +} + // newFailure records a phase-1 error against a file, carrying over the fields of // a page_id failure so abort() can report them without re-fetching anything, // and the code the error's type implies. @@ -323,17 +332,13 @@ func run(cmd *cobra.Command, args []string) error { } for _, r := range records { if r.parent.kind == parentInSet && byAbs[r.parent.abs].spaceID != r.spaceID { - errs = append(errs, failure{ - filename: r.filename, - message: "parent page is not in the target space", - code: jsonout.CodeValidation, - }) + errs = append(errs, validationFailure(r.filename, "parent page is not in the target space")) } } if len(errs) == 0 { ordered, err = topoSort(records, byAbs) if err != nil { - errs = append(errs, failure{filename: "(hierarchy)", message: err.Error(), code: jsonout.CodeValidation}) + errs = append(errs, validationFailure("(hierarchy)", err.Error())) } } } @@ -510,10 +515,12 @@ func reserveOne( // missing entirely -- uglier, but every id is already persisted, so a plain // `markfluence update` finishes the job). // -// What can still fail here is a server or network condition -- and only that. // The conversion runs a second time, but it already ran in preflight against -// the same file, so a document defect never reaches this point; that is the -// residual S7 (no-partial-create) stays Partial for. +// the same file, so a document defect never reaches this point. What can still +// fail here is a server or network condition -- or a local read the converter +// never made: SyncAttachments opens every asset to checksum and upload it, so +// an image that Lstat'd fine in preflight can still be unreadable now. Those +// are the residuals S7 (no-partial-create) stays Partial for. func publishOne(r record, res *createResult, pageID string, version int, c *client.ConfluenceClient) *createResult { // SiteURL, not BaseURL: rewritten links are published into the page, so they // must point at the site even when requests go through the gateway. @@ -656,8 +663,16 @@ func resolveFile( // Warnings with it. This index has not been seeded with the batch's own page // ids yet, so an in-set link renders unresolved here and resolves there -- // reusing this result would publish the unresolved one, which is the ordering - // dependency _plans/026 removed. The *error* is the same in both phases, - // because no error path reads the index at all; that is pinned by + // dependency _plans/026 removed. + // + // The *error* is the same in both phases, and the reason is narrower than + // "the converter ignores the index": whether renderImage runs at all does + // depend on it, since renderLink skips a broken link's children and Broken + // is decided by index.FileExists. What holds is that reserve only ever calls + // SetPage, which writes idx.pages alone -- and nothing in idx.pages can + // raise an error or change one's text, while FileExists and Anchor read + // idx.anchors, fixed at Build time and identical in both phases. A change + // making SetPage also mark a file as existing would break this. Pinned by // TestErrorDoesNotDependOnTheIndex in internal/convert. if _, err := convert.MdToConfluence(mf, root, index, c.SiteURL(), spaceKey, buildinfo.Stamp()); err != nil { return record{}, &convertFailure{err: err} diff --git a/cmd/create/run_test.go b/cmd/create/run_test.go index 38d97c6..b545fb6 100644 --- a/cmd/create/run_test.go +++ b/cmd/create/run_test.go @@ -533,8 +533,10 @@ const collidingBody = "---\ntitle: A\n---\n![arch](arch/diagram.png)\n\n![deploy // with "a page already exists at page_id" instead. Preflight converts now, so // the file is refused with nothing created and nothing written. // -// The fake has no attachment support and errors on an unexpected request, so a -// fixture that reached the publish phase would fail here twice over. +// Asserted on the page count and the file, not on the fake refusing an +// attachment request: reverting the fix fails this file at MdToConfluence in +// the publish phase, which is before SyncAttachments, so the fake never sees +// one. func TestRunRefusesADocumentDefectBeforeCreatingAnything(t *testing.T) { resetOpts(t) dir := t.TempDir() diff --git a/docs/guarantees.md b/docs/guarantees.md index 4f42bdc..dcee15c 100644 --- a/docs/guarantees.md +++ b/docs/guarantees.md @@ -52,7 +52,7 @@ A violation here does damage, rather than producing a wrong answer. | **S4** | `no-removal-as-side-effect` | Nothing is removed as a side effect. Removal is a command's stated purpose or it does not happen. | Vacuous | | **S5** | `remove-only-ours` | markfluence removes only what markfluence created. | Vacuous | | **S6** | `removal-is-previewable` | A command that removes says what it will remove before doing it, and honours `--dry-run`. | Vacuous | -| **S7** | `no-partial-create` | A file `create` fails leaves no page behind. | Partial | +| **S7** | `no-partial-create` | A file that `create` fails to publish leaves no page behind. | Partial | **S1** is enforced by `attachfile.Resolve`, which refuses a traversing path rather than clipping it. @@ -124,19 +124,35 @@ gap is not the part that looks alarming. `create` is three-phase: preflight validates every file, reserve creates a content-less page for each and persists its `page_id`, publish converts and fills each page in. Anything preflight rejects aborts the batch with nothing -created, and since #127 that includes **every failure knowable from the files on -disk** — preflight converts each file and keeps the error, so a document the -converter refuses (two assets wanting one attachment name, say) never reaches -the reserve phase. It used to, which is what made the guarantee worth writing: -the author was left with a content-less page, a `page_id` they did not ask for, -and a re-run that refused because a page was already at that id. - -What remains is a **server or network failure in the publish phase**, which -leaves the reserved stub behind. That is deliberate rather than unaddressed: -`_plans/026` accepted it as the price of reserving every id before converting -anything, which is what stopped link resolution depending on creation order. -The stub is not lost work — its id is already in the frontmatter, so a plain -`markfluence update` finishes publishing it — but a page exists that the +created, and since #127 that includes **every defect the converter can find in +the files on disk** — preflight converts each file and keeps the error, so a +document the converter refuses (two assets wanting one attachment name, say) +never reaches the reserve phase. It used to, which is what made the guarantee +worth writing: the author was left with a content-less page, a `page_id` they +did not ask for, and a re-run that refused because a page was already at that +id. + +Three residuals remain, and only the first is purely remote: + +- **A server or network failure while publishing.** The stub stays, with its + `page_id` already in the frontmatter, so a plain `markfluence update` + finishes publishing it. +- **An attachment that cannot be read.** `client.SyncAttachments` opens every + asset to checksum and upload it, which the converter never does — it only + `Lstat`s. So an unreadable image (mode `000`, a file replaced between the two + steps) fails in the publish phase, locally, after the stub exists. Deliberately + not pre-flighted: it duplicates the read the upload makes anyway and races the + filesystem, so the check can pass and the upload still fail. +- **A frontmatter file that cannot be written.** `reserveOne` calls + `os.WriteFile` *after* `CreatePage`, so a read-only `.md` leaves a stub whose + id is **not** persisted — the one case where `markfluence update` cannot pick + the work up, since nothing on disk names the page. `failKeepingPage` keeps the + id and URL in the result for exactly this reason: the run's own output is the + only remaining trace. + +The first is deliberate rather than unaddressed: `_plans/026` accepted it as the +price of reserving every id before converting anything, which is what stopped +link resolution depending on creation order. In all three a page exists that the command reported as failed, so the guarantee does not hold as written. Closing it would mean deleting the stub, and that is not a change this diff --git a/internal/convert/errorindex_test.go b/internal/convert/errorindex_test.go index 8b02855..5784c23 100644 --- a/internal/convert/errorindex_test.go +++ b/internal/convert/errorindex_test.go @@ -7,11 +7,15 @@ package convert_test // create is three-phase (_plans/034). Preflight validates every file, reserve // creates a content-less page for each and seeds its id into the shared index, // publish converts and fills each page in. Preflight converts too, purely to -// learn whether the file can convert at all -- which is only sound because -// neither NameCollisionError (images.go) nor goldmark's own failure reads the -// index, so the verdict cannot differ between the two phases. It is also why -// the preflight result is *discarded* rather than reused by publish: the HTML -// does differ, since an in-set link resolves only once the id is there. +// learn whether the file can convert at all. That is sound for a narrower +// reason than "the converter ignores the index": whether renderImage runs at +// all does depend on it, since renderLink skips a broken link's children and +// Broken is decided by FileExists. What holds is that reserve only calls +// SetPage, which writes idx.pages alone -- nothing there can raise an error or +// change one's text, and FileExists/Anchor read idx.anchors, fixed at Build +// time and the same in both phases. It is also why the preflight result is +// *discarded* rather than reused by publish: the HTML does differ, since an +// in-set link resolves only once the id is there. // // Both halves are asserted together on purpose. Either alone can pass // vacuously -- "the errors match" proves nothing if the seeding never mattered. From 2356b3da16bde29a9b2f1c2c6f4b17008d79df83 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sun, 6 Sep 2026 18:19:16 -0400 Subject: [PATCH 6/6] docs: cross-link the preflight error-code issue _plans/034's Out of scope now cites #133, with the trap the fix has to avoid: CodeFor alone answers NETWORK for any non-HTTPError. --- _plans/034_create-preflight-conversion.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/_plans/034_create-preflight-conversion.md b/_plans/034_create-preflight-conversion.md index a0871dc..fda4b07 100644 --- a/_plans/034_create-preflight-conversion.md +++ b/_plans/034_create-preflight-conversion.md @@ -212,11 +212,14 @@ created nothing. attachments. It is therefore one of S7's three named residuals, not something this plan closes; anything claiming publish can only fail remotely is wrong. -- **Routing a preflight HTTP error through `jsonout.CodeFor`.** `newFailure` +- **Routing a preflight HTTP error through `jsonout.CodeFor`** (#133). `newFailure` defaults to VALIDATION, so a rejected credential or a 5xx from `checkPageID`, `ResolveSpaceID`, `checkTitleFree` or `checkParentInSpace` still reports VALIDATION rather than AUTH/NETWORK/API. Pre-existing, and a real improvement now that `failure` carries a code at all -- but it changes the code on - failures this issue is not about, so it wants its own decision. + failures this issue is not about, so it wants its own decision. `cmd/fix`'s + `locateCode` is the rule to copy, and `CodeFor` alone is not: it answers + NETWORK for any non-`HTTPError`, so `no title given` would become a network + problem. - **`update`.** It has no reserve phase, so a conversion failure already fails the file with nothing created. Nothing to fix.