From a81bbe36912b899c607408088395118163f3b7c1 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 11:27:35 -0400 Subject: [PATCH 01/22] docs: fix stale and inaccurate claims in README Verified every documented command, flag, and example against the current implementation. Corrects the scopes table (read:page:confluence, read:folder:confluence, and read:confluence-content.summary all over/under-claimed which commands need them), a stale error-message quote, two example outputs that no longer match real formatting/sorting, the --json envelope example (missing roots/dry_run), a stale schema command list, an outdated lossy-transforms claim for read (internal links and image paths now round-trip), a missing error-output prefix, and an overstated PowerShell completions claim. --- README.md | 49 +++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 1075da3..f46e35c 100644 --- a/README.md +++ b/README.md @@ -24,9 +24,10 @@ TBD — published to a tap on the first release. ### Shell completions markfluence generates its own completion scripts for bash, zsh, fish, and -PowerShell. The release archives ship them under `completions/`, and a Homebrew -install puts them where each shell looks, so a `brew install` needs nothing -further. +PowerShell. The release archives ship the bash/zsh/fish scripts under +`completions/`, and a Homebrew install puts them where each shell looks, so a +`brew install` needs nothing further; PowerShell isn't packaged and is +generated on demand instead (below). Otherwise, to load them into the current shell: @@ -110,15 +111,15 @@ The scopes markfluence needs: | Used for | Commands | Scope | | --- | --- | --- | -| Reading pages, and reading/writing page width | all but `schema` | `read:page:confluence` | +| Reading pages, and reading/writing page width | `create`, `update`, `fix`, `info`, `read`, `export`, `find` | `read:page:confluence` | | Creating and updating pages, and setting page width | `create`, `update`, `fix` | `write:page:confluence` | | Resolving a space key to an id | `create`, `find`, `search` | `read:space:confluence` | -| Looking up a folder (a folder can be a page's parent) | `create`, `children`, `find` | `read:folder:confluence` | +| Looking up a folder (a folder can be a page's parent) | `create` | `read:folder:confluence` | | CQL queries | `find`, `search` | `search:confluence` | | Author names | `info` | `read:confluence-user` | | Uploading image attachments | `create`, `update`, `attachment-upload` | `write:confluence-file` | | Downloading attachments | `export`, `attachment-download` | `readonly:content.attachment:confluence` | -| Listing attachments and child pages/folders | `children`, `export`, `read`, `attachment-list` | `read:confluence-content.summary` | +| Listing attachments and child pages/folders | `children`, `export`, `read`, `attachment-list`, `attachment-download` | `read:confluence-content.summary` | Copy-pasteable: @@ -239,8 +240,8 @@ 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. + ✗ [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. ``` A file whose `page_id` doesn't resolve is also a failure, not a fresh page: @@ -296,7 +297,7 @@ frontmatter line is present — otherwise the live page's width is left untouche `update` never writes back to the file. A `page_id` that no longer resolves fails that file with what to do about it -(`page_id 999 not found (deleted, trashed, or wrong); correct it, or remove it and +(`page_id 999 not found (deleted or wrong); correct it, or remove it and use create instead`), and one that isn't a numeric id at all is reported without asking Confluence. Since `update` writes nothing back, fixing the id is always safe: the file is exactly as you left it. @@ -373,11 +374,12 @@ composes with shell redirection. markfluence emits round-trip faithfully, while editor-authored content degrades gracefully — any macro markfluence doesn't map (panels, expand, status, …) and column layouts pass through as raw storage tags, with a macro/cell body kept as - readable markdown, so they round-trip back through `create`/`update`. Some - transforms are lossy (e.g. `CAUTION` - alerts, internal links, original image paths, and table cell background colors - cannot be recovered), so this is - a reading aid, not a guaranteed source round-trip. + readable markdown, so they round-trip back through `create`/`update`. A page or + space link converts back to a markdown link; a mention, an attachment link, and + a blog-post link stay as raw storage, since a markdown link would republish to + something else or nothing at all. Some other transforms are lossy (e.g. + `CAUTION` alerts and table cell background colors cannot be recovered), so this + is a reading aid, not a guaranteed source round-trip. - `storage` — the page's raw storage-format XHTML, exactly as stored. ```sh @@ -451,9 +453,9 @@ markfluence find "Deploy runbook" --json | jq -r '.results[] | select(.type=="pa ``` TYPE ID SPACE STATUS TITLE URL -folder 2950660103 CLOUDSERVICES current Deploy runbook https://org.atlassian.net/wiki/spaces/CLOUDSERVICES/folder/2950660103 -page 5144768 CEX current Deploy runbook https://org.atlassian.net/wiki/spaces/CEX/pages/5144768/Deploy+runbook page 3277005 AVSE archived Deploy runbook https://org.atlassian.net/wiki/spaces/AVSE/pages/3277005/Deploy+runbook +page 5144768 CEX current Deploy runbook https://org.atlassian.net/wiki/spaces/CEX/pages/5144768/Deploy+runbook +folder 2950660103 CLOUDSERVICES current Deploy runbook https://org.atlassian.net/wiki/spaces/CLOUDSERVICES/folder/2950660103 ``` The match is **exact and case-insensitive** — not a substring search. Results are @@ -622,9 +624,9 @@ List a page's attachments. ```console $ markfluence attachment-list 1234567890 -NAME SIZE VER TYPE SOURCE -assets%2Fdiagram.png 24.1 KB 3 image/png assets/diagram.png -notes.pdf 1.2 MB 1 application/pdf - +NAME SIZE VER TYPE SOURCE +assets%2Fdiagram.png 24.1 KB 3 image/png assets/diagram.png +notes.pdf 1.2 MB 1 application/pdf - ``` `NAME` is the name Confluence stores. For an image markfluence published that is @@ -716,10 +718,12 @@ target (a single element for `info`/`read`); `summary` carries batch counts: "schema_version": 1, "markfluence_version": "1.4.0", "command": "update", + "roots": ["/repo/docs"], "results": [ { "ok": true, "status": "published", + "dry_run": false, "file": "docs/foo.md", "page_id": "123", "title": "Foo", @@ -752,6 +756,11 @@ Notes on the schema: - **Per-command stable.** Each command always emits the same keys in the same shapes (empty values are `null` or `[]`); the key *set* differs per command. `schema_version` is bumped on any breaking change. +- **`roots`** lists every distinct [documentation root](#the-documentation-root) + the command resolved, sorted — `[]` for a command with no per-file root + concept (`find`, `search`, ...) or a pre-flight failure that never reached + root resolution. `schema` emits no envelope at all, so it has no `roots` key + to speak of. - **Status verbs** are per-command: `published`/`skipped` (`update`), `created`/`not_created` (`create`), `changed`/`consistent` (`fix`), `created`/`updated`/`skipped` (`attachment-upload`), @@ -810,7 +819,7 @@ from it — without reading it out of this repository: ```console $ markfluence schema | jq -r '.properties.command.enum | join(" ")' -info read update create fix attachment-list attachment-upload attachment-download export +info read update create fix children find search attachment-list attachment-upload attachment-download export $ markfluence update docs/*.md --json > out.json $ markfluence schema > schema.json From 9083b7c917641c415edaba3fa2e706dba0a71e2a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 11:31:23 -0400 Subject: [PATCH 02/22] docs(cmd): fix stale and incomplete help text Verified every subcommand's --help text against its actual implementation. Fixes read's frontmatter field list (parent was missing), create's misleading link/parent-cycle claim, and adds material omissions that were real usage traps: an unknown find --space key errors rather than returning empty, search --cql refuses --space/--type, export's --file id fallback, and attachment-download's outside-root refusal. Also hides create's --persist flag, which only existed to give --no-persist a positive counterpart and was never documented anywhere. --- cmd/attachmentdownload/attachmentdownload.go | 3 +++ cmd/create/create.go | 17 ++++++++++++----- cmd/export/export.go | 2 +- cmd/find/find.go | 2 +- cmd/read/read.go | 2 +- cmd/search/search.go | 3 ++- 6 files changed, 20 insertions(+), 9 deletions(-) diff --git a/cmd/attachmentdownload/attachmentdownload.go b/cmd/attachmentdownload/attachmentdownload.go index 3fb6ff1..8ba8c43 100644 --- a/cmd/attachmentdownload/attachmentdownload.go +++ b/cmd/attachmentdownload/attachmentdownload.go @@ -41,6 +41,9 @@ var Cmd = &cobra.Command{ "downloaded tree matches what the page's markdown references and\n" + "previews locally. An attachment without a recorded path is written\n" + "under its stored name. --flat writes everything under stored names.\n\n" + + "A recorded path that would resolve outside --dest is refused for that\n" + + "attachment, since the path comes from an attachment comment anyone who\n" + + "can edit the page controls.\n\n" + "A file that already exists is skipped unless --force.", Args: cobra.MinimumNArgs(1), ValidArgsFunction: completion.PageThenNames, diff --git a/cmd/create/create.go b/cmd/create/create.go index edb06c0..7019580 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -5,8 +5,10 @@ // page converted and given real content (publish). Reserving every id before // converting anything is what makes link resolution stop depending on // creation order -- a link pointing "forward" in the batch resolves exactly -// like one pointing "backward," and a cycle between two pages in the same -// batch resolves too. +// like one pointing "backward," and two pages that link to each other resolve +// 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. package create import ( @@ -48,8 +50,9 @@ var Cmd = &cobra.Command{ "All files are validated first; if any would 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 they form a\n" + - "cycle. --title and --page-width override the frontmatter (--title requires\n" + + "resolves regardless of which direction it points, or whether the two link\n" + + "to each other. A parent cycle among the given files is rejected instead.\n" + + "--title and --page-width override the frontmatter (--title requires\n" + "a single FILE). Unless --no-persist is given, each created page's\n" + "title/space/parent/page_id/page_width are written back into the frontmatter.", Args: cobra.MinimumNArgs(1), @@ -59,7 +62,7 @@ var Cmd = &cobra.Command{ func init() { Cmd.Flags().StringVar(&spaceOpt, "space", "", "Target space key.") - Cmd.Flags().StringVar(&parentOpt, "parent", "", "Parent page id for the new page(s).") + Cmd.Flags().StringVar(&parentOpt, "parent", "", "Parent page or folder id for the new page(s).") Cmd.Flags().StringVar(&titleOpt, "title", "", "Override the page title (requires a single FILE).") Cmd.Flags().StringVar(&pageWidthOpt, "page-width", "", @@ -70,6 +73,10 @@ func init() { "Do not write anything back into the frontmatter.") Cmd.Flags().BoolVar(&dryRunOpt, "dry-run", false, "Preview what would be created without writing to Confluence or files.") + // --persist exists only so wantPersist has a positive flag to combine with + // --no-persist; it says nothing --no-persist's absence doesn't already say, + // so it stays out of --help. + _ = Cmd.Flags().MarkHidden("persist") completion.RegisterFlag(Cmd, "page-width", completion.Values(pagewidth.Vocabulary()...)) } diff --git a/cmd/export/export.go b/cmd/export/export.go index d6c5386..f6e523b 100644 --- a/cmd/export/export.go +++ b/cmd/export/export.go @@ -61,7 +61,7 @@ var Cmd = &cobra.Command{ func init() { Cmd.Flags().StringVar(&dest, "dest", ".", "Directory to write the export into.") Cmd.Flags().StringVar(&fileFlag, "file", "", - "Name for the page file (default: a slug of the title).") + "Name for the page file (default: a slug of the title, or the page id if that slugs to nothing).") Cmd.Flags().BoolVar(&allAttachments, "all-attachments", false, "Export every attachment on the page, not just the referenced ones.") Cmd.Flags().BoolVar(&skipAttachs, "skip-attachments", false, diff --git a/cmd/find/find.go b/cmd/find/find.go index cf71c64..2c3892a 100644 --- a/cmd/find/find.go +++ b/cmd/find/find.go @@ -42,7 +42,7 @@ var Cmd = &cobra.Command{ func init() { Cmd.Flags().StringVar(&spaceOpt, "space", "", - "Restrict the search to a space, by key.") + "Restrict the search to a space, by key (an unknown key is an error, not an empty result).") completion.RegisterFlag(Cmd, "space", cobra.NoFileCompletions) } diff --git a/cmd/read/read.go b/cmd/read/read.go index 700f910..b865f72 100644 --- a/cmd/read/read.go +++ b/cmd/read/read.go @@ -33,7 +33,7 @@ var Cmd = &cobra.Command{ "PAGE is a numeric page id, a Confluence page URL (the modern\n" + "/wiki/.../pages//... form or a legacy ?pageId= URL), or a\n" + "markdown file whose frontmatter has a page_id.\n\n" + - "The default markdown output carries title/page_id/space/page_width\n" + + "The default markdown output carries title/space/parent/page_id/page_width\n" + "frontmatter and is a best-effort inverse of what create/update publish.", Args: cobra.ExactArgs(1), ValidArgsFunction: completion.MarkdownFiles, diff --git a/cmd/search/search.go b/cmd/search/search.go index 9179be9..41f8af3 100644 --- a/cmd/search/search.go +++ b/cmd/search/search.go @@ -78,7 +78,8 @@ func init() { Cmd.Flags().StringVar(&limitOpt, "limit", defaultLimit, fmt.Sprintf("How many matches to show: a positive number, or %q.", limitAll)) Cmd.Flags().BoolVar(&cqlOpt, "cql", false, - "Treat QUERY as a raw CQL query instead of text to search for.") + "Treat QUERY as a raw CQL query instead of text to search for; "+ + "cannot be combined with --space or an explicit --type (put those clauses in the query).") completion.RegisterFlag(Cmd, "space", cobra.NoFileCompletions) completion.RegisterFlag(Cmd, "type", completion.Values( From 68ce502b8c3a5c43e1bfd91efde73df26af39ca3 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 11:59:04 -0400 Subject: [PATCH 03/22] refactor(create): typed parentKind enum, drop dead pageIDFailure.title parentInfo.kind was a bare string compared against four hand-typed literals with no compile-time exhaustiveness check; a typo would silently fall through every switch instead of failing to build. pageIDFailure.title was set but never read by any caller. --- cmd/create/create.go | 32 ++++++++++++++++++++------------ cmd/create/create_test.go | 6 +++--- cmd/create/parent_test.go | 6 +++--- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 7019580..9ce1c52 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -81,9 +81,19 @@ func init() { completion.RegisterFlag(Cmd, "page-width", completion.Values(pagewidth.Vocabulary()...)) } -// parentInfo describes a resolved parent. kind is top|inset|published|external. +// parentKind is which of the four ways a parent was resolved. +type parentKind string + +const ( + parentTop parentKind = "top" // no parent: a top-level page + parentInSet parentKind = "inset" // a .md parent in this same create batch, not yet published + parentPublished parentKind = "published" // a .md parent already published, resolved by its own page_id + parentExternal parentKind = "external" // a bare page/folder id, from --parent or frontmatter +) + +// parentInfo describes a resolved parent. type parentInfo struct { - kind string + kind parentKind id string // page id (published/external) abs string // absolute path of an in-set parent // parentType is what the parent *is* on the server: "page" or "folder". It @@ -131,7 +141,6 @@ type failure struct { // leaving a consumer to parse them out of the message. type pageIDFailure struct { pageID string - title string // the live page's title, when one exists url string // the live page's URL, when one exists message string } @@ -183,7 +192,6 @@ func pageIDFailureFor(c *client.ConfluenceClient, pageID string, page *client.Pa url := pageURL(c, page, pageID) return &pageIDFailure{ pageID: pageID, - title: page.Title, url: url, message: fmt.Sprintf("a page already exists at page_id %s (%q): %s", pageID, page.Title, url), @@ -279,7 +287,7 @@ func run(cmd *cobra.Command, args []string) error { byAbs[r.absPath] = r } for _, r := range records { - if r.parent.kind == "inset" && byAbs[r.parent.abs].spaceID != r.spaceID { + 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"}) } } @@ -356,7 +364,7 @@ func createAll(ordered []record, c *client.ConfluenceClient, doPersist bool) []* for _, r := range ordered { parentID := r.parent.id - if r.parent.kind == "inset" { + if r.parent.kind == parentInSet { parentID = created[r.parent.abs] // In a dry-run nothing is created, so an in-set parent has no id // yet; that is not a failure (the parent would have been created @@ -615,7 +623,7 @@ func resolveParent( parentValue = parentOpt } if parentValue == "" || parentValue == "null" { - return parentInfo{kind: "top"}, nil + return parentInfo{kind: parentTop}, nil } if strings.HasSuffix(parentValue, ".md") { @@ -655,7 +663,7 @@ func resolveParent( if inSetAbs[parentAbs] { // An in-set parent is a page this run creates, so its kind is known // without asking the server. - return parentInfo{kind: "inset", abs: parentAbs, parentType: "page", display: parentValue}, nil + return parentInfo{kind: parentInSet, abs: parentAbs, parentType: "page", display: parentValue}, nil } data, err := root.FS.ReadFile(rel) if err != nil { @@ -670,14 +678,14 @@ func resolveParent( if err != nil { return parentInfo{}, err } - return parentInfo{kind: "published", id: pID, parentType: parentType, display: parentValue}, nil + return parentInfo{kind: parentPublished, id: pID, parentType: parentType, display: parentValue}, nil } parentType, err := checkParentInSpace(c, parentValue, spaceID) if err != nil { return parentInfo{}, err } - return parentInfo{kind: "external", id: parentValue, parentType: parentType}, nil + return parentInfo{kind: parentExternal, id: parentValue, parentType: parentType}, nil } // checkParentInSpace verifies that parentID names something in spaceID that can @@ -725,7 +733,7 @@ func topoSort(records []record, byAbs map[string]record) ([]record, error) { indeg[r.absPath] = 0 } for _, r := range records { - if r.parent.kind == "inset" { + if r.parent.kind == parentInSet { children[r.parent.abs] = append(children[r.parent.abs], r.absPath) indeg[r.absPath]++ } @@ -760,7 +768,7 @@ func topoSort(records []record, byAbs map[string]record) ([]record, error) { // parentField builds the (value, comment) for the frontmatter parent line. func parentField(p parentInfo, parentID string) (value, comment string) { - if p.kind == "top" { + if p.kind == parentTop { return "null", "" } return parentID, p.display diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index 5a63cea..7d0f02c 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -36,8 +36,8 @@ func TestPageIDFailureFor(t *testing.T) { t.Fatalf("error is %T, want *pageIDFailure", err) } wantURL := "https://wiki.example.net/wiki/spaces/ENG/pages/123/Deploy+Runbook" - if pf.pageID != "123" || pf.url != wantURL || pf.title != "Deploy Runbook" { - t.Errorf("fields = %+v, want id 123, title Deploy Runbook, url %s", pf, wantURL) + if pf.pageID != "123" || pf.url != wantURL { + t.Errorf("fields = %+v, want id 123, url %s", pf, wantURL) } // The URL is the point of the issue: it must be in the message a human // reads, not only in the JSON fields. @@ -54,7 +54,7 @@ func TestPageIDFailureFor(t *testing.T) { if !errors.As(err, &pf) { t.Fatalf("error is %T, want *pageIDFailure", err) } - if pf.pageID != "999" || pf.url != "" || pf.title != "" { + if pf.pageID != "999" || pf.url != "" { t.Errorf("fields = %+v, want id 999 and no page details", pf) } // Must say the id is bad and what to do -- not create a page silently. diff --git a/cmd/create/parent_test.go b/cmd/create/parent_test.go index e954d18..7cd0270 100644 --- a/cmd/create/parent_test.go +++ b/cmd/create/parent_test.go @@ -26,7 +26,7 @@ func TestResolveParentNoneGiven(t *testing.T) { if err != nil { t.Fatal(err) } - if p.kind != "top" { + if p.kind != parentTop { t.Errorf("kind = %q, want top", p.kind) } } @@ -65,7 +65,7 @@ func TestResolveParentMdFileInSet(t *testing.T) { if err != nil { t.Fatal(err) } - if p.kind != "inset" || p.abs != parentPath { + if p.kind != parentInSet || p.abs != parentPath { t.Errorf("p = %+v, want kind=inset abs=%q", p, parentPath) } } @@ -86,7 +86,7 @@ func TestResolveParentMdFilePublished(t *testing.T) { if err != nil { t.Fatal(err) } - if p.kind != "published" || p.id != "100" { + if p.kind != parentPublished || p.id != "100" { t.Errorf("p = %+v, want kind=published id=100", p) } } From a11b3dea69e9c871345c197a84a4028d0f1ee11b Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 11:59:44 -0400 Subject: [PATCH 04/22] refactor(json): share attachment-upload/download's per-file result shape jsonUploadResult and jsonDownloadResult were the same shape -- ok/status/ dry_run/filename/error/code -- with download only adding dest_path. Replaced both with jsonout.AttachmentActionResult, which upload leaves dest_path nil on. Schema updated to match: attachmentUploadResult now requires dest_path (always null). --- cmd/attachmentdownload/json.go | 27 ++++++++++----------------- cmd/attachmentupload/json.go | 26 ++++++++++---------------- cmd/attachmentupload/json_test.go | 1 + internal/jsonout/types.go | 14 ++++++++++++++ schema/json-output/v1.json | 5 +++-- 5 files changed, 38 insertions(+), 35 deletions(-) diff --git a/cmd/attachmentdownload/json.go b/cmd/attachmentdownload/json.go index 7ff33f8..df5987b 100644 --- a/cmd/attachmentdownload/json.go +++ b/cmd/attachmentdownload/json.go @@ -1,23 +1,16 @@ package attachmentdownload -import "github.com/mozilla/markfluence/internal/attachfile" +import ( + "github.com/mozilla/markfluence/internal/attachfile" + "github.com/mozilla/markfluence/internal/jsonout" +) -// jsonDownloadResult is attachment-download's --json result shape: one object -// per attachment. dest_path is the local path written, which is the piece a -// caller cannot derive itself -- it depends on the recorded source path, --flat, -// and --dest. It is null only when resolving the path is what failed. -type jsonDownloadResult struct { - OK bool `json:"ok"` - Status string `json:"status"` - DryRun bool `json:"dry_run"` - Filename string `json:"filename"` - DestPath *string `json:"dest_path"` - Error *string `json:"error"` - Code *string `json:"code"` -} - -func buildResult(r attachfile.Outcome) jsonDownloadResult { - res := jsonDownloadResult{ +// buildResult builds attachment-download's --json result for one attachment. +// dest_path is the local path written, which is the piece a caller cannot +// derive itself -- it depends on the recorded source path, --flat, and --dest. +// It is null only when resolving the path is what failed. +func buildResult(r attachfile.Outcome) jsonout.AttachmentActionResult { + res := jsonout.AttachmentActionResult{ OK: r.Status != attachfile.StatusFailed, Status: r.Status, DryRun: dryRun, diff --git a/cmd/attachmentupload/json.go b/cmd/attachmentupload/json.go index e1198f7..a7650af 100644 --- a/cmd/attachmentupload/json.go +++ b/cmd/attachmentupload/json.go @@ -1,22 +1,16 @@ package attachmentupload -import "github.com/mozilla/markfluence/internal/client" +import ( + "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/jsonout" +) -// jsonUploadResult is attachment-upload's --json result shape: one object per -// file. status uses the same created/updated/skipped verbs the attachments -// array on update and create already reports, so a script that understands one -// understands the other. -type jsonUploadResult struct { - OK bool `json:"ok"` - Status string `json:"status"` - DryRun bool `json:"dry_run"` - Filename string `json:"filename"` - Error *string `json:"error"` - Code *string `json:"code"` -} - -func buildResult(a client.SyncAction) jsonUploadResult { - return jsonUploadResult{ +// buildResult builds attachment-upload's --json result for one file. status uses +// the same created/updated/skipped verbs the attachments array on update and +// create already reports, so a script that understands one understands the +// other. dest_path is always null: upload has no local destination to report. +func buildResult(a client.SyncAction) jsonout.AttachmentActionResult { + return jsonout.AttachmentActionResult{ OK: true, Status: a.Action, DryRun: dryRun, diff --git a/cmd/attachmentupload/json_test.go b/cmd/attachmentupload/json_test.go index 4426b02..9ceb7fb 100644 --- a/cmd/attachmentupload/json_test.go +++ b/cmd/attachmentupload/json_test.go @@ -63,6 +63,7 @@ func TestJSONUploadResultMarshal(t *testing.T) { "status": "created", "dry_run": false, "filename": "a.png", + "dest_path": null, "error": null, "code": null }` diff --git a/internal/jsonout/types.go b/internal/jsonout/types.go index a85e69d..ca55963 100644 --- a/internal/jsonout/types.go +++ b/internal/jsonout/types.go @@ -31,6 +31,20 @@ type Attachment struct { Filename string `json:"filename"` } +// AttachmentActionResult is one file's outcome from attachment-upload or +// attachment-download: what happened, whether it succeeded, and (download only) +// where it landed on disk. DestPath is always nil for attachment-upload, which +// has no local destination to report. +type AttachmentActionResult struct { + OK bool `json:"ok"` + Status string `json:"status"` + DryRun bool `json:"dry_run"` + Filename string `json:"filename"` + DestPath *string `json:"dest_path"` + Error *string `json:"error"` + Code *string `json:"code"` +} + // SingleOpFailure is the results[0] entry a single-target command emits when its // one operation against the page fails (not found, fetch error) -- // #/$defs/singleOpFailure in the schema. diff --git a/schema/json-output/v1.json b/schema/json-output/v1.json index 97f96f6..b46080a 100644 --- a/schema/json-output/v1.json +++ b/schema/json-output/v1.json @@ -424,15 +424,16 @@ } }, "attachmentUploadResult": { - "description": "One uploaded file. status uses the same verbs as the attachments array on update/create.", + "description": "One uploaded file. status uses the same verbs as the attachments array on update/create. dest_path is always null: upload has no local destination to report, and only exists here so upload and download share one result shape.", "type": "object", "additionalProperties": false, - "required": ["ok", "status", "dry_run", "filename", "error", "code"], + "required": ["ok", "status", "dry_run", "filename", "dest_path", "error", "code"], "properties": { "ok": { "type": "boolean" }, "status": { "enum": ["created", "updated", "skipped", "failed"] }, "dry_run": { "type": "boolean" }, "filename": { "type": "string" }, + "dest_path": { "const": null }, "error": { "$ref": "#/$defs/stringOrNull" }, "code": { "$ref": "#/$defs/codeOrNull" } } From 26c536dbbfd86a7c285f6be48a9c65494aaee455 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:00:40 -0400 Subject: [PATCH 05/22] refactor: share the local-attachment shape between convert and client convert.Attachment and client.LocalAttachment were byte-identical structs (Filename, Path, Source), manually field-copied at two call sites through near-duplicate toLocalAttachments helpers in create.go and update.go. internal/convert is deliberately client-free, so the shared shape now lives in a new internal/attachref package that both sides alias to, letting create/update pass pageContent.Attachments straight through. --- cmd/create/create.go | 10 +--------- cmd/update/update.go | 12 ++---------- internal/attachref/attachref.go | 17 +++++++++++++++++ internal/client/client.go | 11 +++++------ internal/convert/page.go | 13 ++++--------- 5 files changed, 29 insertions(+), 34 deletions(-) create mode 100644 internal/attachref/attachref.go diff --git a/cmd/create/create.go b/cmd/create/create.go index 9ce1c52..1d8d978 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -502,7 +502,7 @@ func publishOne(r record, res *createResult, pageID string, version int, c *clie } res.url = pageURL(c, result, pageID) - actions, err := c.SyncAttachments(pageID, toLocalAttachments(pageContent.Attachments)) + actions, err := c.SyncAttachments(pageID, pageContent.Attachments) if err != nil { return res.fail(err, jsonout.CodeFor(err)) } @@ -810,11 +810,3 @@ func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) strin } return base + page.Links.WebUI } - -func toLocalAttachments(atts []convert.Attachment) []client.LocalAttachment { - out := make([]client.LocalAttachment, len(atts)) - for i, a := range atts { - out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename, Source: a.Source} - } - return out -} diff --git a/cmd/update/update.go b/cmd/update/update.go index ceefc9c..2fad8f6 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -217,7 +217,7 @@ func processFile( // no writes. The version bump and page URL are the same values a real run // would produce, so the human output lines are identical. if dryRun { - actions, err := c.PlanAttachments(pageID, toLocalAttachments(pageContent.Attachments)) + actions, err := c.PlanAttachments(pageID, pageContent.Attachments) if err != nil { return r.fail(err, jsonout.CodeFor(err)) } @@ -231,7 +231,7 @@ func processFile( return r } - actions, err := c.SyncAttachments(pageID, toLocalAttachments(pageContent.Attachments)) + actions, err := c.SyncAttachments(pageID, pageContent.Attachments) if err != nil { return r.fail(err, jsonout.CodeFor(err)) } @@ -337,11 +337,3 @@ func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) strin } return base + page.Links.WebUI } - -func toLocalAttachments(atts []convert.Attachment) []client.LocalAttachment { - out := make([]client.LocalAttachment, len(atts)) - for i, a := range atts { - out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename, Source: a.Source} - } - return out -} diff --git a/internal/attachref/attachref.go b/internal/attachref/attachref.go new file mode 100644 index 0000000..efadad8 --- /dev/null +++ b/internal/attachref/attachref.go @@ -0,0 +1,17 @@ +// Package attachref defines the local-attachment reference shared by +// internal/convert (which discovers these while converting a page's markdown) +// and internal/client (which uploads them). internal/convert is deliberately +// client-free, so this shape -- otherwise identical on both sides -- lives here +// instead of being owned by either. +package attachref + +// LocalAttachment is a local file to be uploaded as one page's attachment. +// Path is absolute. Filename is the attachment name, a bijective encoding of +// Source, so distinct images can never collide on one name. Source is the +// normalized page-relative path the image was written as, recorded on the +// attachment so a later read recovers it exactly rather than inferring it. +type LocalAttachment struct { + Filename string `json:"filename"` + Path string `json:"path"` + Source string `json:"source"` +} diff --git a/internal/client/client.go b/internal/client/client.go index 9d78c0c..2be2c93 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -36,6 +36,8 @@ import ( "strconv" "strings" "time" + + "github.com/mozilla/markfluence/internal/attachref" ) // An uploaded attachment carries markfluence bookkeeping in its comment: the @@ -345,12 +347,9 @@ type Property struct { // LocalAttachment is a local image to sync to a page. Source is the markdown // image path it was written as, recorded in the attachment's comment; it may be -// empty, in which case only a checksum is recorded. -type LocalAttachment struct { - Path string - Filename string - Source string -} +// empty, in which case only a checksum is recorded. It's the same shape +// internal/convert discovers images as -- see attachref.LocalAttachment. +type LocalAttachment = attachref.LocalAttachment // AttachmentMeta is the markfluence bookkeeping parsed out of an attachment's // comment. A hand-uploaded attachment has none, leaving Managed false. diff --git a/internal/convert/page.go b/internal/convert/page.go index 9efc2cc..2d5906c 100644 --- a/internal/convert/page.go +++ b/internal/convert/page.go @@ -1,5 +1,7 @@ package convert +import "github.com/mozilla/markfluence/internal/attachref" + // ConfluencePage is the result of converting a markdown body to Confluence // storage format: the storage-format HTML plus the local images the body // references. Fields are ordered so the JSON encoding reads with sorted keys. @@ -15,12 +17,5 @@ type ConfluencePage struct { } // Attachment is a local image the body references, to be uploaded to the page. -// Path is absolute. Filename is the attachment name, a bijective encoding of -// Source, so distinct images can never collide on one name. Source is the -// normalized page-relative path the image was written as, recorded on the -// attachment so a later read recovers it exactly rather than inferring it. -type Attachment struct { - Filename string `json:"filename"` - Path string `json:"path"` - Source string `json:"source"` -} +// It's the same shape internal/client uploads from -- see attachref.LocalAttachment. +type Attachment = attachref.LocalAttachment From b8efe601f3df4c1cb09e75d6bc58c13d0c6828eb Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:01:01 -0400 Subject: [PATCH 06/22] refactor(client): rename Options to ResolveOptions Options and Config read as arbitrarily paired names for what is really a two-stage pipeline (Resolve(Options) -> Config -> New(Config)); the new name makes Options' role as Resolve's input side explicit. --- cmd/attachmentdownload/attachmentdownload.go | 2 +- cmd/attachmentlist/attachmentlist.go | 2 +- cmd/attachmentupload/attachmentupload.go | 2 +- cmd/children/children.go | 2 +- cmd/create/create.go | 2 +- cmd/export/export.go | 2 +- cmd/find/find.go | 2 +- cmd/fix/fix.go | 2 +- cmd/info/info.go | 2 +- cmd/read/read.go | 2 +- cmd/search/search.go | 2 +- cmd/update/update.go | 2 +- docs/root-model.md | 2 +- internal/client/client_test.go | 8 +++--- internal/client/config.go | 8 +++--- internal/client/config_test.go | 26 ++++++++++---------- 16 files changed, 34 insertions(+), 34 deletions(-) diff --git a/cmd/attachmentdownload/attachmentdownload.go b/cmd/attachmentdownload/attachmentdownload.go index 8ba8c43..4afb2e7 100644 --- a/cmd/attachmentdownload/attachmentdownload.go +++ b/cmd/attachmentdownload/attachmentdownload.go @@ -66,7 +66,7 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/attachmentlist/attachmentlist.go b/cmd/attachmentlist/attachmentlist.go index 8da1b76..1684a89 100644 --- a/cmd/attachmentlist/attachmentlist.go +++ b/cmd/attachmentlist/attachmentlist.go @@ -43,7 +43,7 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/attachmentupload/attachmentupload.go b/cmd/attachmentupload/attachmentupload.go index 2ebd39e..1e51450 100644 --- a/cmd/attachmentupload/attachmentupload.go +++ b/cmd/attachmentupload/attachmentupload.go @@ -71,7 +71,7 @@ func run(cmd *cobra.Command, args []string) error { rootOverride, _ := cmd.Flags().GetString("root") roots := project.NewCache(rootOverride) defer roots.Close() - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, Roots: roots, }) if err != nil { diff --git a/cmd/children/children.go b/cmd/children/children.go index 4a00a3c..efa3a19 100644 --- a/cmd/children/children.go +++ b/cmd/children/children.go @@ -61,7 +61,7 @@ func run(cmd *cobra.Command, args []string) error { return fatalFail(err.Error(), jsonout.CodeValidation) } - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/create/create.go b/cmd/create/create.go index 1d8d978..6fbde8c 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -245,7 +245,7 @@ func run(cmd *cobra.Command, args []string) error { rootOverride, _ := cmd.Flags().GetString("root") roots := project.NewCache(rootOverride) defer roots.Close() - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, Roots: roots, }) if err != nil { diff --git a/cmd/export/export.go b/cmd/export/export.go index f6e523b..d5c06dd 100644 --- a/cmd/export/export.go +++ b/cmd/export/export.go @@ -78,7 +78,7 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/find/find.go b/cmd/find/find.go index 2c3892a..97db3c1 100644 --- a/cmd/find/find.go +++ b/cmd/find/find.go @@ -61,7 +61,7 @@ func run(cmd *cobra.Command, args []string) error { return fatalFail("no title given: TITLE must not be empty", jsonout.CodeValidation) } - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/fix/fix.go b/cmd/fix/fix.go index 1eef6d3..47066b6 100644 --- a/cmd/fix/fix.go +++ b/cmd/fix/fix.go @@ -44,7 +44,7 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/info/info.go b/cmd/info/info.go index 15bf1d0..e2fcbb1 100644 --- a/cmd/info/info.go +++ b/cmd/info/info.go @@ -45,7 +45,7 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/read/read.go b/cmd/read/read.go index b865f72..4b4c83f 100644 --- a/cmd/read/read.go +++ b/cmd/read/read.go @@ -62,7 +62,7 @@ func run(cmd *cobra.Command, args []string) error { username, _ := cmd.Flags().GetString("username") cloudID, _ := cmd.Flags().GetString("cloud-id") envFile, _ := cmd.Flags().GetString("env-file") - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/search/search.go b/cmd/search/search.go index 41f8af3..1a1df52 100644 --- a/cmd/search/search.go +++ b/cmd/search/search.go @@ -112,7 +112,7 @@ func run(cmd *cobra.Command, args []string) error { return fatalFail(err.Error(), jsonout.CodeValidation) } - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, }) if err != nil { diff --git a/cmd/update/update.go b/cmd/update/update.go index 2fad8f6..b50e69e 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -76,7 +76,7 @@ func run(cmd *cobra.Command, args []string) error { rootOverride, _ := cmd.Flags().GetString("root") roots := project.NewCache(rootOverride) defer roots.Close() - c, err := client.Resolve(client.Options{ + c, err := client.Resolve(client.ResolveOptions{ URL: url, Username: username, CloudID: cloudID, EnvFile: envFile, Roots: roots, }) if err != nil { diff --git a/docs/root-model.md b/docs/root-model.md index 5cbf707..a55b318 100644 --- a/docs/root-model.md +++ b/docs/root-model.md @@ -40,7 +40,7 @@ second starting point. In the bare case (`project.Discover(cwd)`) that walk is independent of anything else in the invocation. But `create`, `update`, and `attachment-upload` each already build a `project.Cache` for their own per-file root resolution, and hand that same cache to the client config -resolver (`client.Options.Roots`) instead of leaving `.env` to make its own, +resolver (`client.ResolveOptions.Roots`) instead of leaving `.env` to make its own, separate walk. Two consequences follow, for exactly those commands (one with no per-file root concept, like `read` or `search`, never builds a cache to share): `--root`'s override — which otherwise only redirects the per-file diff --git a/internal/client/client_test.go b/internal/client/client_test.go index d61a9c1..6337247 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -778,17 +778,17 @@ func TestResolve(t *testing.T) { t.Setenv("CONFLUENCE_CLOUD_ID", "") // All from .env. - c, err := Resolve(Options{}) + c, err := Resolve(ResolveOptions{}) if err != nil || c.BaseURL() != "https://file.example.net" { t.Fatalf("Resolve(.env) = %v, %v", c, err) } // Flag beats env beats .env for the URL. t.Setenv("CONFLUENCE_URL", "https://env.example.net") - if c, _ := Resolve(Options{URL: "https://flag.example.net"}); c.BaseURL() != "https://flag.example.net" { + if c, _ := Resolve(ResolveOptions{URL: "https://flag.example.net"}); c.BaseURL() != "https://flag.example.net" { t.Errorf("flag should win, got %q", c.BaseURL()) } - if c, _ := Resolve(Options{}); c.BaseURL() != "https://env.example.net" { + if c, _ := Resolve(ResolveOptions{}); c.BaseURL() != "https://env.example.net" { t.Errorf("env should beat .env, got %q", c.BaseURL()) } @@ -797,7 +797,7 @@ func TestResolve(t *testing.T) { if err := os.WriteFile(".env", []byte("CONFLUENCE_URL=u\nCONFLUENCE_USERNAME=x\n"), 0o644); err != nil { t.Fatal(err) } - if _, err := Resolve(Options{URL: "u", Username: "x"}); err == nil { + if _, err := Resolve(ResolveOptions{URL: "u", Username: "x"}); err == nil { t.Error("Resolve with no token: want error") } } diff --git a/internal/client/config.go b/internal/client/config.go index ca2bf57..b275d64 100644 --- a/internal/client/config.go +++ b/internal/client/config.go @@ -21,9 +21,9 @@ const ( var spaceKeyRE = regexp.MustCompile(`^/spaces/([^/]+)/`) -// Options carries the flag values Resolve needs, named so the two URL-ish fields -// can't be transposed at a call site. -type Options struct { +// ResolveOptions carries the flag values Resolve needs, named so the two +// URL-ish fields can't be transposed at a call site. +type ResolveOptions struct { // URL is the --url value (the Confluence site). URL string // Username is the --username value. @@ -57,7 +57,7 @@ type Options struct { // The cloud ID is optional: without one, requests go to the site domain exactly // as before, which is what an unscoped personal token and any Data Center site // need. -func Resolve(opts Options) (*ConfluenceClient, error) { +func Resolve(opts ResolveOptions) (*ConfluenceClient, error) { env, err := loadEnvFile(opts.EnvFile, opts.Roots) if err != nil { return nil, err diff --git a/internal/client/config_test.go b/internal/client/config_test.go index 5f1b339..dbfaeeb 100644 --- a/internal/client/config_test.go +++ b/internal/client/config_test.go @@ -30,7 +30,7 @@ func writeEnvFile(t *testing.T, body string) string { func TestResolveUsesExplicitEnvFile(t *testing.T) { clearConfluenceEnv(t) path := writeEnvFile(t, "CONFLUENCE_URL=https://wiki\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n") - c, err := Resolve(Options{EnvFile: path}) + c, err := Resolve(ResolveOptions{EnvFile: path}) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -42,7 +42,7 @@ func TestResolveUsesExplicitEnvFile(t *testing.T) { func TestResolveFlagOverridesEnvFile(t *testing.T) { clearConfluenceEnv(t) path := writeEnvFile(t, "CONFLUENCE_URL=https://from-file\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n") - c, err := Resolve(Options{URL: "https://from-flag", EnvFile: path}) + c, err := Resolve(ResolveOptions{URL: "https://from-flag", EnvFile: path}) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -54,7 +54,7 @@ func TestResolveFlagOverridesEnvFile(t *testing.T) { func TestResolveMissingExplicitEnvFileErrors(t *testing.T) { clearConfluenceEnv(t) missing := filepath.Join(t.TempDir(), "nope.env") - if _, err := Resolve(Options{EnvFile: missing}); err == nil { + if _, err := Resolve(ResolveOptions{EnvFile: missing}); err == nil { t.Fatal("Resolve: want error for a missing --env-file path") } } @@ -64,7 +64,7 @@ func TestResolveDefaultEnvFileMissingIsFine(t *testing.T) { // No ./.env in this temp cwd, and no explicit env file: the missing default // is tolerated, so we fail only on missing config values (not a read error). t.Chdir(t.TempDir()) - _, err := Resolve(Options{}) + _, err := Resolve(ResolveOptions{}) if err == nil { t.Fatal("want a missing-config error") } @@ -85,7 +85,7 @@ func TestResolveDefaultEnvFileFoundInCwdWithNoProjectFile(t *testing.T) { // itself -- today's behavior, preserved. t.Chdir(dir) - c, err := Resolve(Options{}) + c, err := Resolve(ResolveOptions{}) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -112,7 +112,7 @@ func TestResolveDefaultEnvFileFoundAtDiscoveredProjectRoot(t *testing.T) { // discovery finds by walking up. t.Chdir(sub) - c, err := Resolve(Options{}) + c, err := Resolve(ResolveOptions{}) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -121,7 +121,7 @@ func TestResolveDefaultEnvFileFoundAtDiscoveredProjectRoot(t *testing.T) { } } -// TestResolveRootsOverridesEnvDiscovery covers Options.Roots: when the caller +// TestResolveRootsOverridesEnvDiscovery covers ResolveOptions.Roots: when the caller // passes its own --root-backed project.Cache, .env is read from that root, not // from a plain upward walk from the working directory -- so a --root pointed // at a different project also redirects which .env create/update/ @@ -139,7 +139,7 @@ func TestResolveRootsOverridesEnvDiscovery(t *testing.T) { roots := project.NewCache(override) defer roots.Close() - c, err := Resolve(Options{Roots: roots}) + c, err := Resolve(ResolveOptions{Roots: roots}) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -155,7 +155,7 @@ func TestResolveCloudIDPrecedence(t *testing.T) { "CONFLUENCE_CLOUD_ID=from-file\n") // From .env: requests move to the gateway, the site is untouched. - c, err := Resolve(Options{EnvFile: path}) + c, err := Resolve(ResolveOptions{EnvFile: path}) if err != nil { t.Fatalf("Resolve: %v", err) } @@ -168,10 +168,10 @@ func TestResolveCloudIDPrecedence(t *testing.T) { // Env beats .env; flag beats env. t.Setenv(cloudIDEnv, "from-env") - if c, _ := Resolve(Options{EnvFile: path}); c.BaseURL() != gatewayPrefix+"from-env" { + if c, _ := Resolve(ResolveOptions{EnvFile: path}); c.BaseURL() != gatewayPrefix+"from-env" { t.Errorf("env should beat .env, got %q", c.BaseURL()) } - if c, _ := Resolve(Options{CloudID: "from-flag", EnvFile: path}); c.BaseURL() != gatewayPrefix+"from-flag" { + if c, _ := Resolve(ResolveOptions{CloudID: "from-flag", EnvFile: path}); c.BaseURL() != gatewayPrefix+"from-flag" { t.Errorf("flag should win, got %q", c.BaseURL()) } } @@ -187,7 +187,7 @@ func TestResolveRejectsURLishCloudID(t *testing.T) { "ex/confluence/abc", "abc/wiki", } { - _, err := Resolve(Options{CloudID: bad, EnvFile: path}) + _, err := Resolve(ResolveOptions{CloudID: bad, EnvFile: path}) if err == nil { t.Errorf("Resolve(cloud ID %q): want an error", bad) continue @@ -201,7 +201,7 @@ func TestResolveRejectsURLishCloudID(t *testing.T) { func TestResolveWithoutCloudIDKeepsSiteURL(t *testing.T) { clearConfluenceEnv(t) path := writeEnvFile(t, "CONFLUENCE_URL=https://wiki/\nCONFLUENCE_USERNAME=bot\nCONFLUENCE_TOKEN=secret\n") - c, err := Resolve(Options{EnvFile: path}) + c, err := Resolve(ResolveOptions{EnvFile: path}) if err != nil { t.Fatalf("Resolve: %v", err) } From 6627c9eb21bcd1402f008f52d801eb29e92e7616 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:13:03 -0400 Subject: [PATCH 07/22] test: remove low-value and dead tests TestSubcommandsRegistered duplicated TestCommandEnumMatchesRegisteredCommands for every command except schema (the one enum-exempt case nothing else checks); shrunk to just that. Removed an orphaned test-stub comment in attachment-download left over from when the escape-refusal logic moved to internal/attachfile (tested there now). Consolidated schema_test.go's duplicated cobra SetOut/SetErr/SetArgs boilerplate into runCmd. Dropped TestAttachmentCommentBudget's constant-arithmetic assertion, which checked Go's arithmetic rather than attachmentComment's behavior. --- .../attachmentdownload_test.go | 4 --- cmd/root_test.go | 18 +++++----- cmd/schema/schema_test.go | 33 ++++++++++--------- internal/client/client_test.go | 3 -- 4 files changed, 27 insertions(+), 31 deletions(-) diff --git a/cmd/attachmentdownload/attachmentdownload_test.go b/cmd/attachmentdownload/attachmentdownload_test.go index 21a465a..f0994b4 100644 --- a/cmd/attachmentdownload/attachmentdownload_test.go +++ b/cmd/attachmentdownload/attachmentdownload_test.go @@ -6,7 +6,6 @@ import ( "github.com/mozilla/markfluence/internal/client" ) -// managed builds an attachment carrying a recorded source path. func TestSelectAttachmentsAll(t *testing.T) { all := []client.Attachment{{Title: "a.png"}, {Title: "b.png"}} got, missing := selectAttachments(all, nil) @@ -36,6 +35,3 @@ func TestSelectAttachmentsReportsMissing(t *testing.T) { t.Errorf("missing = %v, want [nope.png]", missing) } } - -// TestDestPathEscapeMessageNamesTheAttachment keeps the failure actionable: the -// user needs to know which attachment was refused. diff --git a/cmd/root_test.go b/cmd/root_test.go index 96e8128..51a5b5c 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -22,16 +22,18 @@ func TestRootCommandWiring(t *testing.T) { } } -func TestSubcommandsRegistered(t *testing.T) { - want := map[string]bool{ - "update": false, "create": false, "fix": false, "info": false, "read": false, "schema": false, - } +// TestSchemaCommandRegistered checks the one registration +// TestCommandEnumMatchesRegisteredCommands can't: every other subcommand's +// registration is implied by its presence in the schema's command enum (that +// test's second loop), but "schema" is deliberately exempt from the enum, so +// nothing else would catch its command being dropped entirely. +func TestSchemaCommandRegistered(t *testing.T) { for _, c := range rootCmd.Commands() { - delete(want, c.Name()) - } - for name := range want { - t.Errorf("subcommand %q not registered", name) + if c.Name() == "schema" { + return + } } + t.Error(`subcommand "schema" not registered`) } // completionOut collects the generated completion scripts; see diff --git a/cmd/schema/schema_test.go b/cmd/schema/schema_test.go index 0d3d450..a96d102 100644 --- a/cmd/schema/schema_test.go +++ b/cmd/schema/schema_test.go @@ -11,8 +11,10 @@ import ( "github.com/santhosh-tekuri/jsonschema/v6" ) -// run executes the command with a captured writer and returns what it printed. -func runCmd(t *testing.T, args ...string) string { +// runCmd executes the command with a captured writer and returns what it +// printed and any error, so a caller expecting success can t.Fatal on err and +// a caller expecting a usage error (TestRejectsArguments) can assert on it. +func runCmd(t *testing.T, args ...string) (string, error) { t.Helper() var out bytes.Buffer Cmd.SetOut(&out) @@ -23,17 +25,25 @@ func runCmd(t *testing.T, args ...string) string { Cmd.SetErr(nil) Cmd.SetArgs(nil) }) - if err := Cmd.Execute(); err != nil { + err := Cmd.Execute() + return out.String(), err +} + +// mustRun is runCmd for the common case: the command must succeed. +func mustRun(t *testing.T, args ...string) string { + t.Helper() + out, err := runCmd(t, args...) + if err != nil { t.Fatalf("schema %v: %v", args, err) } - return out.String() + return out } // TestPrintsSchemaVerbatim pins the whole point of the command: a consumer can // diff or cache what it prints against the published file, so the bytes must // match the embedded schema exactly. func TestPrintsSchemaVerbatim(t *testing.T) { - got := runCmd(t) + got := mustRun(t) if got != schemadoc.V1 { t.Errorf("output is not the embedded schema verbatim (%d bytes vs %d)", len(got), len(schemadoc.V1)) @@ -48,7 +58,7 @@ func TestPrintsSchemaVerbatim(t *testing.T) { // compiles, not merely valid JSON: a truncated or mangled embed would still // parse as an object. func TestOutputIsAUsableSchema(t *testing.T) { - out := runCmd(t) + out := mustRun(t) var doc struct { Schema string `json:"$schema"` @@ -100,16 +110,7 @@ func TestEmbeddedSchemaDescribesEmittedVersion(t *testing.T) { // TestRejectsArguments guards the completion promise: the command takes none. func TestRejectsArguments(t *testing.T) { - var out bytes.Buffer - Cmd.SetOut(&out) - Cmd.SetErr(&out) - Cmd.SetArgs([]string{"v1"}) - t.Cleanup(func() { - Cmd.SetOut(nil) - Cmd.SetErr(nil) - Cmd.SetArgs(nil) - }) - if err := Cmd.Execute(); err == nil { + if _, err := runCmd(t, "v1"); err == nil { t.Error("schema v1 should be a usage error") } } diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 6337247..aa9d629 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -913,9 +913,6 @@ func TestSyncAttachmentsWritesATruncatedChecksum(t *testing.T) { // itself -- up from 165 before the truncation. func TestAttachmentCommentBudget(t *testing.T) { const overhead = len("markfluence: ") + len("sha256=") + checksumHexLen + len(" path=") - if overhead != 58 { - t.Fatalf("overhead = %d, want 58", overhead) - } sum := strings.Repeat("a", checksumHexLen) longestFittingPath := strings.Repeat("p", 255-overhead) if got := attachmentComment(sum, longestFittingPath); len(got) != 255 { From c7cb33aea506c579088783bf4483e54c92165f98 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:15:24 -0400 Subject: [PATCH 08/22] refactor: hoist duplicated pageURL into client.PageURL create.go and update.go each defined a byte-identical pageURL function and an identical TestPageURLUsesSiteNotGateway test. Moved it to internal/client as a ConfluenceClient method, since it only needs SiteURL and a Page, and consolidated the test into internal/client's own suite. --- cmd/create/create.go | 21 +++++--------------- cmd/create/create_test.go | 36 ---------------------------------- cmd/update/update.go | 15 ++------------ cmd/update/update_test.go | 36 ---------------------------------- internal/client/client.go | 15 ++++++++++++++ internal/client/client_test.go | 34 ++++++++++++++++++++++++++++++++ 6 files changed, 56 insertions(+), 101 deletions(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 6fbde8c..e2ecae9 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -189,7 +189,7 @@ func pageIDFailureFor(c *client.ConfluenceClient, pageID string, page *client.Pa message: pageref.NotFoundMessage(pageID, "remove it to create a new page, or correct it"), } } - url := pageURL(c, page, pageID) + url := c.PageURL(page, pageID) return &pageIDFailure{ pageID: pageID, url: url, @@ -226,10 +226,10 @@ func checkTitleFree(c *client.ConfluenceClient, title, spaceKey, spaceID string) // page that is not there and concludes markfluence is wrong. return fmt.Errorf("an archived page titled %q already exists in space %s and still holds "+ "that title; restore it, rename it, or pick another title: %s", - title, spaceKey, pageURL(c, &d, d.ID)) + title, spaceKey, c.PageURL(&d, d.ID)) } return fmt.Errorf("a page titled %q already exists in space %s: %s", - title, spaceKey, pageURL(c, &d, d.ID)) + title, spaceKey, c.PageURL(&d, d.ID)) } func run(cmd *cobra.Command, args []string) error { @@ -443,7 +443,7 @@ func reserveOne( } pageID = result.ID res.pageID = pageID - res.url = pageURL(c, result, pageID) + res.url = c.PageURL(result, pageID) if persist { parentValue, parentComment := parentField(r.parent, parentID) @@ -500,7 +500,7 @@ func publishOne(r record, res *createResult, pageID string, version int, c *clie if err != nil { return res.fail(err, jsonout.CodeFor(err)) } - res.url = pageURL(c, result, pageID) + res.url = c.PageURL(result, pageID) actions, err := c.SyncAttachments(pageID, pageContent.Attachments) if err != nil { @@ -799,14 +799,3 @@ func resolveWidth(cliPageWidth string, fm map[string]string) (pagewidth.Width, e } return pagewidth.Declared(fm) } - -func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) string { - if page.Links.WebUI == "" { - return fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", c.SiteURL(), pageID) - } - base := page.Links.Base - if base == "" { - base = c.SiteURL() + "/wiki" - } - return base + page.Links.WebUI -} diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index 7d0f02c..a6e6c2b 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -161,42 +161,6 @@ func TestOverrideNeedsSingleFile(t *testing.T) { } } -// In gateway mode the request base is api.atlassian.com, which must never reach a -// reader. Every pageURL branch has to resolve against the site instead. -func TestPageURLUsesSiteNotGateway(t *testing.T) { - c := client.New(client.Config{SiteURL: "https://wiki.example.net", CloudID: "abc-123"}) - - tests := []struct { - name string - base, webu string - want string - }{ - { - "no webui link falls back to the site", - "", "", - "https://wiki.example.net/wiki/pages/viewpage.action?pageId=42", - }, - { - "webui with no base joins onto the site", - "", "/spaces/ENG/pages/42/Title", - "https://wiki.example.net/wiki/spaces/ENG/pages/42/Title", - }, - { - "the API's own base is preferred when present", - "https://wiki.example.net/wiki", "/spaces/ENG/pages/42/Title", - "https://wiki.example.net/wiki/spaces/ENG/pages/42/Title", - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - page := &client.Page{ID: "42", Links: client.Links{Base: tc.base, WebUI: tc.webu}} - if got := pageURL(c, page, "42"); got != tc.want { - t.Errorf("pageURL = %q, want %q", got, tc.want) - } - }) - } -} - // parentServer stands in for Confluence for the parent lookups, routing by path // so a folder id can 404 as a page and resolve as a folder -- which is the whole // shape of the bug in #68. diff --git a/cmd/update/update.go b/cmd/update/update.go index b50e69e..4f75e22 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -176,7 +176,7 @@ func processFile( } r.title = title r.versionPrev = page.Version.Number - r.url = pageURL(c, page, pageID) + r.url = c.PageURL(page, pageID) if !force && page.Version.CreatedAt != "" { if pageUpdated, err := time.Parse(time.RFC3339, page.Version.CreatedAt); err == nil { @@ -244,7 +244,7 @@ func processFile( return r.fail(err, jsonout.CodeFor(err)) } r.versionNew = next - r.url = pageURL(c, result, pageID) + r.url = c.PageURL(result, pageID) // Assert the page width (a separate content-property call) only when set; // non-fatal (a failure is a warning, not an error). @@ -326,14 +326,3 @@ func resolveWidth(cliPageWidth string, mf *frontmatter.MarkdownFile) (pagewidth. } return "", false, nil } - -func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) string { - if page.Links.WebUI == "" { - return fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", c.SiteURL(), pageID) - } - base := page.Links.Base - if base == "" { - base = c.SiteURL() + "/wiki" - } - return base + page.Links.WebUI -} diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index 7d2ddc7..aa867c6 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -101,42 +101,6 @@ func TestOverrideNeedsSingleFile(t *testing.T) { } } -// In gateway mode the request base is api.atlassian.com, which must never reach a -// reader. Every pageURL branch has to resolve against the site instead. -func TestPageURLUsesSiteNotGateway(t *testing.T) { - c := client.New(client.Config{SiteURL: "https://wiki.example.net", CloudID: "abc-123"}) - - tests := []struct { - name string - base, webu string - want string - }{ - { - "no webui link falls back to the site", - "", "", - "https://wiki.example.net/wiki/pages/viewpage.action?pageId=42", - }, - { - "webui with no base joins onto the site", - "", "/spaces/ENG/pages/42/Title", - "https://wiki.example.net/wiki/spaces/ENG/pages/42/Title", - }, - { - "the API's own base is preferred when present", - "https://wiki.example.net/wiki", "/spaces/ENG/pages/42/Title", - "https://wiki.example.net/wiki/spaces/ENG/pages/42/Title", - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - page := &client.Page{ID: "42", Links: client.Links{Base: tc.base, WebUI: tc.webu}} - if got := pageURL(c, page, "42"); got != tc.want { - t.Errorf("pageURL = %q, want %q", got, tc.want) - } - }) - } -} - // TestProcessFileRejectsNonNumericPageID covers the local half of the fix: a // page_id that is not an id never reaches the API, so the reader gets a sentence // instead of a 400 body. The client points at a host that does not resolve, so a diff --git a/internal/client/client.go b/internal/client/client.go index 2be2c93..01f9929 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -138,6 +138,21 @@ func (c *ConfluenceClient) BaseURL() string { return c.baseURL } // whether requests are routed through the gateway. func (c *ConfluenceClient) SiteURL() string { return c.siteURL } +// PageURL builds a page's human-facing URL from its own links, falling back to +// the legacy ?pageId= form when Links.WebUI is empty (a page fetched by a route +// that doesn't populate it). Always built off SiteURL, never BaseURL: a reader +// must never see the gateway host. +func (c *ConfluenceClient) PageURL(page *Page, pageID string) string { + if page.Links.WebUI == "" { + return fmt.Sprintf("%s/wiki/pages/viewpage.action?pageId=%s", c.SiteURL(), pageID) + } + base := page.Links.Base + if base == "" { + base = c.SiteURL() + "/wiki" + } + return base + page.Links.WebUI +} + // HTTPError is returned when the API responds with a >= 400 status. type HTTPError struct { StatusCode int diff --git a/internal/client/client_test.go b/internal/client/client_test.go index aa9d629..35a8488 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -344,6 +344,40 @@ func TestGetPagePropagatesError(t *testing.T) { } } +func TestPageURLUsesSiteNotGateway(t *testing.T) { + c := New(Config{SiteURL: "https://wiki.example.net", CloudID: "abc-123"}) + + tests := []struct { + name string + base, webu string + want string + }{ + { + "no webui link falls back to the site", + "", "", + "https://wiki.example.net/wiki/pages/viewpage.action?pageId=42", + }, + { + "webui with no base joins onto the site", + "", "/spaces/ENG/pages/42/Title", + "https://wiki.example.net/wiki/spaces/ENG/pages/42/Title", + }, + { + "the API's own base is preferred when present", + "https://wiki.example.net/wiki", "/spaces/ENG/pages/42/Title", + "https://wiki.example.net/wiki/spaces/ENG/pages/42/Title", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + page := &Page{ID: "42", Links: Links{Base: tc.base, WebUI: tc.webu}} + if got := c.PageURL(page, "42"); got != tc.want { + t.Errorf("PageURL = %q, want %q", got, tc.want) + } + }) + } +} + // --- folders ----------------------------------------------------------------- func TestGetFolderOrNilReturnsFolder(t *testing.T) { From de10e0b13f6e7e2a66436b29b04a7f3d38c5ac51 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:19:56 -0400 Subject: [PATCH 09/22] test: add internal/clienttest, consolidate internal/client's own helpers internal/client had 5 different named httptest-server-plus-client helpers (newServer, countingServer, updateServer, newFindServer, newSearchServer) plus 3 bare inline repeats of the same 3-line construction. Added internal/clienttest for every other package's tests to build on, and a local newTestServer for internal/client's own tests, which can't import clienttest: an internal (white-box) test file may not import a package that itself imports the package under test. --- internal/client/client_test.go | 40 ++++++++++++++++++------------- internal/client/find_test.go | 7 ++---- internal/client/search_test.go | 20 ++++++---------- internal/clienttest/clienttest.go | 27 +++++++++++++++++++++ 4 files changed, 60 insertions(+), 34 deletions(-) create mode 100644 internal/clienttest/clienttest.go diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 35a8488..cfb6f33 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -59,10 +59,23 @@ type resp struct { body string } +// newTestServer starts an httptest.Server running handler, registers its +// shutdown as test cleanup, and returns a client pointed at it. It can't use +// internal/clienttest: an internal (white-box) test file may not import a +// package that itself imports the package under test, and clienttest imports +// client. Every other package's tests should use internal/clienttest instead; +// this copy exists only because internal/client's own tests can't. +func newTestServer(t *testing.T, handler http.HandlerFunc) *ConfluenceClient { + t.Helper() + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + return New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}) +} + func newServer(t *testing.T, responses ...resp) (*ConfluenceClient, *scripted) { t.Helper() s := &scripted{responses: responses} - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { s.calls = append(s.calls, r.Method) body, _ := io.ReadAll(r.Body) s.bodies = append(s.bodies, string(body)) @@ -76,9 +89,8 @@ func newServer(t *testing.T, responses ...resp) (*ConfluenceClient, *scripted) { s.idx++ w.WriteHeader(out.status) _, _ = w.Write([]byte(out.body)) - })) - t.Cleanup(srv.Close) - return New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}), s + }) + return c, s } func eqStrings(a, b []string) bool { @@ -167,11 +179,10 @@ func TestListContentPropertiesFollowsPagination(t *testing.T) { func countingServer(t *testing.T, handler func(w http.ResponseWriter, n int32)) (*ConfluenceClient, *int32) { t.Helper() var n int32 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, _ *http.Request) { handler(w, atomic.AddInt32(&n, 1)) - })) - t.Cleanup(srv.Close) - return New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}), &n + }) + return c, &n } func TestSendRetriesOn429ThenSucceeds(t *testing.T) { @@ -458,14 +469,12 @@ func TestListChildNodesDecodesRows(t *testing.T) { // folders come from a separate v1 path, and missing it loses whole subtrees. func TestListChildFoldersHitsTheFolderPath(t *testing.T) { var gotPath string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { gotPath = r.URL.Path _, _ = w.Write([]byte(`{"results":[{"id":"22","type":"folder","title":"Articles",` + `"status":"current","extensions":{"position":666},` + `"_links":{"webui":"/spaces/ENG/folder/22"}}]}`)) - })) - t.Cleanup(srv.Close) - c := New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}) + }) got, err := c.ListChildFolders("1") if err != nil { @@ -1270,7 +1279,7 @@ func TestBackoffCapsAfterJitter(t *testing.T) { func updateServer(t *testing.T, putStatus int, getBody string) (*ConfluenceClient, *[]string) { t.Helper() var seen []string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { seen = append(seen, r.Method) if r.Method == http.MethodPut { w.WriteHeader(putStatus) @@ -1282,9 +1291,8 @@ func updateServer(t *testing.T, putStatus int, getBody string) (*ConfluenceClien return } _, _ = w.Write([]byte(getBody)) - })) - t.Cleanup(srv.Close) - return New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}), &seen + }) + return c, &seen } func livePage(version int, title, body string) string { diff --git a/internal/client/find_test.go b/internal/client/find_test.go index fc46206..3cbb413 100644 --- a/internal/client/find_test.go +++ b/internal/client/find_test.go @@ -3,7 +3,6 @@ package client import ( "errors" "net/http" - "net/http/httptest" "strings" "testing" ) @@ -21,7 +20,7 @@ type findServer struct { func newFindServer(t *testing.T, f *findServer) *ConfluenceClient { t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + return newTestServer(t, func(w http.ResponseWriter, r *http.Request) { f.urls = append(f.urls, r.URL.String()) var body string var status int @@ -41,9 +40,7 @@ func newFindServer(t *testing.T, f *findServer) *ConfluenceClient { } w.WriteHeader(status) _, _ = w.Write([]byte(body)) - })) - t.Cleanup(srv.Close) - return New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}) + }) } func v2Page(id, title, status, space string) string { diff --git a/internal/client/search_test.go b/internal/client/search_test.go index f6d3297..e5dde30 100644 --- a/internal/client/search_test.go +++ b/internal/client/search_test.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "net/http" - "net/http/httptest" "net/url" "reflect" "strings" @@ -24,7 +23,7 @@ type urlLog struct { func newSearchServer(t *testing.T, bodies ...string) (*ConfluenceClient, *urlLog) { t.Helper() s := &urlLog{bodies: bodies} - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, r *http.Request) { s.urls = append(s.urls, r.URL.String()) if s.idx >= len(s.bodies) { t.Errorf("unexpected extra request: %s", r.URL) @@ -34,9 +33,8 @@ func newSearchServer(t *testing.T, bodies ...string) (*ConfluenceClient, *urlLog body := s.bodies[s.idx] s.idx++ _, _ = w.Write([]byte(body)) - })) - t.Cleanup(srv.Close) - return New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}), s + }) + return c, s } // row builds a minimal folder search hit. @@ -134,11 +132,9 @@ func TestSearchCQLIgnoresTotalSize(t *testing.T) { // TestSearchCQLBoundsARunawayCursor: a next link that never clears must fail // loudly rather than hang or return a plausible-looking prefix of the results. func TestSearchCQLBoundsARunawayCursor(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(page("/rest/api/search?next=true&cursor=loop", row("1")))) - })) - t.Cleanup(srv.Close) - c := New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}) + }) got, err := c.SearchCQL("type = folder") if err == nil { @@ -762,11 +758,9 @@ func TestSearchResultsCarriesTheQuery(t *testing.T) { // TestSearchResultsCarriesTheQueryOnFailure: it is wanted most when the search // did not work. func TestSearchResultsCarriesTheQueryOnFailure(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + c := newTestServer(t, func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusInternalServerError) - })) - t.Cleanup(srv.Close) - c := New(Config{SiteURL: srv.URL, Username: "u", Token: "t"}) + }) got, err := c.SearchRawCQL(`type = space`, 10) if err == nil { diff --git a/internal/clienttest/clienttest.go b/internal/clienttest/clienttest.go new file mode 100644 index 0000000..df204a1 --- /dev/null +++ b/internal/clienttest/clienttest.go @@ -0,0 +1,27 @@ +// Package clienttest builds a *client.ConfluenceClient against an +// httptest.Server for tests across the repo -- internal/client's own suite and +// every other package whose tests need a client talking to canned responses. +// Before this existed, each package (and several test files within +// internal/client itself) reimplemented "start a server, register its +// shutdown, point a client at it" independently, with the same +// username/token placeholders retyped each time. +package clienttest + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/mozilla/markfluence/internal/client" +) + +// New starts an httptest.Server running handler, registers its shutdown as +// test cleanup, and returns a *client.ConfluenceClient pointed at it. The +// username and token are fixed placeholders: nothing in a test server built +// this way checks them, since real auth happens on Atlassian's side. +func New(t *testing.T, handler http.HandlerFunc) *client.ConfluenceClient { + t.Helper() + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + return client.New(client.Config{SiteURL: srv.URL, Username: "u", Token: "t"}) +} From 674306314be4ed8740649be513a1bf1480916af4 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:21:49 -0400 Subject: [PATCH 10/22] test: adopt internal/clienttest in pagetree, pagedoc, attachfile Each had its own single-purpose local reimplementation of the same httptest-server-plus-client boilerplate; switched all three to build on the shared internal/clienttest.New. --- internal/attachfile/attachfile_test.go | 8 +++----- internal/pagedoc/pagelinks_test.go | 9 ++++----- internal/pagetree/pagetree_test.go | 9 ++++----- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/internal/attachfile/attachfile_test.go b/internal/attachfile/attachfile_test.go index 60bd5ac..9f95537 100644 --- a/internal/attachfile/attachfile_test.go +++ b/internal/attachfile/attachfile_test.go @@ -2,13 +2,13 @@ package attachfile import ( "net/http" - "net/http/httptest" "os" "path/filepath" "strings" "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" ) func managed(title, source string) client.Attachment { @@ -119,11 +119,9 @@ func TestResolveEscapeMessageNamesTheAttachment(t *testing.T) { // testClient serves body for any request, standing in for Confluence. func testClient(t *testing.T, body string) *client.ConfluenceClient { t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + return clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(body)) - })) - t.Cleanup(srv.Close) - return client.New(client.Config{SiteURL: srv.URL, Username: "u", Token: "t"}) + }) } // withDownload returns an attachment carrying a download link, without which diff --git a/internal/pagedoc/pagelinks_test.go b/internal/pagedoc/pagelinks_test.go index 495e15e..2185328 100644 --- a/internal/pagedoc/pagelinks_test.go +++ b/internal/pagedoc/pagelinks_test.go @@ -4,11 +4,11 @@ import ( "encoding/json" "fmt" "net/http" - "net/http/httptest" "strings" "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" "github.com/mozilla/markfluence/internal/convert" ) @@ -19,7 +19,7 @@ import ( func linkServer(t *testing.T, spaces map[string][]string) (*client.ConfluenceClient, *[]string) { t.Helper() var paths []string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { paths = append(paths, r.URL.Path+"?"+r.URL.RawQuery) q := r.URL.Query() @@ -46,9 +46,8 @@ func linkServer(t *testing.T, spaces map[string][]string) (*client.ConfluenceCli }) } _ = json.NewEncoder(w).Encode(map[string]any{"results": rows}) - })) - t.Cleanup(srv.Close) - return client.New(client.Config{SiteURL: srv.URL, Username: "u", Token: "t"}), &paths + }) + return c, &paths } // pageWithBody is a fetched page carrying a storage body and a webui link, which diff --git a/internal/pagetree/pagetree_test.go b/internal/pagetree/pagetree_test.go index d3f8564..799ba7e 100644 --- a/internal/pagetree/pagetree_test.go +++ b/internal/pagetree/pagetree_test.go @@ -4,11 +4,11 @@ import ( "encoding/json" "fmt" "net/http" - "net/http/httptest" "strings" "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" ) // node is a fixture row: what the fake server returns for one child. @@ -23,7 +23,7 @@ type node struct { func treeServer(t *testing.T, tree map[string][]node) (*client.ConfluenceClient, *int) { t.Helper() var calls int - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { calls++ parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") // .../content/{id}/child/{kind} @@ -50,9 +50,8 @@ func treeServer(t *testing.T, tree map[string][]node) (*client.ConfluenceClient, }) } _ = json.NewEncoder(w).Encode(map[string]any{"results": rows}) - })) - t.Cleanup(srv.Close) - return client.New(client.Config{SiteURL: srv.URL, Username: "u", Token: "t"}), &calls + }) + return c, &calls } // fixture: root holds a folder and two pages, interleaved by position; the From 44b98ba86806503826c1404e1e7242ba9525edf0 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:39:25 -0400 Subject: [PATCH 11/22] test(fix): cover plannedChanges, locatePage, and the write-only-on-change guarantee cmd/fix had no non-json test file: json_test.go only covered output shaping. Added direct tests for locatePage's page_id/title-search paths (found, not-found, zero/one/multiple title matches), plannedChanges' per-field diffing (fill-when-missing, update-when-differing, the parent-null normalization, title never overwritten, page_width's default-to-max and case-insensitive comparison), and processFile's central guarantee -- fix never writes the file unless there's a real change, and never writes at all under --dry-run or on failure. --- cmd/fix/fix_test.go | 385 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 385 insertions(+) create mode 100644 cmd/fix/fix_test.go diff --git a/cmd/fix/fix_test.go b/cmd/fix/fix_test.go new file mode 100644 index 0000000..78f2c1b --- /dev/null +++ b/cmd/fix/fix_test.go @@ -0,0 +1,385 @@ +package fix + +import ( + "fmt" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" +) + +// --- locatePage -------------------------------------------------------------- + +func pageJSON(id, title, parentID, webui string) string { + return fmt.Sprintf(`{"id":%q,"title":%q,"parentId":%q,"_links":{"webui":%q}}`, + id, title, parentID, webui) +} + +func TestLocatePageByIDFound(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/wiki/api/v2/pages/123" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + _, _ = w.Write([]byte(pageJSON("123", "Runbook", "", "/spaces/ENG/pages/123/Runbook"))) + }) + page, err := locatePage(map[string]string{"page_id": "123"}, c) + if err != nil { + t.Fatalf("locatePage: %v", err) + } + if page.ID != "123" || page.Title != "Runbook" { + t.Errorf("page = %+v, want id=123 title=Runbook", page) + } +} + +func TestLocatePageByIDNotFound(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"status":404,"title":"Cannot find a page with id 999"}]}`)) + }) + _, err := locatePage(map[string]string{"page_id": "999"}, c) + if err == nil { + t.Fatal("want an error for a page_id that resolves to nothing") + } + for _, want := range []string{"999", "not found", "remove it"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + +func TestLocatePageNoIDOrTitle(t *testing.T) { + // example.net never resolves to anything reachable, so a stray request would + // fail loudly rather than pass; this path must return before any request. + c := client.New(client.Config{SiteURL: "https://wiki.example.net"}) + _, err := locatePage(map[string]string{}, c) + if err == nil || !strings.Contains(err.Error(), "no page_id or title") { + t.Errorf("err = %v, want a no-page_id-or-title error", err) + } +} + +func TestLocatePageByTitleNoMatch(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"results":[]}`)) + }) + _, err := locatePage(map[string]string{"title": "Ghost"}, c) + if err == nil || !strings.Contains(err.Error(), `no Confluence page found with title "Ghost"`) { + t.Errorf("err = %v, want a no-match error naming the title", err) + } +} + +func TestLocatePageByTitleOneMatch(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasPrefix(r.URL.Path, "/wiki/api/v2/pages/"): + _, _ = w.Write([]byte(pageJSON("55", "Runbook", "", "/spaces/ENG/pages/55/Runbook"))) + default: + _, _ = w.Write([]byte(`{"results":[` + pageJSON("55", "Runbook", "", "/spaces/ENG/pages/55/Runbook") + `]}`)) + } + }) + page, err := locatePage(map[string]string{"title": "Runbook"}, c) + if err != nil { + t.Fatalf("locatePage: %v", err) + } + if page.ID != "55" { + t.Errorf("page.ID = %q, want 55 (the single match, re-fetched)", page.ID) + } +} + +func TestLocatePageByTitleMultipleMatchesDisambiguates(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"results":[` + + pageJSON("1", "Runbook", "", "/spaces/ENG/pages/1/Runbook") + `,` + + pageJSON("2", "Runbook", "", "/spaces/OPS/pages/2/Runbook") + `]}`)) + }) + _, err := locatePage(map[string]string{"title": "Runbook"}, c) + if err == nil { + t.Fatal("want an error when multiple pages share the title") + } + for _, want := range []string{"found 2 pages", "Runbook", "1", "2", "add a page_id"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + +// --- plannedChanges ------------------------------------------------------------ + +func TestPlannedChangesNoneWhenConsistent(t *testing.T) { + fm := map[string]string{ + "page_id": "123", "space": "ENG", "parent": "null", "title": "Runbook", "page_width": "max", + } + page := &client.Page{ID: "123", Title: "Runbook", Links: client.Links{WebUI: "/spaces/ENG/pages/123/Runbook"}} + got := plannedChanges(fm, page, "max") + if len(got) != 0 { + t.Errorf("changes = %+v, want none", got) + } +} + +func TestPlannedChangesFillsMissingFields(t *testing.T) { + page := &client.Page{ID: "123", Title: "Runbook", Links: client.Links{WebUI: "/spaces/ENG/pages/123/Runbook"}} + got := plannedChanges(map[string]string{}, page, "") + want := map[string]string{"page_id": "123", "space": "ENG", "parent": "null", "title": "Runbook"} + if len(got) != len(want) { + t.Fatalf("changes = %+v, want %d entries", got, len(want)) + } + for _, ch := range got { + if ch.oldDisplay != noneDisplay { + t.Errorf("field %s: old = %q, want %q", ch.field, ch.oldDisplay, noneDisplay) + } + if want[ch.field] != ch.newValue { + t.Errorf("field %s: new = %q, want %q", ch.field, ch.newValue, want[ch.field]) + } + } +} + +func TestPlannedChangesUpdatesFieldsThatDiffer(t *testing.T) { + fm := map[string]string{"page_id": "999", "space": "OLD", "parent": "1"} + page := &client.Page{ID: "123", ParentID: "2", Links: client.Links{WebUI: "/spaces/ENG/pages/123/Runbook"}} + got := plannedChanges(fm, page, "") + byField := map[string]change{} + for _, ch := range got { + byField[ch.field] = ch + } + if ch := byField["page_id"]; ch.oldDisplay != "999" || ch.newValue != "123" { + t.Errorf("page_id change = %+v, want 999 -> 123", ch) + } + if ch := byField["space"]; ch.oldDisplay != "OLD" || ch.newValue != "ENG" { + t.Errorf("space change = %+v, want OLD -> ENG", ch) + } + if ch := byField["parent"]; ch.oldDisplay != "1" || ch.newValue != "2" { + t.Errorf("parent change = %+v, want 1 -> 2", ch) + } +} + +func TestPlannedChangesParentNullNormalizes(t *testing.T) { + // A top-level live page (no ParentID) already recorded as "null" must not be + // treated as a diff -- orNull("") and the frontmatter's "null" must compare equal. + fm := map[string]string{"parent": "null"} + page := &client.Page{ID: "1", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} + got := plannedChanges(fm, page, "") + for _, ch := range got { + if ch.field == "parent" { + t.Errorf("parent change = %+v, want none (both sides are null)", ch) + } + } +} + +func TestPlannedChangesTitlePresentIsUntouched(t *testing.T) { + fm := map[string]string{"page_id": "1", "space": "ENG", "parent": "null", "title": "Kept"} + page := &client.Page{ID: "1", Title: "Live Title", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} + got := plannedChanges(fm, page, "") + for _, ch := range got { + if ch.field == "title" { + t.Errorf("title change = %+v, want none: an existing title is never overwritten", ch) + } + } +} + +func TestPlannedChangesSkipsWidthWhenLiveWidthUnknown(t *testing.T) { + fm := map[string]string{"page_id": "1", "space": "ENG", "parent": "null", "title": "X"} + page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} + got := plannedChanges(fm, page, "") + for _, ch := range got { + if ch.field == "page_width" { + t.Errorf("page_width change = %+v, want none when liveWidth is unknown", ch) + } + } +} + +func TestPlannedChangesWidthDefaultsToMaxWhenUnset(t *testing.T) { + fm := map[string]string{"page_id": "1", "space": "ENG", "parent": "null", "title": "X"} + page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} + + t.Run("live width already max: no change", func(t *testing.T) { + got := plannedChanges(fm, page, "max") + for _, ch := range got { + if ch.field == "page_width" { + t.Errorf("page_width change = %+v, want none: unset frontmatter defaults to max", ch) + } + } + }) + t.Run("live width differs: filled from (none)", func(t *testing.T) { + got := plannedChanges(fm, page, "narrow") + var found *change + for i, ch := range got { + if ch.field == "page_width" { + found = &got[i] + } + } + if found == nil || found.oldDisplay != noneDisplay || found.newValue != "narrow" { + t.Errorf("page_width change = %+v, want (none) -> narrow", found) + } + }) +} + +func TestPlannedChangesWidthCaseInsensitive(t *testing.T) { + fm := map[string]string{ + "page_id": "1", "space": "ENG", "parent": "null", "title": "X", "page_width": " Wide ", + } + page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} + got := plannedChanges(fm, page, "wide") + for _, ch := range got { + if ch.field == "page_width" { + t.Errorf("page_width change = %+v, want none: %q normalizes to wide", ch, fm["page_width"]) + } + } +} + +func TestPlannedChangesWidthDiffers(t *testing.T) { + fm := map[string]string{ + "page_id": "1", "space": "ENG", "parent": "null", "title": "X", "page_width": "narrow", + } + page := &client.Page{ID: "1", Title: "X", Links: client.Links{WebUI: "/spaces/ENG/pages/1/X"}} + got := plannedChanges(fm, page, "max") + var found *change + for i, ch := range got { + if ch.field == "page_width" { + found = &got[i] + } + } + if found == nil || found.oldDisplay != "narrow" || found.newValue != "max" { + t.Errorf("page_width change = %+v, want narrow -> max", found) + } +} + +// --- processFile: the write-only-on-a-real-change guarantee ------------------- + +func writeFixture(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "f.md") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// fixServer answers locatePage's page_id lookup and pagewidth.Read's +// content-property lookup for one page. +func fixServer(t *testing.T, page string, widthProperty string) *client.ConfluenceClient { + t.Helper() + return clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/properties"): + if widthProperty == "" { + _, _ = w.Write([]byte(`{"results":[]}`)) + return + } + _, _ = w.Write([]byte(`{"results":[{"value":` + widthProperty + `}]}`)) + default: + _, _ = w.Write([]byte(page)) + } + }) +} + +func TestProcessFileConsistentDoesNotWrite(t *testing.T) { + content := "---\npage_id: 1\nspace: ENG\nparent: null\ntitle: X\npage_width: max\n---\nbody\n" + path := writeFixture(t, content) + c := fixServer(t, pageJSON("1", "X", "", "/spaces/ENG/pages/1/X"), `"max"`) + + r := processFile(path, c) + if !r.ok || r.status != statusConsistent { + t.Fatalf("result = %+v, want ok/consistent", r) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != content { + t.Errorf("file was modified, want it untouched:\n%s", got) + } +} + +func TestProcessFileDryRunDoesNotWrite(t *testing.T) { + content := "---\npage_id: 1\ntitle: X\n---\nbody\n" + path := writeFixture(t, content) + c := fixServer(t, pageJSON("1", "X", "", "/spaces/ENG/pages/1/X"), `"max"`) + + dryRun = true + t.Cleanup(func() { dryRun = false }) + + r := processFile(path, c) + if !r.ok || r.status != statusChanged || len(r.changes) == 0 { + t.Fatalf("result = %+v, want ok/changed with a nonempty diff", r) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != content { + t.Errorf("dry-run modified the file, want it untouched:\n%s", got) + } +} + +func TestProcessFileWritesOnRealChange(t *testing.T) { + content := "---\npage_id: 123\ntitle: X\n---\nbody\n" + path := writeFixture(t, content) + c := fixServer(t, pageJSON("123", "X", "", "/spaces/ENG/pages/123/X"), `"max"`) + + r := processFile(path, c) + if !r.ok || r.status != statusChanged { + t.Fatalf("result = %+v, want ok/changed", r) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(got), "page_id: 123") { + t.Errorf("file = %q, want it to record the resolved page_id", got) + } + if !strings.Contains(string(got), "space: ENG") { + t.Errorf("file = %q, want it to record the resolved space", got) + } +} + +func TestProcessFileFailsWhenPageNotFound(t *testing.T) { + content := "---\npage_id: 999\ntitle: X\n---\nbody\n" + path := writeFixture(t, content) + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"status":404,"title":"Cannot find a page with id 999"}]}`)) + }) + + r := processFile(path, c) + if r.ok { + t.Fatal("want a failure when the page_id resolves to nothing") + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != content { + t.Errorf("file was modified on failure, want it untouched:\n%s", got) + } +} + +// --- norm / orNull ------------------------------------------------------------- + +func TestNorm(t *testing.T) { + cases := map[string]string{ + "": "", + " ": "", + "null": "", + " null ": "", + "ENG": "ENG", + " ENG ": "ENG", + "Null": "Null", // only the literal lowercase "null" is a sentinel + } + for in, want := range cases { + if got := norm(in); got != want { + t.Errorf("norm(%q) = %q, want %q", in, got, want) + } + } +} + +func TestOrNull(t *testing.T) { + if got := orNull(""); got != "null" { + t.Errorf(`orNull("") = %q, want "null"`, got) + } + if got := orNull("123"); got != "123" { + t.Errorf(`orNull("123") = %q, want "123"`, got) + } +} From 77fd1662a0625322d376a2fb0e25b191dcacca83 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:42:05 -0400 Subject: [PATCH 12/22] test(update): cover the mtime-skip guarantee and a full publish success path update's most distinctive behavior -- skip republishing unless the file was modified after the page's last version, unless --force -- had no test driving processFile through a real skip decision; every existing test hand-built an updateResult{status: statusSkipped} instead. Added tests for the skip itself, --force bypassing it, and a full success path (through UpdatePage) that also covers the file-newer-than-page half of the same check. Migrated the file's existing httptest usage to internal/clienttest in the same pass. --- cmd/update/update_test.go | 134 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 128 insertions(+), 6 deletions(-) diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index aa867c6..99a46d3 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -1,14 +1,17 @@ package update import ( + "encoding/json" + "fmt" "net/http" - "net/http/httptest" "os" "path/filepath" "strings" "testing" + "time" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" "github.com/mozilla/markfluence/internal/frontmatter" "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/linkindex" @@ -128,14 +131,13 @@ func TestProcessFileRejectsNonNumericPageID(t *testing.T) { // TestProcessFileReportsMissingPage is the issue itself: a page_id the server // answers 404 for used to surface as "GET https://...: HTTP 404: {...}". func TestProcessFileReportsMissingPage(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/wiki/api/v2/pages/999" { t.Errorf("unexpected request: %s %s", r.Method, r.URL) } w.WriteHeader(http.StatusNotFound) _, _ = w.Write([]byte(`{"errors":[{"status":404,"code":"NOT_FOUND"}]}`)) - })) - defer srv.Close() + }) dir := t.TempDir() path := filepath.Join(dir, "f.md") @@ -143,7 +145,7 @@ func TestProcessFileReportsMissingPage(t *testing.T) { t.Fatalf("writing fixture: %v", err) } - r := processFile(path, client.New(client.Config{SiteURL: srv.URL}), project.NewCache(""), linkindex.NewCache()) + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) if r.ok { t.Fatal("a page_id that resolves to nothing must fail the file") } @@ -153,7 +155,7 @@ func TestProcessFileReportsMissingPage(t *testing.T) { t.Errorf("errMsg =\n %q\nwant\n %q", r.errMsg, want) } // The raw transport error must not leak: no method, URL, or response body. - for _, unwanted := range []string{"HTTP 404", "GET ", srv.URL, "errors"} { + for _, unwanted := range []string{"HTTP 404", "GET ", c.SiteURL(), "errors"} { if strings.Contains(r.errMsg, unwanted) { t.Errorf("errMsg = %q, should not contain %q", r.errMsg, unwanted) } @@ -162,3 +164,123 @@ func TestProcessFileReportsMissingPage(t *testing.T) { t.Errorf("code = %q, want %q", r.code, jsonout.CodeNotFound) } } + +// pageWithVersion builds a minimal page fixture with a given version number and +// createdAt, which is what processFile's mtime-skip check compares the file +// against. +func pageWithVersion(id string, versionNumber int, createdAt string) string { + return fmt.Sprintf( + `{"id":%q,"title":"Old Title","version":{"number":%d,"createdAt":%q},`+ + `"_links":{"webui":"/spaces/ENG/pages/%s/Old+Title"}}`, + id, versionNumber, createdAt, id) +} + +func writeUpdateFixture(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "f.md") + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +// TestProcessFilePublishesSuccessfully is the full happy path: the file's mtime +// is "now" (just written), which is after the page's 2020 version, so this also +// covers the file-newer-than-page half of the mtime check. +func TestProcessFilePublishesSuccessfully(t *testing.T) { + var sawPut bool + var putVersion int + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2020-01-01T00:00:00Z"))) + case http.MethodPut: + sawPut = true + var body struct { + Version struct { + Number int `json:"number"` + } `json:"version"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + putVersion = body.Version.Number + _, _ = w.Write([]byte(pageWithVersion("1", body.Version.Number, "2026-01-01T00:00:00Z"))) + default: + t.Errorf("unexpected method: %s", r.Method) + } + }) + + path := writeUpdateFixture(t, "---\npage_id: 1\n---\nHello.\n") + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok || r.status != statusPublished { + t.Fatalf("result = %+v, want ok/published", r) + } + if !sawPut { + t.Fatal("want UpdatePage to have been called") + } + if putVersion != 4 { + t.Errorf("PUT carried version = %d, want 4 (previous 3 + 1)", putVersion) + } + if r.versionNew != 4 { + t.Errorf("r.versionNew = %d, want 4", r.versionNew) + } +} + +// TestProcessFileSkipsWhenFileOlderThanPage is the mtime-skip guarantee itself: +// a file not modified since the page's last version must not republish. +func TestProcessFileSkipsWhenFileOlderThanPage(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + t.Errorf("unexpected %s request: a skip must not touch the page further", r.Method) + } + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2099-01-01T00:00:00Z"))) + }) + + path := writeUpdateFixture(t, "---\npage_id: 1\n---\nHello.\n") + past := time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC) + if err := os.Chtimes(path, past, past); err != nil { + t.Fatal(err) + } + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok || r.status != statusSkipped { + t.Fatalf("result = %+v, want ok/skipped", r) + } + if r.versionNew != 3 { + t.Errorf("r.versionNew = %d, want 3 (unchanged by a skip)", r.versionNew) + } +} + +// TestProcessFileForceBypassesMtimeSkip: --force publishes even when the file +// looks unchanged since the page's last version. +func TestProcessFileForceBypassesMtimeSkip(t *testing.T) { + var sawPut bool + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodGet: + _, _ = w.Write([]byte(pageWithVersion("1", 3, "2099-01-01T00:00:00Z"))) + case http.MethodPut: + sawPut = true + _, _ = w.Write([]byte(pageWithVersion("1", 4, "2099-01-01T00:00:00Z"))) + default: + t.Errorf("unexpected method: %s", r.Method) + } + }) + + path := writeUpdateFixture(t, "---\npage_id: 1\n---\nHello.\n") + past := time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC) + if err := os.Chtimes(path, past, past); err != nil { + t.Fatal(err) + } + + force = true + t.Cleanup(func() { force = false }) + + r := processFile(path, c, project.NewCache(""), linkindex.NewCache()) + if !r.ok || r.status != statusPublished { + t.Fatalf("result = %+v, want ok/published: --force bypasses the mtime skip", r) + } + if !sawPut { + t.Fatal("want UpdatePage to have been called despite the old mtime") + } +} From 50f43967283bfb960571ecd2cfdba675083434b4 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:45:52 -0400 Subject: [PATCH 13/22] test(create): cover topoSort's cycle rejection, --parent, and resolveFile's own validation Three gaps: topoSort's parent-cycle detection was never actually triggered by any test (buildRecords always t.Fatalf's on error); the plain "--parent " (parentExternal) path -- as opposed to a .md reference -- was never exercised at all; and resolveFile's own validation errors (no title, no space, --space/frontmatter conflict) were only ever used as arbitrary example strings for testing the abort/JSON machinery, never asserted as resolveFile's actual output. --- cmd/create/create_test.go | 84 +++++++++++++++++++++++++++++++++++++++ cmd/create/parent_test.go | 50 +++++++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/cmd/create/create_test.go b/cmd/create/create_test.go index a6e6c2b..1742a80 100644 --- a/cmd/create/create_test.go +++ b/cmd/create/create_test.go @@ -9,7 +9,9 @@ import ( "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/frontmatter" + "github.com/mozilla/markfluence/internal/linkindex" "github.com/mozilla/markfluence/internal/pagewidth" + "github.com/mozilla/markfluence/internal/project" ) // testClient is a client that talks to nothing: it exists so URL-building and the @@ -319,3 +321,85 @@ func TestCheckTitleFreeSeesArchivedPages(t *testing.T) { } }) } + +// --- resolveFile's own validation branches ------------------------------------ + +func TestResolveFileNoTitle(t *testing.T) { + dir := t.TempDir() + path := write(t, dir, "a.md", "no frontmatter here\n") + roots := project.NewCache("") + t.Cleanup(roots.Close) + + _, err := resolveFile(path, testClient(), map[string]bool{}, map[string]string{}, roots, linkindex.NewCache()) + if err == nil || !strings.Contains(err.Error(), "no title given") { + t.Errorf("err = %v, want a no-title error", err) + } +} + +func TestResolveFileNoSpace(t *testing.T) { + dir := t.TempDir() + path := write(t, dir, "a.md", "---\ntitle: X\n---\nbody\n") + roots := project.NewCache("") + t.Cleanup(roots.Close) + + _, err := resolveFile(path, testClient(), map[string]bool{}, map[string]string{}, roots, linkindex.NewCache()) + if err == nil || !strings.Contains(err.Error(), "no space given") { + t.Errorf("err = %v, want a no-space error", err) + } +} + +func TestResolveFileSpaceConflict(t *testing.T) { + dir := t.TempDir() + path := write(t, dir, "a.md", "---\ntitle: X\nspace: OPS\n---\nbody\n") + roots := project.NewCache("") + t.Cleanup(roots.Close) + + spaceOpt = "ENG" + t.Cleanup(func() { spaceOpt = "" }) + + _, err := resolveFile(path, testClient(), map[string]bool{}, map[string]string{}, roots, linkindex.NewCache()) + want := `--space "ENG" conflicts with frontmatter space "OPS"` + if err == nil || err.Error() != want { + t.Errorf("err = %v, want %q", err, want) + } +} + +// --- topoSort ------------------------------------------------------------------ + +// TestTopoSortDetectsCycle: two in-set files naming each other as parent form a +// graph with no valid order at all, which must be rejected outright rather than +// left to loop or silently drop one side. +func TestTopoSortDetectsCycle(t *testing.T) { + a := record{filename: "a.md", absPath: "/docs/a.md", parent: parentInfo{kind: parentInSet, abs: "/docs/b.md"}} + b := record{filename: "b.md", absPath: "/docs/b.md", parent: parentInfo{kind: parentInSet, abs: "/docs/a.md"}} + byAbs := map[string]record{a.absPath: a, b.absPath: b} + + _, err := topoSort([]record{a, b}, byAbs) + if err == nil || !strings.Contains(err.Error(), "parent cycle detected") { + t.Errorf("err = %v, want a parent-cycle error", err) + } +} + +// TestTopoSortOrdersParentsBeforeChildren: fed in child-first input order, the +// output must still place each in-set parent before the child that names it. +func TestTopoSortOrdersParentsBeforeChildren(t *testing.T) { + grandchild := record{filename: "c.md", absPath: "/docs/c.md", parent: parentInfo{kind: parentInSet, abs: "/docs/b.md"}} + child := record{filename: "b.md", absPath: "/docs/b.md", parent: parentInfo{kind: parentInSet, abs: "/docs/a.md"}} + parent := record{filename: "a.md", absPath: "/docs/a.md"} + byAbs := map[string]record{ + grandchild.absPath: grandchild, child.absPath: child, parent.absPath: parent, + } + + // Deliberately out of order: grandchild, child, parent. + got, err := topoSort([]record{grandchild, child, parent}, byAbs) + if err != nil { + t.Fatalf("topoSort: %v", err) + } + pos := map[string]int{} + for i, r := range got { + pos[r.absPath] = i + } + if pos[parent.absPath] > pos[child.absPath] || pos[child.absPath] > pos[grandchild.absPath] { + t.Errorf("order = %v, want parent before child before grandchild", got) + } +} diff --git a/cmd/create/parent_test.go b/cmd/create/parent_test.go index 7cd0270..fccbfe2 100644 --- a/cmd/create/parent_test.go +++ b/cmd/create/parent_test.go @@ -91,6 +91,56 @@ func TestResolveParentMdFilePublished(t *testing.T) { } } +// TestResolveParentExternalID is the plain "--parent 500" case: a bare id, +// never a .md path, resolved straight through checkParentInSpace. +func TestResolveParentExternalID(t *testing.T) { + root := rootFor(t, t.TempDir()) + c := parentServer(t, map[string]string{"500": `{"id":"500","spaceId":"space1"}`}, nil) + + parentOpt = "500" + t.Cleanup(func() { parentOpt = "" }) + + p, err := resolveParent(filepath.Join(root.Dir, "a.md"), map[string]string{}, nil, c, "space1", root) + if err != nil { + t.Fatal(err) + } + if p.kind != parentExternal || p.id != "500" || p.parentType != "page" { + t.Errorf("p = %+v, want kind=external id=500 parentType=page", p) + } +} + +// TestResolveParentExternalFolderID: --parent works identically for a folder, +// which every v2 page route would 404 on -- the whole point of #68. +func TestResolveParentExternalFolderID(t *testing.T) { + root := rootFor(t, t.TempDir()) + c := parentServer(t, nil, map[string]string{"600": `{"id":"600","type":"folder","spaceId":"space1"}`}) + + parentOpt = "600" + t.Cleanup(func() { parentOpt = "" }) + + p, err := resolveParent(filepath.Join(root.Dir, "a.md"), map[string]string{}, nil, c, "space1", root) + if err != nil { + t.Fatal(err) + } + if p.kind != parentExternal || p.id != "600" || p.parentType != "folder" { + t.Errorf("p = %+v, want kind=external id=600 parentType=folder", p) + } +} + +// TestResolveParentBothSetIsAnError: --parent and a frontmatter parent can't +// both name the parent, since one would silently win over the other. +func TestResolveParentBothSetIsAnError(t *testing.T) { + root := rootFor(t, t.TempDir()) + parentOpt = "500" + t.Cleanup(func() { parentOpt = "" }) + + _, err := resolveParent(filepath.Join(root.Dir, "a.md"), + map[string]string{"parent": "other.md"}, nil, nil, "", root) + if err == nil || !strings.Contains(err.Error(), "both --parent and a frontmatter 'parent' are set") { + t.Errorf("err = %v, want the both-set conflict error", err) + } +} + // TestResolveParentEscapingRootIsHardError is S2 for parent: -- a parent // outside the documentation root is refused outright, not left unresolved and // reported the way an escaping link is: a parent is load-bearing, and From 4fe50f7837b98e2e3db43a57d040069623fef685 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 12:47:37 -0400 Subject: [PATCH 14/22] fix(attachfile): remove a partial file left behind by a failed download Write's Create truncates (or makes) the destination file before DownloadAttachment runs. On failure the file was left in place -- empty, or holding whatever was written before the error -- so a retry without --force saw it via the existence check and reported skipped, silently treating a download that never completed as "already there." Found while adding test coverage for this path; the accompanying test reproduces the bug and confirms the fix. --- internal/attachfile/attachfile.go | 6 ++++++ internal/attachfile/attachfile_test.go | 28 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/internal/attachfile/attachfile.go b/internal/attachfile/attachfile.go index 8cf7c57..ceeecb8 100644 --- a/internal/attachfile/attachfile.go +++ b/internal/attachfile/attachfile.go @@ -192,6 +192,12 @@ func Write(c *client.ConfluenceClient, a client.Attachment, opts Options) Outcom } defer func() { _ = f.Close() }() if err := c.DownloadAttachment(a, f); err != nil { + // Create truncated (or made) the file before the download failed, so it + // now exists with partial or no content. Left in place, the next run's + // os.Stat above would see it and report a skip -- "already there" -- + // masking a download that never actually completed. + _ = f.Close() + _ = rootFS.Remove(rel) res.Status, res.Err, res.Code = StatusFailed, err, jsonout.CodeFor(err) return res } diff --git a/internal/attachfile/attachfile_test.go b/internal/attachfile/attachfile_test.go index 9f95537..667dde2 100644 --- a/internal/attachfile/attachfile_test.go +++ b/internal/attachfile/attachfile_test.go @@ -172,6 +172,34 @@ func TestWriteSkipsExistingUnlessForce(t *testing.T) { } } +// TestWriteRemovesPartialFileOnDownloadFailure: Create makes (or truncates) the +// destination before the download runs, so a failure partway through -- or +// before a single byte is written, as here -- must not leave that file behind. +// Left in place, the next run's own-existence check would see it and report a +// skip, silently masking a download that never actually completed. +func TestWriteRemovesPartialFileOnDownloadFailure(t *testing.T) { + root := t.TempDir() + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + att := withDownload(client.Attachment{Title: "x.png"}) + + got := Write(c, att, Options{Root: root}) + if got.Status != StatusFailed { + t.Fatalf("status = %q, want failed", got.Status) + } + if _, err := os.Stat(filepath.Join(root, "x.png")); !os.IsNotExist(err) { + t.Errorf("a failed download left a file behind (err=%v); a retry would see it and skip", err) + } + + // The retry itself: without the fix, this would report skipped instead of + // trying again. + c2 := testClient(t, "BYTES") + if got := Write(c2, att, Options{Root: root}); got.Status != StatusDownloaded { + t.Errorf("retry status = %q, want downloaded (not skipped over the earlier failure)", got.Status) + } +} + func TestWriteDryRunCreatesNothing(t *testing.T) { root := t.TempDir() c := testClient(t, "BYTES") From 80dde9a4162d182faf3607a8be0299efa5c5b47b Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:31:19 -0400 Subject: [PATCH 15/22] test(client): cover GetUser, CreatePage's parentId, and the credential-rejection integration The credential-rejection-vs-404 distinction (notFound/RejectedCredential) had a unit test for the detection logic itself but nothing feeding that exact response shape through GetPageOrNil/GetPageBodyOrNil/GetFolderOrNil to confirm they actually propagate the error instead of reading it as "page absent" -- the specific integration notFound exists to protect. Also added GetUser (display-name lookup, empty-id short-circuit, best-effort-empty-on-failure) and CreatePage's conditional parentId field, neither of which had any coverage. --- internal/client/client_test.go | 91 ++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index cfb6f33..01189c1 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -2,6 +2,7 @@ package client import ( "bytes" + "encoding/json" "errors" "fmt" "io" @@ -355,6 +356,32 @@ func TestGetPagePropagatesError(t *testing.T) { } } +// TestGetPageOrNilReportsRejectedCredentialAsError is the integration notFound +// exists for: a revoked token's 404 must not be swallowed as "page absent," or +// every id looks not-found regardless of the real problem. +func TestGetPageOrNilReportsRejectedCredentialAsError(t *testing.T) { + c, _ := newServer(t, resp{404, `{"title":"Not Found"}`}) + p, err := c.GetPageOrNil("1") + if err == nil { + t.Fatal("GetPageOrNil: want an error for a rejected-credential 404, not nil/nil") + } + if p != nil { + t.Errorf("GetPageOrNil returned a page alongside the error: %+v", p) + } + var he *HTTPError + if !errors.As(err, &he) || !he.RejectedCredential() { + t.Errorf("err = %v, want an HTTPError whose RejectedCredential() is true", err) + } +} + +func TestGetPageBodyOrNilReportsRejectedCredentialAsError(t *testing.T) { + c, _ := newServer(t, resp{404, `{"title":"Not Found"}`}) + p, err := c.GetPageBodyOrNil("1") + if err == nil || p != nil { + t.Fatalf("GetPageBodyOrNil = %v, %v; want nil page and an error", p, err) + } +} + func TestPageURLUsesSiteNotGateway(t *testing.T) { c := New(Config{SiteURL: "https://wiki.example.net", CloudID: "abc-123"}) @@ -422,6 +449,18 @@ func TestGetFolderOrNilReturnsNilOn404(t *testing.T) { } } +func TestGetFolderOrNilReportsRejectedCredentialAsError(t *testing.T) { + c, _ := newServer(t, resp{404, `{"title":"Not Found"}`}) + f, err := c.GetFolderOrNil("9") + if err == nil || f != nil { + t.Fatalf("GetFolderOrNil = %v, %v; want nil folder and an error", f, err) + } + var he *HTTPError + if !errors.As(err, &he) || !he.RejectedCredential() { + t.Errorf("err = %v, want an HTTPError whose RejectedCredential() is true", err) + } +} + // TestGetPageReportsParentType pins the field that distinguishes a folder parent // from a page parent without a second request. func TestGetPageReportsParentType(t *testing.T) { @@ -752,6 +791,58 @@ func TestDownloadAttachmentDoesNotLeakCredentialsOnRedirect(t *testing.T) { } } +// --- user / page creation ------------------------------------------------------ + +func TestGetUserReturnsDisplayName(t *testing.T) { + c, _ := newServer(t, resp{200, `{"displayName":"Ada Lovelace"}`}) + if got := c.GetUser("abc123"); got != "Ada Lovelace" { + t.Errorf("GetUser = %q, want Ada Lovelace", got) + } +} + +func TestGetUserEmptyAccountIDMakesNoRequest(t *testing.T) { + c, s := newServer(t) + if got := c.GetUser(""); got != "" { + t.Errorf("GetUser(\"\") = %q, want empty", got) + } + if len(s.calls) != 0 { + t.Errorf("calls = %v, want none for an empty account id", s.calls) + } +} + +func TestGetUserReturnsEmptyOnFailure(t *testing.T) { + c, _ := newServer(t, resp{500, `boom`}) + if got := c.GetUser("abc123"); got != "" { + t.Errorf("GetUser = %q, want empty on a failed lookup (best-effort)", got) + } +} + +func TestCreatePageOmitsParentIDWhenEmpty(t *testing.T) { + c, s := newServer(t, resp{200, `{"id":"1"}`}) + if _, err := c.CreatePage("space1", "Title", "

x

", ""); err != nil { + t.Fatal(err) + } + if strings.Contains(s.lastBody(), "parentId") { + t.Errorf("request body = %q, must not carry parentId for a top-level page", s.lastBody()) + } +} + +func TestCreatePageIncludesParentIDWhenSet(t *testing.T) { + c, s := newServer(t, resp{200, `{"id":"1"}`}) + if _, err := c.CreatePage("space1", "Title", "

x

", "999"); err != nil { + t.Fatal(err) + } + var body struct { + ParentID string `json:"parentId"` + } + if err := json.Unmarshal([]byte(s.lastBody()), &body); err != nil { + t.Fatalf("unmarshaling request body: %v", err) + } + if body.ParentID != "999" { + t.Errorf("parentId = %q, want 999", body.ParentID) + } +} + // --- misc -------------------------------------------------------------------- func TestLoadDotenv(t *testing.T) { From 8fc36a0b72c8ed7879ec1ab6ee75ff6af47588bc Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:40:32 -0400 Subject: [PATCH 16/22] test(client): cover retry/backoff edges, isIdempotent, and SpaceKeyFromWebUI Adds: attempt-count pinning for a persistently unreachable server (maxRetries+1 total) and confirmation a non-idempotent POST gets exactly one attempt on a transport failure; a retried POST resending the same body (verified true today via net/http's own GetBody handling for the bytes.Reader/bytes.Buffer types this client always uses, not attempt's explicit rebuild -- confirmed by mutation testing, comment corrected to say so); parseRetryAfter's future-HTTP-date branch, which the existing table only exercised with a fixed past date; isIdempotent's method table; SetContentProperty returning the second (not first) error when both attempts fail; and SpaceKeyFromWebUI's non-matching/empty cases. --- internal/client/client_test.go | 74 ++++++++++++++++++++++++++++++++ internal/client/config_test.go | 15 +++++++ internal/client/retrylog_test.go | 37 ++++++++++++++++ 3 files changed, 126 insertions(+) diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 01189c1..96c090b 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -157,6 +157,26 @@ func TestSetContentPropertyRetriesOnceAndDetectsApplied(t *testing.T) { } } +// TestSetContentPropertyReturnsSecondErrorWhenBothAttemptsFail: lastErr must be +// the retry's error, not the first attempt's -- the one a caller can still act +// on, since the first is already stale by the time it's reported. +func TestSetContentPropertyReturnsSecondErrorWhenBothAttemptsFail(t *testing.T) { + // Plain 500s with no Retry-After: neither is retried by send's own loop + // (retryableStatus requires 502/503/504, 429, or a Retry-After header), so + // exactly two requests happen -- SetContentProperty's own two attempts. + c, s := newServer(t, resp{500, `first`}, resp{500, `second`}) + _, err := c.SetContentProperty("1", "k", "max") + if err == nil { + t.Fatal("want an error when both attempts fail") + } + if !strings.Contains(err.Error(), "second") { + t.Errorf("err = %v, want it to carry the second (retry) attempt's body", err) + } + if !eqStrings(s.calls, []string{"GET", "GET"}) { + t.Errorf("calls = %v, want two attempts", s.calls) + } +} + func TestListContentPropertiesFollowsPagination(t *testing.T) { c, s := newServer(t, resp{200, `{"results":[{"key":"a"}],"_links":{"next":"/wiki/api/v2/pages/1/properties?cursor=X"}}`}, @@ -248,6 +268,29 @@ func TestSendRetriesPostOn429(t *testing.T) { } } +// TestSendRetriesPostPreservesBody: a retried POST must resend the same body, +// not an empty one from an already-drained reader. Every request body in this +// client is built as a *bytes.Reader (doJSON) or *bytes.Buffer (uploadAttachment), +// both of which net/http.NewRequest gives an automatic GetBody -- confirmed by +// mutation testing to be what actually provides this guarantee; attempt's own +// explicit rebuild is redundant for these two body types but is kept as an +// explicit, visible contract rather than relying on that stdlib behavior. +func TestSendRetriesPostPreservesBody(t *testing.T) { + c, s := newServer(t, resp{429, `{}`}, resp{200, `{"id":"9"}`}) + if _, err := c.CreatePage("space1", "Title", "

hello

", ""); err != nil { + t.Fatalf("CreatePage after 429 retry: %v", err) + } + if len(s.bodies) != 2 { + t.Fatalf("got %d requests, want 2 (one retry)", len(s.bodies)) + } + if s.bodies[0] == "" { + t.Fatal("first attempt sent an empty body") + } + if s.bodies[1] != s.bodies[0] { + t.Errorf("retried body = %q, want it identical to the first attempt's %q", s.bodies[1], s.bodies[0]) + } +} + func TestSendExhaustsRetries(t *testing.T) { c, n := countingServer(t, func(w http.ResponseWriter, _ int32) { w.Header().Set("Retry-After", "0") @@ -287,6 +330,37 @@ func TestParseRetryAfter(t *testing.T) { } } +// TestParseRetryAfterFutureHTTPDate covers the d > 0 branch TestParseRetryAfter's +// table can't: a past date is fixed and always exercises d <= 0 (retry now), so +// the future-date case needs a value computed at test time. +func TestParseRetryAfterFutureHTTPDate(t *testing.T) { + future := time.Now().Add(90 * time.Second).UTC().Format(http.TimeFormat) + d, ok := parseRetryAfter(future) + if !ok { + t.Fatalf("parseRetryAfter(%q) ok = false, want true", future) + } + // Allow slack for the time elapsed between formatting and parsing. + if d < 85*time.Second || d > 90*time.Second { + t.Errorf("parseRetryAfter(%q) = %v, want roughly 90s", future, d) + } +} + +func TestIsIdempotent(t *testing.T) { + for method, want := range map[string]bool{ + http.MethodGet: true, + http.MethodHead: true, + http.MethodPut: true, + http.MethodDelete: true, + http.MethodOptions: true, + http.MethodPost: false, + http.MethodPatch: false, + } { + if got := isIdempotent(method); got != want { + t.Errorf("isIdempotent(%s) = %v, want %v", method, got, want) + } + } +} + func TestBackoffCapsAndHonorsRetryAfter(t *testing.T) { if d := backoff(0, 0); d != baseBackoff { t.Errorf("backoff(0,0) = %v, want %v", d, baseBackoff) diff --git a/internal/client/config_test.go b/internal/client/config_test.go index dbfaeeb..cd188c3 100644 --- a/internal/client/config_test.go +++ b/internal/client/config_test.go @@ -210,3 +210,18 @@ func TestResolveWithoutCloudIDKeepsSiteURL(t *testing.T) { t.Errorf("BaseURL/SiteURL = %q/%q, want https://wiki for both", c.BaseURL(), c.SiteURL()) } } + +func TestSpaceKeyFromWebUI(t *testing.T) { + if got := SpaceKeyFromWebUI("/spaces/ENG/pages/123/Title"); got != "ENG" { + t.Errorf("space = %q, want ENG", got) + } + if got := SpaceKeyFromWebUI("/spaces/ENG/folder/123"); got != "ENG" { + t.Errorf("space (folder) = %q, want ENG", got) + } + if got := SpaceKeyFromWebUI("/wiki/pages/viewpage.action?pageId=123"); got != "" { + t.Errorf("space = %q, want empty for a webui link with no /spaces/ prefix", got) + } + if got := SpaceKeyFromWebUI(""); got != "" { + t.Errorf("space = %q, want empty for an empty webui", got) + } +} diff --git a/internal/client/retrylog_test.go b/internal/client/retrylog_test.go index e3c6bd3..e7a3b70 100644 --- a/internal/client/retrylog_test.go +++ b/internal/client/retrylog_test.go @@ -157,6 +157,43 @@ func TestRetryLoggerReportsATransportFailure(t *testing.T) { } } +// TestSendExhaustsRetriesOnTransportFailure pins the attempt count for a +// persistently unreachable server: maxRetries retries after the first attempt, +// maxRetries+1 total, then give up -- not fewer (a regression that stopped +// retrying transport failures early) and not more (a regression that ignored +// the cap and retried forever). +func TestSendExhaustsRetriesOnTransportFailure(t *testing.T) { + events := captureRetries(t) + c := New(Config{SiteURL: "http://127.0.0.1:1", Username: "u", Token: "t"}) + if _, err := c.GetPage("1"); err == nil { + t.Fatal("GetPage: want a transport error") + } + if got := len(*events); got != maxRetries+1 { + t.Fatalf("got %d attempts, want %d (maxRetries+1)", got, maxRetries+1) + } + last := (*events)[len(*events)-1] + if last.Retrying { + t.Error("final attempt says Retrying, want false: retries are exhausted") + } +} + +// TestSendTransportFailureOnNonIdempotentMethodDoesNotRetry: a POST that never +// reached the server at all must not be retried, since retrying could mean +// executing it twice if the first attempt actually landed. +func TestSendTransportFailureOnNonIdempotentMethodDoesNotRetry(t *testing.T) { + events := captureRetries(t) + c := New(Config{SiteURL: "http://127.0.0.1:1", Username: "u", Token: "t"}) + if _, err := c.CreatePage("space1", "Title", "

x

", ""); err == nil { + t.Fatal("CreatePage: want a transport error") + } + if got := len(*events); got != 1 { + t.Fatalf("got %d attempts, want exactly 1 (POST is not idempotent)", got) + } + if (*events)[0].Retrying { + t.Error("Retrying = true, want false for a non-idempotent method") + } +} + // TestSetRetryLoggerNilIsSafe: the logger is unset in every command's tests and // in library use. func TestSetRetryLoggerNilIsSafe(t *testing.T) { From ea1e1c9c47ca991e2fb440912c8627dd84cebfce Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:42:40 -0400 Subject: [PATCH 17/22] test(linkindex): cover Cache's memoization and the slug/heading-extraction functions Cache -- the whole point of the type -- had no test file at all; verified it builds once per root.Dir and reuses that index across later Get calls (proven by mutating the tree on disk between two Get calls and checking the second doesn't see the change), keeps separate roots separate, and propagates a build error. GithubSlug/ConfluenceSlug/ extractHeadings had no direct test anywhere despite Unicode-aware slugging being a documented design point: added cases for Unicode letters, punctuation stripping, the per-character-vs-collapsed whitespace-to-hyphen difference between the two sluggers, and extractHeadings' fenced-code-block skip (including an unterminated fence). --- internal/linkindex/cache_test.go | 76 +++++++++++++++++++++++++++++++ internal/linkindex/slug_test.go | 77 ++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 internal/linkindex/cache_test.go create mode 100644 internal/linkindex/slug_test.go diff --git a/internal/linkindex/cache_test.go b/internal/linkindex/cache_test.go new file mode 100644 index 0000000..0346c59 --- /dev/null +++ b/internal/linkindex/cache_test.go @@ -0,0 +1,76 @@ +package linkindex + +import ( + "path/filepath" + "testing" +) + +// TestCacheBuildsOncePerRoot is the entire point of Cache: a second Get for the +// same root.Dir must return the index already built, not rebuild it -- proven +// by changing the tree on disk between the two calls and confirming the second +// call doesn't see the change. +func TestCacheBuildsOncePerRoot(t *testing.T) { + root := rootAt(t, t.TempDir()) + write(t, filepath.Join(root.Dir, "a.md"), "---\npage_id: 1\ntitle: A\n---\nbody\n") + + c := NewCache() + idx1, err := c.Get(root) + if err != nil { + t.Fatal(err) + } + if _, ok := idx1.Page("a.md"); !ok { + t.Fatal("want a.md indexed on the first build") + } + + write(t, filepath.Join(root.Dir, "b.md"), "---\npage_id: 2\ntitle: B\n---\nbody\n") + + idx2, err := c.Get(root) + if err != nil { + t.Fatal(err) + } + if idx2 != idx1 { + t.Error("Get returned a different *Index for the same root, want the cached one") + } + if _, ok := idx2.Page("b.md"); ok { + t.Error("second Get saw a file added after the first build, want the cached (stale) index") + } +} + +// TestCacheBuildsSeparatelyPerRoot: two distinct roots must not share an index +// -- the memoization key is root.Dir, not "the first root ever seen." +func TestCacheBuildsSeparatelyPerRoot(t *testing.T) { + rootA := rootAt(t, t.TempDir()) + write(t, filepath.Join(rootA.Dir, "a.md"), "---\npage_id: 1\ntitle: A\n---\nbody\n") + rootB := rootAt(t, t.TempDir()) + write(t, filepath.Join(rootB.Dir, "b.md"), "---\npage_id: 2\ntitle: B\n---\nbody\n") + + c := NewCache() + idxA, err := c.Get(rootA) + if err != nil { + t.Fatal(err) + } + idxB, err := c.Get(rootB) + if err != nil { + t.Fatal(err) + } + if idxA == idxB { + t.Fatal("two different roots got the same *Index") + } + if _, ok := idxA.Page("b.md"); ok { + t.Error("rootA's index contains rootB's file") + } + if _, ok := idxB.Page("a.md"); ok { + t.Error("rootB's index contains rootA's file") + } +} + +func TestCacheGetPropagatesBuildError(t *testing.T) { + root := rootAt(t, t.TempDir()) + if err := root.FS.Close(); err != nil { + t.Fatal(err) + } + c := NewCache() + if _, err := c.Get(root); err == nil { + t.Fatal("want an error when the root's filesystem is already closed") + } +} diff --git a/internal/linkindex/slug_test.go b/internal/linkindex/slug_test.go new file mode 100644 index 0000000..b47fe77 --- /dev/null +++ b/internal/linkindex/slug_test.go @@ -0,0 +1,77 @@ +package linkindex + +import "testing" + +func TestGithubSlug(t *testing.T) { + cases := map[string]string{ + "Hello World": "hello-world", + "Hello, World!": "hello-world", + " Leading/trailing": "leadingtrailing", + " Hello ": "hello", + "Café Menu": "café-menu", // Unicode letters are preserved, not stripped + "under_score": "under_score", + "": "", + "---": "", // trims to nothing + } + for in, want := range cases { + if got := GithubSlug(in); got != want { + t.Errorf("GithubSlug(%q) = %q, want %q", in, got, want) + } + } + // Each whitespace character becomes its own hyphen -- runs are not + // collapsed, unlike ConfluenceSlug. + if got := GithubSlug("Hello World"); got != "hello---world" { + t.Errorf(`GithubSlug("Hello World") = %q, want "hello---world"`, got) + } +} + +func TestConfluenceSlug(t *testing.T) { + // Case and punctuation survive; only whitespace becomes a hyphen. + if got := ConfluenceSlug("Hello, World!"); got != "Hello,-World!" { + t.Errorf(`ConfluenceSlug("Hello, World!") = %q, want "Hello,-World!"`, got) + } + // Runs of whitespace collapse to a single hyphen -- the opposite of + // GithubSlug, which hyphenates each whitespace character individually. + if got := ConfluenceSlug("Hello World"); got != "Hello-World" { + t.Errorf(`ConfluenceSlug("Hello World") = %q, want "Hello-World"`, got) + } + // Leading/trailing whitespace is trimmed before collapsing, not turned + // into a leading/trailing hyphen. + if got := ConfluenceSlug(" Hello World "); got != "Hello-World" { + t.Errorf(`ConfluenceSlug(" Hello World ") = %q, want "Hello-World"`, got) + } +} + +func TestExtractHeadings(t *testing.T) { + body := "# Title\n" + + "some text\n" + + "## Sub Heading\n" + + "```\n" + + "# not a heading, inside a fence\n" + + "```\n" + + "### Real Heading\n" + + "#NoSpaceAfterHash\n" + + "###\n" + // all hashes, nothing else + "# \n" // hash then only whitespace -- trims to empty + + got := extractHeadings(body) + want := []string{"Title", "Sub Heading", "Real Heading"} + if len(got) != len(want) { + t.Fatalf("got %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("heading %d = %q, want %q", i, got[i], want[i]) + } + } +} + +func TestExtractHeadingsUnterminatedFenceSkipsToEnd(t *testing.T) { + // A fence that's never closed must not leave later real headings exposed + // by some off-by-one toggle; everything after the open fence is "in code." + body := "# Before\n```\n# inside, unterminated\n## also inside\n" + got := extractHeadings(body) + if len(got) != 1 || got[0] != "Before" { + t.Errorf("got %v, want just [Before]", got) + } +} From 6cf30a4b2d0abdd4f6e0d094d8dfa86b5d293472 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:43:57 -0400 Subject: [PATCH 18/22] test(project): confirm Cache.Close is safe on a backfilled shared root walkAndCache's backfill can point several byDir keys at the same *Root, so Close -- which closes every key's Root unconditionally -- calls FS.Close() on the same underlying os.Root handle more than once. Confirmed empirically that os.Root.Close is idempotent (returns nil on every call, not just the first), so this was already safe; added a test that builds a real backfilled scenario and closes it, so a future change to Close's iteration or to os.Root's semantics can't regress it silently. --- internal/project/cache_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/project/cache_test.go b/internal/project/cache_test.go index edc8864..5fed55b 100644 --- a/internal/project/cache_test.go +++ b/internal/project/cache_test.go @@ -159,3 +159,36 @@ func TestCacheCloseClearsEntries(t *testing.T) { t.Errorf("byDir has %d entries after Close, want 0", len(c.byDir)) } } + +// TestCacheCloseIsSafeWithABackfilledSharedRoot: walkAndCache's backfill can +// point several byDir keys at the same *Root, so Close -- which iterates every +// key and closes its Root unconditionally -- calls FS.Close() on that same +// underlying handle more than once. os.Root.Close is safe to call repeatedly +// (confirmed: it returns nil every time, not an error on the second call), so +// this must not panic or fail. +func TestCacheCloseIsSafeWithABackfilledSharedRoot(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, Filename), []byte("# marker\n"), 0o644); err != nil { + t.Fatal(err) + } + x := filepath.Join(root, "docs", "a") + y := filepath.Join(root, "docs", "b") + for _, d := range []string{x, y} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + + c := NewCache("") + if _, err := c.Resolve(x); err != nil { + t.Fatal(err) + } + if _, err := c.Resolve(y); err != nil { + t.Fatal(err) + } + // x, y, and the backfilled "docs" ancestor all share one *Root. + if len(c.byDir) < 3 { + t.Fatalf("byDir has %d entries, want at least 3 (x, y, and the backfilled ancestor)", len(c.byDir)) + } + c.Close() // must not panic despite closing the same *os.Root more than once +} From 933a6ca5262e0a0caf35561a624ea0111d0a156f Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:46:38 -0400 Subject: [PATCH 19/22] test(find): cover run() end-to-end find had no test of run() itself, only its pure table() helper. Added the success path, "No matches found.", the empty-title usage error, an unknown --space usage error, and --json output shape -- using a bare *cobra.Command carrying just the flags run() reads (CONFLUENCE_TOKEN via the environment, since it's never a flag) rather than the real root command tree. --- cmd/find/find_test.go | 142 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/cmd/find/find_test.go b/cmd/find/find_test.go index f8bd9c5..22b3bca 100644 --- a/cmd/find/find_test.go +++ b/cmd/find/find_test.go @@ -1,12 +1,154 @@ package find import ( + "encoding/json" + "io" + "net/http" + "os" "strings" "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" + "github.com/mozilla/markfluence/internal/ui" + "github.com/spf13/cobra" ) +// testCmd builds a bare *cobra.Command carrying the flags run() reads, +// pointed at url. It doesn't go through the real root command (which would +// need a full command tree and CONFLUENCE_TOKEN can't be a flag), so the +// token is supplied via the environment instead, exactly as a real +// invocation would. +func testCmd(t *testing.T, url 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", "", "") + return c +} + +// 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 +} + +// findRunServer answers the two requests FindByTitle makes with no --space: +// the v2 pages query and the CQL search. +func findRunServer(t *testing.T, pages, search string) string { + t.Helper() + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasPrefix(r.URL.Path, "/wiki/api/v2/pages"): + _, _ = w.Write([]byte(pages)) + case strings.HasPrefix(r.URL.Path, "/wiki/rest/api/search"): + _, _ = w.Write([]byte(search)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + }) + return c.SiteURL() +} + +func TestRunFindsMatches(t *testing.T) { + url := findRunServer(t, + `{"results":[{"id":"1","title":"Runbook","status":"current",`+ + `"_links":{"webui":"/spaces/ENG/pages/1/Runbook"}}],"_links":{}}`, + `{"results":[],"_links":{}}`) + + out, err := captureStdout(t, func() error { return run(testCmd(t, url), []string{"Runbook"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "Runbook") || !strings.Contains(out, "1") { + t.Errorf("output = %q, want it to list the match", out) + } +} + +func TestRunReportsNoMatches(t *testing.T) { + url := findRunServer(t, `{"results":[],"_links":{}}`, `{"results":[],"_links":{}}`) + + out, err := captureStdout(t, func() error { return run(testCmd(t, url), []string{"Ghost"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "No matches found.") { + t.Errorf("output = %q, want the no-matches message", out) + } +} + +func TestRunEmptyTitleIsAUsageError(t *testing.T) { + cmd := testCmd(t, "https://wiki.example.net") + _, err := captureStdout(t, func() error { return run(cmd, []string{" "}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 2 { + t.Fatalf("run: %v, want a silent exit-2 usage error", err) + } +} + +func TestRunUnknownSpaceIsAUsageError(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/wiki/api/v2/spaces") { + _, _ = w.Write([]byte(`{"results":[]}`)) + return + } + t.Errorf("unexpected request with an unresolved space: %s", r.URL.Path) + }) + // run() reads the package-level spaceOpt (bound to Cmd's own --space flag + // in init()), not a flag on whatever *cobra.Command is passed in. + spaceOpt = "NOPE" + t.Cleanup(func() { spaceOpt = "" }) + + _, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"Runbook"}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 2 { + t.Fatalf("run: %v, want a silent exit-2 usage error for an unknown space", err) + } +} + +func TestRunJSONOutput(t *testing.T) { + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + url := findRunServer(t, + `{"results":[{"id":"1","title":"Runbook","status":"current",`+ + `"_links":{"webui":"/spaces/ENG/pages/1/Runbook"}}],"_links":{}}`, + `{"results":[],"_links":{}}`) + + out, err := captureStdout(t, func() error { return run(testCmd(t, url), []string{"Runbook"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + var env struct { + Command string `json:"command"` + Results []struct { + ID string `json:"id"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if env.Command != "find" || len(env.Results) != 1 || env.Results[0].ID != "1" { + t.Errorf("envelope = %+v, want command=find with one result id=1", env) + } +} + func TestTableAligns(t *testing.T) { got := table([]client.TitleMatch{ {ID: "500", Type: "page", Title: "Runbook", Status: "current", Space: "ENG", From d9ff43e750999b06648f6729299ef676be1df838 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:47:45 -0400 Subject: [PATCH 20/22] test(children): cover run() end-to-end children had no test of run() itself, only parseDepth and tree(). Added the success path, "No children.", an invalid --depth usage error, and --json output shape, using the same bare-*cobra.Command pattern as find's run() tests. --- cmd/children/children_test.go | 120 ++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/cmd/children/children_test.go b/cmd/children/children_test.go index 6e32b2a..c618553 100644 --- a/cmd/children/children_test.go +++ b/cmd/children/children_test.go @@ -1,12 +1,73 @@ package children import ( + "encoding/json" + "io" + "net/http" + "os" "strings" "testing" + "github.com/mozilla/markfluence/internal/clienttest" "github.com/mozilla/markfluence/internal/pagetree" + "github.com/mozilla/markfluence/internal/ui" + "github.com/spf13/cobra" ) +// testCmd builds a bare *cobra.Command carrying the flags run() reads, +// 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. +func testCmd(t *testing.T, url 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", "", "") + return c +} + +// 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 +} + +// childServer answers the v1 child/page and child/folder routes pagetree.Walk +// makes for one page: pages under "1", none under any other id. +func childServer(t *testing.T) string { + t.Helper() + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasPrefix(r.URL.Path, "/wiki/rest/api/content/1/child/page"): + _, _ = w.Write([]byte(`{"results":[{"id":"2","type":"page","title":"Child",` + + `"status":"current","extensions":{"position":0},"_links":{"webui":"/spaces/ENG/pages/2/Child"}}]}`)) + case strings.HasPrefix(r.URL.Path, "/wiki/rest/api/content/"): + _, _ = w.Write([]byte(`{"results":[]}`)) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + } + }) + return c.SiteURL() +} + // TestParseDepth covers the flag's whole vocabulary. 0 is the interesting case: // it is a common spelling of "unlimited" elsewhere, so accepting it would launch // an unbounded walk for someone who may have meant the opposite. @@ -77,3 +138,62 @@ func TestTreeHasNoTrailingSpaces(t *testing.T) { } } } + +func TestRunListsChildren(t *testing.T) { + url := childServer(t) + out, err := captureStdout(t, func() error { return run(testCmd(t, url), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "Child") || !strings.Contains(out, "2") { + t.Errorf("output = %q, want the child page listed", out) + } +} + +func TestRunNoChildren(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"results":[]}`)) + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "No children.") { + t.Errorf("output = %q, want the no-children message", out) + } +} + +func TestRunInvalidDepthIsAUsageError(t *testing.T) { + depthOpt = "0" + t.Cleanup(func() { depthOpt = "1" }) + + _, err := captureStdout(t, func() error { + return run(testCmd(t, "https://wiki.example.net"), []string{"1"}) + }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 2 { + t.Fatalf("run: %v, want a silent exit-2 usage error for --depth 0", err) + } +} + +func TestRunJSONOutput(t *testing.T) { + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + url := childServer(t) + out, err := captureStdout(t, func() error { return run(testCmd(t, url), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + var env struct { + Command string `json:"command"` + Results []struct { + ID string `json:"id"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if env.Command != "children" || len(env.Results) != 1 || env.Results[0].ID != "2" { + t.Errorf("envelope = %+v, want command=children with one result id=2", env) + } +} From ffa0516e7cde1fefb3aafbd8e62a8a089e725591 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:49:21 -0400 Subject: [PATCH 21/22] test(info): cover run() end-to-end info had no test of run() itself, only its pure formatting helpers. Added the success path, a missing-page operational failure, --properties triggering the content-properties fetch and section, and --json output shape. --- cmd/info/info_test.go | 126 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/cmd/info/info_test.go b/cmd/info/info_test.go index b56497c..fa07c5f 100644 --- a/cmd/info/info_test.go +++ b/cmd/info/info_test.go @@ -1,14 +1,60 @@ package info import ( + "encoding/json" "errors" + "io" + "net/http" + "os" "strings" "testing" "unicode/utf8" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" + "github.com/mozilla/markfluence/internal/ui" + "github.com/spf13/cobra" ) +// testCmd builds a bare *cobra.Command carrying the flags run() reads, +// 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. +func testCmd(t *testing.T, url 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", "", "") + return c +} + +// 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 +} + +const pageJSON = `{"id":"1","title":"Runbook","status":"current","spaceId":"77",` + + `"version":{"number":3},"_links":{"webui":"/spaces/ENG/pages/1/Runbook"}}` + func TestRenderValue(t *testing.T) { if got := renderValue("max"); got != "max" { t.Errorf("renderValue(string) = %q, want max", got) @@ -54,3 +100,83 @@ func TestPropertiesSectionFetchError(t *testing.T) { t.Errorf("propertiesSection(error) = %q", got) } } + +func TestRunPrintsPageInfo(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/properties"): + _, _ = w.Write([]byte(`{"results":[]}`)) + default: + _, _ = w.Write([]byte(pageJSON)) + } + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "Runbook") || !strings.Contains(out, "id:") { + t.Errorf("output = %q, want the page's metadata", out) + } +} + +func TestRunPageNotFound(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"status":404,"code":"NOT_FOUND"}]}`)) + }) + _, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"999"}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run: %v, want a silent exit-1 error for a missing page", err) + } +} + +func TestRunWithPropertiesFlag(t *testing.T) { + showProperties = true + t.Cleanup(func() { showProperties = false }) + + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/properties"): + _, _ = w.Write([]byte(`{"results":[{"key":"content-appearance-published","value":"max"}]}`)) + default: + _, _ = w.Write([]byte(pageJSON)) + } + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "content properties:") || !strings.Contains(out, "content-appearance-published") { + t.Errorf("output = %q, want the properties section listed", out) + } +} + +func TestRunJSONOutput(t *testing.T) { + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/properties"): + _, _ = w.Write([]byte(`{"results":[]}`)) + default: + _, _ = w.Write([]byte(pageJSON)) + } + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + var env struct { + Command string `json:"command"` + Results []struct { + PageID string `json:"page_id"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + if env.Command != "info" || len(env.Results) != 1 || env.Results[0].PageID != "1" { + t.Errorf("envelope = %+v, want command=info with one result page_id=1", env) + } +} From d341ac03f41bfceecfd20aa923ba9efff07debae Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Sat, 29 Aug 2026 14:50:54 -0400 Subject: [PATCH 22/22] test(read): cover run() end-to-end read had no test file exercising run() at all -- only json_test.go's marshaling helpers. Added markdown and storage format output, the unsupported-format usage error, a missing-page operational failure, the no-readable-body check (the one thing standing between reading a folder and a confusing downstream error), and --json output shape. --- cmd/read/read_test.go | 161 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 cmd/read/read_test.go diff --git a/cmd/read/read_test.go b/cmd/read/read_test.go new file mode 100644 index 0000000..d257328 --- /dev/null +++ b/cmd/read/read_test.go @@ -0,0 +1,161 @@ +package read + +import ( + "encoding/json" + "io" + "net/http" + "os" + "strings" + "testing" + + "github.com/mozilla/markfluence/internal/clienttest" + "github.com/mozilla/markfluence/internal/ui" + "github.com/spf13/cobra" +) + +// testCmd builds a bare *cobra.Command carrying the flags run() reads, +// 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. +func testCmd(t *testing.T, url 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", "", "") + return c +} + +// 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 +} + +func pageWithBody(body string) string { + return `{"id":"1","title":"Runbook","spaceId":"77",` + + `"body":{"storage":{"value":` + `"` + body + `"` + `,"representation":"storage"}},` + + `"_links":{"webui":"/spaces/ENG/pages/1/Runbook"}}` +} + +func TestRunPrintsMarkdown(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/properties"): + _, _ = w.Write([]byte(`{"results":[]}`)) + default: + _, _ = w.Write([]byte(pageWithBody("

Hello

"))) + } + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if !strings.Contains(out, "title: Runbook") { + t.Errorf("output = %q, want frontmatter with the title", out) + } + if !strings.Contains(out, "Hello") { + t.Errorf("output = %q, want the converted body", out) + } +} + +func TestRunPrintsStorage(t *testing.T) { + formatFlag = formatStorage + t.Cleanup(func() { formatFlag = formatMarkdown }) + + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(pageWithBody("

Hello

"))) + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + if strings.TrimSpace(out) != "

Hello

" { + t.Errorf("output = %q, want the raw storage body verbatim", out) + } + if strings.Contains(out, "title:") { + t.Errorf("output = %q, storage format should carry no frontmatter", out) + } +} + +func TestRunInvalidFormatIsAUsageError(t *testing.T) { + formatFlag = "pdf" + t.Cleanup(func() { formatFlag = formatMarkdown }) + + _, err := captureStdout(t, func() error { + return run(testCmd(t, "https://wiki.example.net"), []string{"1"}) + }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 2 { + t.Fatalf("run: %v, want a silent exit-2 usage error for an unsupported --format", err) + } +} + +func TestRunPageNotFound(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"errors":[{"status":404,"code":"NOT_FOUND"}]}`)) + }) + _, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"999"}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run: %v, want a silent exit-1 error for a missing page", err) + } +} + +func TestRunEmptyBodyIsAFailure(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"id":"1","title":"Folder"}`)) + }) + _, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if !ui.IsSilent(err) || ui.ExitCode(err) != 1 { + t.Fatalf("run: %v, want a silent exit-1 error for a page with no readable body", err) + } +} + +func TestRunJSONOutput(t *testing.T) { + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + + c := clienttest.New(t, func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "/properties"): + _, _ = w.Write([]byte(`{"results":[]}`)) + default: + _, _ = w.Write([]byte(pageWithBody("

Hello

"))) + } + }) + out, err := captureStdout(t, func() error { return run(testCmd(t, c.SiteURL()), []string{"1"}) }) + if err != nil { + t.Fatalf("run: %v", err) + } + var env struct { + Command string `json:"command"` + Results []struct { + PageID string `json:"page_id"` + Format string `json:"format"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("output is not valid JSON: %v\n%s", err, out) + } + res := env.Results + if env.Command != "read" || len(res) != 1 || res[0].PageID != "1" || res[0].Format != "markdown" { + t.Errorf("envelope = %+v, want command=read with one result page_id=1 format=markdown", env) + } +}