From 9a5078c2aa974cd43c339b207bf487a861196f10 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:33:10 -0400 Subject: [PATCH 1/8] docs: plan for classifying an error code by the failure's origin Fixes #133 by making a --json failure's code distinguish a server failure from a local one, in both directions: create's preflight stops reporting VALIDATION for an HTTP failure, and the four attachment-path sites stop reporting NETWORK for a local file that cannot be read. --- _plans/035_error-code-classification.md | 269 ++++++++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 _plans/035_error-code-classification.md diff --git a/_plans/035_error-code-classification.md b/_plans/035_error-code-classification.md new file mode 100644 index 0000000..091b5f8 --- /dev/null +++ b/_plans/035_error-code-classification.md @@ -0,0 +1,269 @@ +# Plan: classify an error code by the failure's origin + +Make a `--json` failure's `code` distinguish a server failure from a local one, +in both directions: a preflight HTTP failure in `create` stops reporting +`VALIDATION`, and a local file that cannot be read on an attachment path stops +reporting `NETWORK`. Fixes #133. + +## The bug + +`create`'s preflight makes four kinds of server call -- `checkPageID` +(`GetPageOrNil`), `ResolveSpaceID`, `resolveParent` (`checkParentInSpace`), and +`checkTitleFree` (`SearchPagesByTitle`). Every one can fail with an +`*client.HTTPError`, and `newFailure` (create.go:192) stamps every phase-1 error +`jsonout.CodeValidation` -- the code that means "there is something wrong with +your file". + +The worst case is the one `RejectedCredential` exists for. A revoked token +answers every v2 route with a 404 whose body names nothing, and `GetPageOrNil` +gates its nil-return on `notFound`, which excludes exactly that case +(client.go:248) -- so `checkPageID` returns the `*HTTPError`, and `--json` +reports `VALIDATION` against a file that is perfectly fine. `jsonout.CodeFor` +asks `RejectedCredential` *before* its status switch precisely so this reports +`AUTH`, and `create` throws that away. `create`'s own phase 3 (`publishOne`) +already uses `CodeFor`, so today one credential produces `AUTH` from publish and +`VALIDATION` from preflight. `cmd/fix` gets it right for the HTTP half via +`locateCode` (fix/json.go:142). Two commands classifying one failure +differently is the defect #133 names. + +The same defect runs the other way on the attachment paths, and #127 walked +past it. `client.planAttachments` calls `fileChecksum(att.Path)` +(client.go:1033) -- a local `os.Open` -- and returns that error raw, so a +`SyncAttachments`/`PlanAttachments`/`ForceUploadAttachments` failure classified +through bare `CodeFor` reports **`NETWORK`** for an unreadable local asset. Four +sites do that. `create`'s `publishOne` comment names this exact condition as one +of S7's residuals ("an image that Lstat'd fine in preflight can still be +unreadable now"), and when it happens `--json` blames the network. + +`CodeFor` alone cannot fix either direction: it answers `NETWORK` for any +non-`HTTPError`, so routing everything through it would report `no title given` +as a transport problem. + +## Decisions + +**The fallback rule is not enough on its own, because a transport failure is not +an `*HTTPError`.** #133 proposes lifting `fix`'s `locateCode` -- "is it an +`*HTTPError`? then `CodeFor`, else `VALIDATION`" -- into `internal/jsonout`. +That fixes the status half only. `doJSON` builds an `HTTPError` only once it has +a status (client.go:451); a dial failure, a TLS error, or a malformed response +body comes back as a plain error from `send`, so under a bare type check a +preflight failure with the VPN down still reports `VALIDATION` -- and would in +`fix` too, while `update` reports `NETWORK` for the same failure because it +calls `CodeFor` at the call site. The distinction matters more than the status +one: `NETWORK` vs `VALIDATION` is what a consumer branches on to decide whether +retrying is worth anything. + +**So the client types its own request-path errors, and callers ask a predicate +rather than marking call sites.** The rejected alternative was to wrap each +client call's error in `create` and `fix` with a typed marker, the way +`convertFailure` and `attachmentupload`'s `badInput` already do. That works, but +the obligation lands on every future call site and fails *silently* when +forgotten: a new client call added to preflight without the wrapper reports +`VALIDATION`, which is the bug being fixed, reintroduced. Typing at the source +puts the guarantee in one function where a test can hold it, the same reasoning +that keeps the traversal clamp in `internal/attachfile` instead of in two +commands. + +**The rule, stated so it stays true at 118 call sites:** an error a client +method returns because *the request* failed is typed; an error that came from +the caller's own data is not. So `send`'s transport returns, all of `doJSON`'s +own returns (`json.Marshal`, `http.NewRequest`, `json.Unmarshal` of the +response), the next-link parse in the pagination helpers, and the two other +request builders (`DownloadAttachment`, the multipart upload) are typed -- +while `DownloadAttachment`'s `w.Write`, `os.Open` in the upload path, and +`Resolve`'s config errors are left exactly as they are, for their callers to +classify as `IO`/`CONFIG`. There is deliberately **no** claim that every error +from `internal/client` is typed: `w.Write` writing the destination file is a +local disk failure, and calling it a request error would be the same lie in a +new place. + +**`*HTTPError` and the new type are siblings, and the predicate lives in the +client.** `doJSON` returns one or the other. Rather than have `jsonout` run two +`errors.As` checks -- and grow a third the day the client grows a type -- +`internal/client` answers for its own error types with `FromRequest(err) bool`. +`jsonout` already imports `client` for `CodeFor`, so this adds no dependency +edge. + +**The new type is unexported; `FromRequest` is the whole new surface.** The only +sanctioned use is the predicate, and keeping the type unexported means nothing +outside `internal/client` can start branching on it and grow a second +classification rule -- which is how the disagreement in #133 arose. It is also +the reversible direction: unexported to exported is additive the day someone +genuinely needs `errors.As`, while the reverse breaks callers. `*HTTPError` +stays exported, because `notFound`, `RejectedCredential`, the hint logic, and +`CodeFor` all read its fields. + +**`Error()` returns the inner text verbatim, and the wrapper carries nothing +else.** It is a classification tag, exactly like `convertFailure`, which also +carries no message of its own. This is what guarantees the change is invisible +to human output and to every existing test that asserts on an error string: the +only observable difference is the `code` field. + +**A malformed 200 response classifies as `NETWORK`.** `CodeFor`'s existing +non-`HTTPError` branch answers `NETWORK`, which is right for a dial failure and +a stretch for a response body that failed to decode. Both mean "no usable +answer, and retrying is not obviously pointless", and adding a code to split +them would be a vocabulary change (`schema_version` territory) for a case whose +message already says what happened. + +**The mirror sites are fixed here, not deferred.** `_plans/034` deferred #133 +because it changed codes on failures that issue was not about, when there was no +shared rule to appeal to. The rule is now the thing being added, so applying it +everywhere it belongs *is* the change -- and landing a helper whose entire +purpose is "stop guessing the code" while four call sites nearby keep guessing +would read as though the guessing were deliberate. The accepted cost is a +`--json` code change on failures #133 does not mention: `NETWORK` -> `IO` for an +unreadable or missing local asset. + +**No schema change.** `$defs/code` is one global enum and all eight codes are +valid on every result shape (v1.json:159). `README.md` lists the eight values +and never claims which one a given failure carries, so it needs no edit either. + +**Exit codes do not move.** A preflight `AUTH` stays a per-file failure with +exit 1. The README's contract scopes exit 2 to "bad flags, credential +*resolution*" -- resolving a credential locally -- not the server rejecting one, +and `update` already reports a rejected credential per-file. Making `create` +fatal here would create a new inconsistency of exactly the kind #133 is about. + +## Implementation + +### `internal/client` + +- `requestError` -- unexported, holds one error, `Error()` returns the inner + text verbatim, plus `Unwrap()`. +- `FromRequest(err error) bool` -- true for `*HTTPError` or `*requestError`, + false for anything else including nil. Its doc comment carries the rule above, + including what is deliberately *not* typed and why. +- Wrap sites: `send`'s transport returns; every non-status return in `doJSON`; + the next-link/URL parse in the pagination helpers; `DownloadAttachment`'s and + the multipart upload's `http.NewRequest`. `json.Marshal` of a request body + cannot fail for any body this code builds, but it is wrapped too, so the + invariant is statable as "`doJSON` returns only typed errors" rather than + "only typed errors except one". + +### `internal/jsonout` + +- `CodeOr(err error, fallback Code) Code` -- `client.FromRequest(err)` then + `CodeFor(err)`, else `fallback`. The fallback is a parameter rather than + hardcoded `VALIDATION` so the call site says which default it is choosing; + both directions of the bug are one call with a different fallback. + +### `cmd/create` + +- `newFailure` -> `jsonout.CodeOr(err, jsonout.CodeValidation)`, after the + `convertFailure` check so `CONVERT` still wins. Local phase-1 errors (`no + title given`, `pageIDFailure`, the duplicate-title messages, `no space + given`, `space %q not found`, `resolveParent`'s conflicts) are neither client + type, so they stay `VALIDATION` by construction. +- `publishOne`'s `SyncAttachments` -> `CodeOr(err, jsonout.CodeIO)`. + +### `cmd/fix` + +- `locateCode` deleted; `processFile` calls `jsonout.CodeOr(err, + jsonout.CodeValidation)`. Its tests move to `internal/jsonout` with it. + +### `cmd/update` + +- `SyncAttachments` and `PlanAttachments` -> `CodeOr(err, jsonout.CodeIO)`. + +### `cmd/attachmentupload` + +- `plan(...)` -> `CodeOr(err, jsonout.CodeIO)`. This command already + distinguishes `IO` from `VALIDATION` upstream via `localAttachmentsCode` and + then loses the distinction one call later, which is the clearest single + illustration of the bug. + +### `internal/attachfile` + +- `Write`'s `DownloadAttachment` -> `CodeOr(err, jsonout.CodeIO)`. + +## Tests + +- **`internal/client`** -- the new invariant, in the package that owns it: a 500 + is an `*HTTPError`; a closed server (dial refused) and a 200 with a malformed + JSON body are `*requestError`; all three satisfy `FromRequest`. + `FromRequest(errors.New("x"))` and `FromRequest(nil)` are false. `Unwrap` + returns the inner error and `Error()` matches it exactly -- that last one is + what pins "no message anywhere changes". +- **`internal/jsonout`** -- `CodeOr` table: a rejected-credential 404 is `AUTH` + (not `NOT_FOUND`), 403 is `AUTH`, 500 is `API`, a `*requestError` is + `NETWORK`, and a plain `errors.New("no title given")` is the fallback. The + last row is #133's named hazard asserted directly. +- **`cmd/create`** -- through the existing `fakeConfluence`, with a per-test + override answering one route 404 with `"title":"Not Found"`: a file carrying a + `page_id` reports `code: AUTH` in the emitted envelope. Plus a 403 on + `/wiki/api/v2/spaces` reporting `AUTH`, and a `no title given` file still + reporting `VALIDATION` in the same batch. +- **`cmd/fix`** -- the same rejected-credential 404 reporting `AUTH`. This is + the consistency claim #133 actually makes, so it is asserted from both sides + rather than inferred from a shared helper. +- **Mirror-direction guard** -- a 403 from the attachment listing still reports + `AUTH` through `CodeOr(err, CodeIO)`, so the flipped fallback cannot swallow a + server failure. + +**Not tested, deliberately.** The local-`IO` direction end to end. An asset that +is missing at upload time never reaches `fileChecksum` -- the converter reports +it `IMAGE BROKEN` and adds no attachment -- so provoking it needs a file +readable at convert time and unreadable at upload time, which means either a +`chmod 000` that behaves differently as root or a hook in the client existing +only for a test. The `IO` classification is pinned by the `jsonout` table on a +real `fs.PathError`, and the guard above pins the direction that can regress +silently. + +## Docs + +- `CLAUDE.md`, the `internal/client` bullet: the package returns two error + types on the request path and answers `FromRequest` about them, with the + request-vs-local rule in a clause and the note that file handling on the + attachment paths is deliberately outside it. The existing `CodeFor` + /`RejectedCredential` sentence there gains the `CodeOr` counterpart. +- No `README.md` change (it lists the eight codes and claims nothing per-code), + no schema change, no `docs/confluence/` change (no new API knowledge). + +## Out of scope (deliberately) + +- **A new guarantee in `docs/guarantees.md`.** An `error-code-names-the-cause` + R3 was considered and dropped. Worded checkably it would say "no error from a + request reports a local code, and no local error reports + `NETWORK`/`AUTH`/`NOT_FOUND`/`API`" -- but it would have to land as Partial, + since it rests on 118 code-assignment sites across `cmd`/`internal` being + individually right and nothing mechanically stops a new site from passing a + wrong fallback. Auditing all 118 to claim Holds is a large review surface for + a small yield, and a wrong call in that sweep would be a silent status lie. + This change cites #133 and nothing else. +- **`frontmatter.ParseFile`'s unreadable-file error staying `VALIDATION`.** It + is a local read failure reported as a file defect, which looks like a + near-miss of the same bug but is the documented house answer: `checkResult`'s + schema description commits to it in writing ("status=failed ... (unreadable, + unterminated frontmatter, ...) -- code is VALIDATION in that case", + v1.json:440). Changing it in `create` alone would make `create` and `check` + disagree, which is the shape of defect this change exists to remove. Same for + `roots.Resolve` and `linkindex` build failures, which are tree walks reported + against the file that triggered them. +- **`DownloadAttachment`'s "no download link" error.** It is returned before any + request is made -- Confluence handed back an attachment record with an empty + `_links.download` -- so it is neither request nor local, and the `IO` fallback + reports it as a disk problem. Left as a named residual rather than given a + third error type or a `CodeOr` variant: it is one error on one path, its + message says precisely what happened, and the reader's next move (look at the + attachment record) is not changed by the code. +- **A client-wide "every error is typed" invariant.** Ruled out above: + `DownloadAttachment`'s `w.Write` and the upload's `os.Open` are local + failures, and `Resolve` is config. The invariant is scoped to the request + path and says so. +- **Splitting `NETWORK` for a malformed response.** Would need a ninth code and + a `schema_version` conversation. + +## Commits + +1. `docs: plan for classifying an error code by the failure's origin` +2. `feat(client): type every request-path error, and FromRequest to ask` +3. `feat(jsonout): add CodeOr, classifying by whether a request failed` +4. `fix(create): report a preflight HTTP failure by its status, not VALIDATION` +5. `refactor(fix): classify a locate failure through jsonout.CodeOr` +6. `fix: report a local attachment failure as IO, not NETWORK` +7. `docs: record the two client error types and CodeOr` + +Commit 6 is the four mirror sites in one commit: it is one rule applied to four +call sites, and splitting it would produce four commits whose messages differ +only by package name. From 1cb0fe735a5c84a9e784e671e99e3a52ec018a7f Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:35:00 -0400 Subject: [PATCH 2/8] feat(client): type every request-path error, and FromRequest to ask doJSON built an *HTTPError only once it had a status, so a transport failure, a request that would not build, and a response body that would not decode all came back as plain errors indistinguishable from a caller's own "no title given". A command classifying a failure for --json could therefore tell a 403 from a bad file, but not a dropped connection from one. requestError tags those three, so the package now returns exactly two error types on the request path and answers FromRequest about both. It carries no message of its own -- Error() is the inner text verbatim -- so no human output and no message-asserting test changes; the only observable difference is what a caller may now conclude. Deliberately not a claim that every error from this package is typed: DownloadAttachment writing to the caller's writer, uploadAttachment opening the caller's file, and Resolve reading the environment are local failures, and tagging them would misreport an unreadable file as a network problem -- the same defect in a new place. Refs #133 --- internal/client/client.go | 61 +++++++++++++++++++++--- internal/client/client_test.go | 86 ++++++++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 7 deletions(-) diff --git a/internal/client/client.go b/internal/client/client.go index 5832163..613e1e1 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -250,6 +250,48 @@ func notFound(err error) bool { return errors.As(err, &he) && he.StatusCode == http.StatusNotFound && !he.RejectedCredential() } +// requestError marks an error as having come from a request that never +// produced a usable answer: a transport failure, a request that could not be +// built, or a response body that would not decode. It is a classification tag +// and nothing else -- Error() returns the inner text verbatim, so a wrapped +// error reads exactly as it did before -- which is what lets FromRequest +// separate a server failure from a local one without changing any message. +type requestError struct{ err error } + +func (e *requestError) Error() string { return e.err.Error() } +func (e *requestError) Unwrap() error { return e.err } + +// wrapRequest tags err as a request failure, leaving nil alone. +func wrapRequest(err error) error { + if err == nil { + return nil + } + return &requestError{err: err} +} + +// FromRequest reports whether err came from a Confluence request rather than +// from the caller's own data. It is what a command asks before classifying a +// failure for --json: a request failure classifies by CodeFor (AUTH, NOT_FOUND, +// API, NETWORK), and anything else is a local problem the caller names itself +// (VALIDATION for a bad file, IO for an unreadable one). +// +// The rule this package holds up: an error a method returns because *the +// request* failed is typed -- an *HTTPError once there is a status, a +// requestError when there is not. An error that came from the caller's own data +// is deliberately left untyped, so it keeps whatever meaning its caller gives +// it. That is why this is not "every error from internal/client is typed": +// DownloadAttachment writing to the caller's writer, uploadAttachment opening +// the caller's file, and Resolve reading the environment are local failures, +// and calling them request errors would be the same misreport in a new place. +func FromRequest(err error) bool { + var he *HTTPError + if errors.As(err, &he) { + return true + } + var re *requestError + return errors.As(err, &re) +} + // viaGateway reports whether the request went to the platform API gateway. The // URL already carries the answer, so the error needs no extra field and no // construction site has to change. @@ -427,7 +469,7 @@ func (c *ConfluenceClient) doJSON( if reqBody != nil { b, err := json.Marshal(reqBody) if err != nil { - return err + return wrapRequest(err) } body = bytes.NewReader(b) } @@ -438,20 +480,22 @@ func (c *ConfluenceClient) doJSON( // the body on a retry. req, err := http.NewRequest(method, rawURL, body) if err != nil { - return err + return wrapRequest(err) } if reqBody != nil { req.Header.Set("Content-Type", "application/json") } status, respBody, err := c.send(req, timeout) if err != nil { - return err + return err // already tagged by send } if status >= 400 { return &HTTPError{StatusCode: status, Method: method, URL: rawURL, Body: string(respBody)} } if out != nil && len(respBody) > 0 { - return json.Unmarshal(respBody, out) + // A body that will not decode is a request that produced no usable + // answer, not a defect in anything the caller passed in. + return wrapRequest(json.Unmarshal(respBody, out)) } return nil } @@ -478,7 +522,10 @@ func (c *ConfluenceClient) send(req *http.Request, timeout time.Duration) (int, ev.Retrying = attempt < maxRetries && isIdempotent(req.Method) if !ev.Retrying { logRetry(ev) - return 0, nil, err + // Tagged on the way out, after the retry event has recorded the + // raw error: a transport failure is a request failure, and every + // caller of send returns it unchanged. + return 0, nil, wrapRequest(err) } ev.Delay = backoff(attempt, 0) logRetry(ev) @@ -988,7 +1035,7 @@ func (c *ConfluenceClient) DownloadAttachment(att Attachment, w io.Writer) error rawURL := c.baseURL + "/wiki" + att.Links.Download req, err := http.NewRequest(http.MethodGet, rawURL, nil) if err != nil { - return err + return wrapRequest(err) } status, body, err := c.send(req, timeoutDownload) if err != nil { @@ -1200,7 +1247,7 @@ func (c *ConfluenceClient) uploadAttachment(rawURL, filename, comment, filePath, req, err := http.NewRequest(http.MethodPost, rawURL, &buf) if err != nil { - return err + return wrapRequest(err) } req.Header.Set("Content-Type", mw.FormDataContentType()) req.Header.Set("X-Atlassian-Token", "nocheck") diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 0232a2c..7a1004a 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -1759,3 +1759,89 @@ func TestHTTPErrorHint(t *testing.T) { }) } } + +// TestFromRequestOnEveryRequestFailure pins the invariant the --json error codes +// rest on: every way a request can fail arrives as one of this package's two +// error types, so a caller can tell a server failure from a local one without +// inspecting the message. +func TestFromRequestOnEveryRequestFailure(t *testing.T) { + t.Run("a status is an HTTPError", func(t *testing.T) { + c, _ := newServer(t, resp{status: 500, body: `{"title":"boom"}`}) + err := c.doJSON(http.MethodGet, c.baseURL+"/x", nil, nil, nil, timeoutRead) + var he *HTTPError + if !errors.As(err, &he) { + t.Fatalf("err = %v (%T), want *HTTPError", err, err) + } + if !FromRequest(err) { + t.Error("FromRequest = false, want true") + } + }) + + t.Run("a transport failure is a requestError", func(t *testing.T) { + // A server that is started and immediately closed: the address is + // well-formed and nothing is listening, which is what a dropped VPN + // looks like from here. + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + dead := srv.URL + srv.Close() + c := New(Config{SiteURL: dead, Username: "u", Token: "t"}) + err := c.doJSON(http.MethodGet, dead+"/x", nil, nil, nil, timeoutRead) + var re *requestError + if !errors.As(err, &re) { + t.Fatalf("err = %v (%T), want *requestError", err, err) + } + if !FromRequest(err) { + t.Error("FromRequest = false, want true") + } + }) + + t.Run("an undecodable body is a requestError", func(t *testing.T) { + c, _ := newServer(t, resp{status: 200, body: `not json at all`}) + var out struct { + ID string `json:"id"` + } + err := c.doJSON(http.MethodGet, c.baseURL+"/x", nil, nil, &out, timeoutRead) + var re *requestError + if !errors.As(err, &re) { + t.Fatalf("err = %v (%T), want *requestError", err, err) + } + if !FromRequest(err) { + t.Error("FromRequest = false, want true") + } + }) +} + +// TestFromRequestIsFalseForALocalError guards the direction that would turn an +// unreadable file into a network problem. +func TestFromRequestIsFalseForALocalError(t *testing.T) { + for _, tt := range []struct { + name string + err error + }{ + {"a plain error", errors.New("no title given")}, + {"a wrapped plain error", fmt.Errorf("reading: %w", errors.New("permission denied"))}, + {"nil", nil}, + } { + t.Run(tt.name, func(t *testing.T) { + if FromRequest(tt.err) { + t.Errorf("FromRequest(%v) = true, want false", tt.err) + } + }) + } +} + +// TestRequestErrorIsTransparent is what makes the tagging invisible: the text a +// reader sees, and every test asserting on it, is the inner error's own. +func TestRequestErrorIsTransparent(t *testing.T) { + inner := errors.New("dial tcp 127.0.0.1:1: connect: connection refused") + wrapped := wrapRequest(inner) + if got := wrapped.Error(); got != inner.Error() { + t.Errorf("Error() = %q, want %q", got, inner.Error()) + } + if !errors.Is(wrapped, inner) { + t.Error("errors.Is(wrapped, inner) = false, want true") + } + if wrapRequest(nil) != nil { + t.Error("wrapRequest(nil) != nil") + } +} From 9bbd608484e45942f17d09300f2a98d3bd248649 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:36:40 -0400 Subject: [PATCH 3/8] feat(jsonout): add CodeOr, classifying by whether a request failed A failure site that mixes local and server errors -- create's preflight, fix's page location, every attachment path -- has had only two wrong options. CodeFor answers NETWORK for anything that is not an *HTTPError, so "no title given" becomes a transport problem; a constant reports a rejected credential as a defect in a file that is fine. CodeOr asks client.FromRequest first, so a request failure classifies by status (and by RejectedCredential before it) while a local one takes the caller's fallback. The fallback is a parameter so the call site states which local meaning it means: VALIDATION where the file is wrong, IO where it could not be read. Refs #133 --- internal/jsonout/jsonout.go | 21 +++++++++ internal/jsonout/jsonout_test.go | 80 ++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/internal/jsonout/jsonout.go b/internal/jsonout/jsonout.go index db0b45d..ccafc70 100644 --- a/internal/jsonout/jsonout.go +++ b/internal/jsonout/jsonout.go @@ -135,3 +135,24 @@ func CodeFor(err error) Code { } return CodeAPI } + +// CodeOr classifies err the way CodeFor does when it came from a Confluence +// request, and returns fallback when it did not. +// +// This is what most failure sites want, and CodeFor alone is not: CodeFor +// answers NETWORK for any non-nil error that is not an *HTTPError, so a site +// that mixes local and server failures -- create's preflight, fix's page +// location, every attachment path -- would report "no title given" as a +// network problem. Passing everything to a constant is the other half of the +// same mistake, and is what reported a rejected credential as VALIDATION +// against a file that was fine (#133). +// +// fallback is a parameter rather than a hardcoded VALIDATION so the call site +// says which local meaning it is choosing: VALIDATION where a local failure +// means the file is wrong, IO where it means the file could not be read. +func CodeOr(err error, fallback Code) Code { + if client.FromRequest(err) { + return CodeFor(err) + } + return fallback +} diff --git a/internal/jsonout/jsonout_test.go b/internal/jsonout/jsonout_test.go index 6f2c8cd..c9782d0 100644 --- a/internal/jsonout/jsonout_test.go +++ b/internal/jsonout/jsonout_test.go @@ -3,6 +3,9 @@ package jsonout import ( "bytes" "errors" + "net/http" + "net/http/httptest" + "os" "strings" "testing" @@ -96,3 +99,80 @@ func TestCodeFor(t *testing.T) { } func errWrap(err error) error { return errors.Join(errors.New("context"), err) } + +// TestCodeOr covers both directions of #133: a server failure must not report a +// local code, and a local failure must not report a server one. +func TestCodeOr(t *testing.T) { + tests := []struct { + name string + err error + fallback Code + want Code + }{ + // The failure #133 exists for. A revoked token answers every v2 route + // with a 404 that names nothing, so the code has to come from CodeFor + // (which asks RejectedCredential first) and not from the fallback. + { + "rejected credential 404", + &client.HTTPError{StatusCode: 404, Body: `{"title":"Not Found"}`}, + CodeValidation, CodeAuth, + }, + {"403", &client.HTTPError{StatusCode: 403}, CodeValidation, CodeAuth}, + { + "genuine 404", + &client.HTTPError{StatusCode: 404, Body: `{"title":"Cannot find a page with id 1"}`}, + CodeValidation, CodeNotFound, + }, + {"500", &client.HTTPError{StatusCode: 500}, CodeValidation, CodeAPI}, + {"wrapped 403", errWrap(&client.HTTPError{StatusCode: 403}), CodeValidation, CodeAuth}, + // The hazard the issue names: routing everything through CodeFor would + // report a local defect as a network problem. + {"a local defect", errors.New("no title given"), CodeValidation, CodeValidation}, + // The mirror hazard, on the attachment paths: a file the client could + // not open is not a network failure. + {"an unreadable file", missingFileErr(t), CodeIO, CodeIO}, + {"a server failure under an IO fallback", &client.HTTPError{StatusCode: 403}, CodeIO, CodeAuth}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := CodeOr(tt.err, tt.fallback); got != tt.want { + t.Errorf("CodeOr(%v, %q) = %q, want %q", tt.err, tt.fallback, got, tt.want) + } + }) + } +} + +// TestCodeOrOnATransportFailure uses a real unreachable server rather than a +// hand-built error: the transport error type is unexported by design, and the +// point of the assertion is that a caller who cannot see the type still gets +// NETWORK rather than the local fallback. +// +// CreatePage, not a read: the client retries a transport failure only for an +// idempotent method, so a GET here would spend the full retry budget (four +// backoffs, ~15s) before reporting anything -- internal/client stubs the sleep +// for its own suite, and no other package can. +func TestCodeOrOnATransportFailure(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + dead := srv.URL + srv.Close() + c := client.New(client.Config{SiteURL: dead, Username: "u", Token: "t"}) + + _, err := c.CreatePage("space1", "Title", "", "") + if err == nil { + t.Fatal("CreatePage against a closed server returned no error") + } + if got := CodeOr(err, CodeValidation); got != CodeNetwork { + t.Errorf("CodeOr(transport failure) = %q, want NETWORK", got) + } +} + +// missingFileErr returns a real *fs.PathError, the shape a failed checksum of a +// local attachment produces. +func missingFileErr(t *testing.T) error { + t.Helper() + _, err := os.Open(t.TempDir() + "/does-not-exist") + if err == nil { + t.Fatal("opening a nonexistent file succeeded") + } + return err +} From 096e7212efb73a63be133dd31185d3ae8ddc944a Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:38:20 -0400 Subject: [PATCH 4/8] fix(create): report a preflight HTTP failure by its status, not VALIDATION Phase 1 makes four kinds of server call -- checkPageID, ResolveSpaceID, checkParentInSpace, checkTitleFree -- and newFailure stamped every phase-1 error VALIDATION, the code that means "there is something wrong with your file". So a 403, a 500, or a rejected credential from any of them blamed the file. The rejected credential is the case that matters. It arrives as a 404 on every v2 route, GetPageOrNil does not read that one as "absent", and CodeFor asks RejectedCredential before its status switch precisely so it reports AUTH -- which create then threw away. The same token already reported AUTH from phase 3, so one credential produced two different codes depending on which phase noticed. newFailure now defaults through jsonout.CodeOr. Every local phase-1 error is not a client error, so it still takes VALIDATION, and the convertFailure check stays after it so CONVERT still wins. Fixes #133 --- cmd/create/create.go | 15 +++++- cmd/create/run_test.go | 102 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) diff --git a/cmd/create/create.go b/cmd/create/create.go index 85926ab..b9391ab 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -189,12 +189,25 @@ func validationFailure(filename, message string) failure { // newFailure records a phase-1 error against a file, carrying over the fields of // a page_id failure so abort() can report them without re-fetching anything, // and the code the error's type implies. +// +// The code defaults through jsonout.CodeOr rather than to VALIDATION, because +// phase 1 makes four kinds of server call -- checkPageID, ResolveSpaceID, +// checkParentInSpace, checkTitleFree -- and each can fail for reasons that have +// nothing to do with the file. A hardcoded VALIDATION reported a revoked token +// as a defect in a file that was perfectly fine, which is the worst case +// because a rejected credential arrives as a 404 on every v2 route and +// GetPageOrNil deliberately does not swallow that one (#133). Every local +// error here -- no title, no space, a taken page_id, a title clash, a parent +// conflict -- is not a client error, so it still takes the fallback. func newFailure(filename string, err error) failure { - f := failure{filename: filename, message: err.Error(), code: jsonout.CodeValidation} + f := failure{filename: filename, message: err.Error(), code: jsonout.CodeOr(err, jsonout.CodeValidation)} var pf *pageIDFailure if errors.As(err, &pf) { f.pageID, f.url = pf.pageID, pf.url } + // After CodeOr: a converter failure is neither a request nor a plain + // validation error, and CONVERT is what phase 3 reported for it before the + // check moved into preflight. var cf *convertFailure if errors.As(err, &cf) { f.code = jsonout.CodeConvert diff --git a/cmd/create/run_test.go b/cmd/create/run_test.go index b545fb6..c153bc2 100644 --- a/cmd/create/run_test.go +++ b/cmd/create/run_test.go @@ -41,6 +41,15 @@ type fakeConfluence struct { // created under that title -- used to test a publish-phase failure after a // successful reserve. failUpdateForTitle string + // rejectCredential makes every route answer the way the API answers a + // revoked token: 404 with a title that names nothing. This is the shape + // #133 is about -- it is not a missing page, and reporting it as one (or as + // a defect in the file) sends the reader to check an id that was never the + // problem. + rejectCredential bool + // spacesStatus, when non-zero, is the status the space lookup answers with, + // for a preflight server failure that is not a credential rejection. + spacesStatus int } type fakePage struct { @@ -60,8 +69,19 @@ func (f *fakeConfluence) handle(w http.ResponseWriter, r *http.Request) { f.mu.Lock() defer f.mu.Unlock() + if f.rejectCredential { + w.WriteHeader(http.StatusNotFound) + _, _ = fmt.Fprint(w, `{"statusCode":404,"title":"Not Found"}`) + return + } + switch { case r.Method == http.MethodGet && r.URL.Path == "/wiki/api/v2/spaces": + if f.spacesStatus != 0 { + w.WriteHeader(f.spacesStatus) + _, _ = fmt.Fprint(w, `{"statusCode":403,"message":"no"}`) + return + } _, _ = fmt.Fprint(w, `{"results":[{"id":"space1"}]}`) case r.Method == http.MethodGet && r.URL.Path == "/wiki/api/v2/pages": @@ -642,6 +662,88 @@ func TestRunConversionFailureReportsCONVERT(t *testing.T) { } } +// TestRunPreflightRejectedCredentialReportsAUTH is #133's worst case. A revoked +// token answers every v2 route with a 404 naming nothing, and GetPageOrNil +// deliberately does not read that as "absent" -- so checkPageID hands back the +// *HTTPError and preflight used to stamp it VALIDATION, blaming a file that is +// perfectly fine. Asserted alongside a genuine VALIDATION failure in the same +// envelope, since a code that were always AUTH would pass the first half alone. +func TestRunPreflightRejectedCredentialReportsAUTH(t *testing.T) { + resetOpts(t) + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + dir := t.TempDir() + spaceOpt = "ENG" + withID := write(t, dir, "withid.md", "---\ntitle: With Id\npage_id: 123\n---\nbody\n") + untitled := write(t, dir, "untitled.md", "---\ntitle: \"\"\n---\nbody\n") + + c, fake := newFakeConfluence(t) + fake.rejectCredential = true + out, runErr := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{withID, untitled}) + }) + if runErr == nil { + t.Fatal("run should have failed") + } + schematest.ValidateEnvelope(t, []byte(out)) + + codes := resultCodes(t, out) + if got := codes["withid.md"]; got != string(jsonout.CodeAuth) { + t.Errorf("withid.md code = %q, want AUTH -- a rejected credential is not a defect in the file", got) + } + if got := codes["untitled.md"]; got != string(jsonout.CodeValidation) { + t.Errorf("untitled.md code = %q, want VALIDATION", got) + } +} + +// TestRunPreflightServerFailureIsNotVALIDATION covers the other three preflight +// calls through the space lookup: a 403 there is the server refusing, and +// nothing about it is knowable from the file. +func TestRunPreflightServerFailureIsNotVALIDATION(t *testing.T) { + resetOpts(t) + ui.SetJSON(true) + t.Cleanup(func() { ui.SetJSON(false) }) + dir := t.TempDir() + spaceOpt = "ENG" + path := write(t, dir, "a.md", "---\ntitle: A\n---\nbody\n") + + c, fake := newFakeConfluence(t) + fake.spacesStatus = http.StatusForbidden + out, runErr := captureStdout(t, func() error { + return run(testCmd(t, c.SiteURL(), dir), []string{path}) + }) + if runErr == nil { + t.Fatal("run should have failed") + } + schematest.ValidateEnvelope(t, []byte(out)) + + if got := resultCodes(t, out)["a.md"]; got != string(jsonout.CodeAuth) { + t.Errorf("a.md code = %q, want AUTH", got) + } +} + +// resultCodes maps each result's base filename to the code it reported, +// skipping results that carry none (a file the batch never reached). +func resultCodes(t *testing.T, out string) map[string]string { + t.Helper() + var env struct { + Results []struct { + File string `json:"file"` + Code *string `json:"code"` + } `json:"results"` + } + if err := json.Unmarshal([]byte(out), &env); err != nil { + t.Fatalf("unmarshal %q: %v", out, err) + } + codes := map[string]string{} + for _, r := range env.Results { + if r.Code != nil { + codes[filepath.Base(r.File)] = *r.Code + } + } + return codes +} + // captureStdout runs fn with os.Stdout redirected, returning what it printed. func captureStdout(t *testing.T, fn func() error) (string, error) { t.Helper() From 6bbb82ae40fb6557101d848761c6c3002619eae9 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:39:44 -0400 Subject: [PATCH 5/8] refactor(fix): classify a locate failure through jsonout.CodeOr locateCode was the rule create needed, so it moved to internal/jsonout rather than being copied. fix loses nothing and gains the transport case: its own type check reported VALIDATION for a dial failure, because doJSON builds an *HTTPError only once there is a status. Its unit test is replaced by one running through processFile, so what is pinned is the wiring rather than a helper that no longer lives here -- including the assertion #133 is actually about, that the credential create's preflight now reports AUTH reports AUTH here too. Refs #133 --- cmd/fix/fix.go | 7 ++++- cmd/fix/json.go | 12 -------- cmd/fix/json_test.go | 69 ++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 69 insertions(+), 19 deletions(-) diff --git a/cmd/fix/fix.go b/cmd/fix/fix.go index 0e9749e..7ca79d9 100644 --- a/cmd/fix/fix.go +++ b/cmd/fix/fix.go @@ -115,7 +115,12 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult { } page, err := locatePage(mf.Frontmatter, c) if err != nil { - return r.fail(err, locateCode(err)) + // locatePage mixes server failures (GetPageOrNil, SearchPagesByTitle) + // with local ones (no page_id or title, an ambiguous title), so the code + // comes from the error's origin. This was fix's own locateCode, lifted + // into jsonout when create needed the identical rule (#133); the + // transport case is what a bare type check got wrong here too. + return r.fail(err, jsonout.CodeOr(err, jsonout.CodeValidation)) } r.pageID = page.ID diff --git a/cmd/fix/json.go b/cmd/fix/json.go index 07ee762..dfdf3eb 100644 --- a/cmd/fix/json.go +++ b/cmd/fix/json.go @@ -1,10 +1,8 @@ package fix import ( - "errors" "fmt" - "github.com/mozilla/markfluence/internal/client" "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/ui" ) @@ -137,16 +135,6 @@ func summarize(results []*fixResult) map[string]int { return s } -// locateCode classifies a page-location failure: an HTTP status maps via CodeFor, -// anything else is a frontmatter/target problem (VALIDATION). -func locateCode(err error) jsonout.Code { - var he *client.HTTPError - if errors.As(err, &he) { - return jsonout.CodeFor(err) - } - return jsonout.CodeValidation -} - func nullableStr(s string) *string { if s == "" { return nil diff --git a/cmd/fix/json_test.go b/cmd/fix/json_test.go index 9519224..4b28b0f 100644 --- a/cmd/fix/json_test.go +++ b/cmd/fix/json_test.go @@ -3,9 +3,12 @@ package fix import ( "bytes" "encoding/json" + "net/http" + "os" + "path/filepath" "testing" - "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/schematest" ) @@ -108,12 +111,66 @@ func TestSummarize(t *testing.T) { } } -func TestLocateCode(t *testing.T) { - if got := locateCode(&client.HTTPError{StatusCode: 404}); got != jsonout.CodeNotFound { - t.Errorf("locateCode(404) = %q, want NOT_FOUND", got) +// TestProcessFileClassifiesALocateFailureByOrigin is the consistency claim of +// #133 asserted from fix's side: the same rejected credential that create's +// preflight reports AUTH must report AUTH here too. It runs through processFile +// rather than the classifier, so what is pinned is the wiring -- a genuine 404 +// still reports NOT_FOUND, and a file with nothing to locate by still reports +// VALIDATION. +func TestProcessFileClassifiesALocateFailureByOrigin(t *testing.T) { + tests := []struct { + name string + body string + status int + respBody string + want jsonout.Code + }{ + { + "a rejected credential is AUTH, not NOT_FOUND", + "---\ntitle: A\npage_id: 123\n---\nbody\n", + http.StatusNotFound, `{"statusCode":404,"title":"Not Found"}`, + jsonout.CodeAuth, + }, + { + // A genuine 404 never reaches the classifier: GetPageOrNil reports + // the page as absent, and locatePage turns that into its own + // message about the id in the file. + "a genuine 404 is a local failure about the id", + "---\ntitle: A\npage_id: 123\n---\nbody\n", + http.StatusNotFound, `{"statusCode":404,"title":"Cannot find a page with id 123"}`, + jsonout.CodeValidation, + }, + { + "a 500 is API", + "---\ntitle: A\npage_id: 123\n---\nbody\n", + http.StatusInternalServerError, `boom`, + jsonout.CodeAPI, + }, + { + "nothing to locate by is VALIDATION", + "---\nspace: ENG\n---\nbody\n", + http.StatusOK, `{"results":[]}`, + jsonout.CodeValidation, + }, } - if got := locateCode(errString("no page_id or title")); got != jsonout.CodeValidation { - t.Errorf("locateCode(logic) = %q, want VALIDATION", got) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := clienttest.New(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(tt.respBody)) + }) + path := filepath.Join(t.TempDir(), "a.md") + if err := os.WriteFile(path, []byte(tt.body), 0o644); err != nil { + t.Fatal(err) + } + r := processFile(path, c) + if r.ok { + t.Fatal("processFile should have failed") + } + if r.code != tt.want { + t.Errorf("code = %q, want %q (error: %s)", r.code, tt.want, r.errMsg) + } + }) } } From 6c571c6d2173cbcc2deb9e015abc79d7b04742b1 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:42:53 -0400 Subject: [PATCH 6/8] fix: report a local attachment failure as IO, not NETWORK #133 inverted, at four sites. client.planAttachments checksums every local file, so SyncAttachments/PlanAttachments/ForceUploadAttachments fail with an os.Open error when an asset cannot be read -- and bare CodeFor answers NETWORK for anything without an HTTP status, so --json blamed the network for a file on disk. create's publishOne names this exact condition as one of S7's residuals ("an image that Lstat'd fine in preflight can still be unreadable now"). attachment-upload is the worst of the four: its whole input is local files, and it already separates IO from VALIDATION upstream in localAttachmentsCode before losing the distinction one call later. attachfile.Write is the download direction, where DownloadAttachment writes to the destination file as it goes. All four now pass IO as the fallback, so a server failure on the same call still classifies by its status -- which is what the new attachment-upload test pins from both sides. Refs #133 --- cmd/attachmentupload/attachmentupload.go | 15 ++++- cmd/attachmentupload/attachmentupload_test.go | 56 +++++++++++++++++++ cmd/create/create.go | 6 +- cmd/update/update.go | 7 ++- internal/attachfile/attachfile.go | 6 +- 5 files changed, 85 insertions(+), 5 deletions(-) diff --git a/cmd/attachmentupload/attachmentupload.go b/cmd/attachmentupload/attachmentupload.go index 3be6394..7c72a7e 100644 --- a/cmd/attachmentupload/attachmentupload.go +++ b/cmd/attachmentupload/attachmentupload.go @@ -101,7 +101,7 @@ func run(cmd *cobra.Command, args []string) error { actions, err := plan(c, pageID, attachments) if err != nil { - return operationalFail(pageID, err, jsonout.CodeFor(err), roots) + return operationalFail(pageID, err, planCode(err), roots) } return report(actions, roots) } @@ -146,6 +146,19 @@ func forced(actions []client.SyncAction) []client.SyncAction { // collision or a directory passed where a file was meant. type badInput struct{ error } +// planCode maps a plan failure to its --json code. Not bare CodeFor: plan +// checksums every local file, so an unreadable one fails here -- and this +// command's whole input is local files. It already tells IO from VALIDATION +// upstream (localAttachmentsCode) and used to lose the distinction one call +// later, reporting a file it could not read as a network failure. A refused +// listing still classifies by its status. +// +// Named rather than inlined so a test can assert the decision this command +// actually makes, instead of re-deriving the same expression beside it. +func planCode(err error) jsonout.Code { + return jsonout.CodeOr(err, jsonout.CodeIO) +} + // localAttachmentsCode maps a localAttachments failure to its --json code. func localAttachmentsCode(err error) jsonout.Code { var bad badInput diff --git a/cmd/attachmentupload/attachmentupload_test.go b/cmd/attachmentupload/attachmentupload_test.go index cbd3a0d..5a2711e 100644 --- a/cmd/attachmentupload/attachmentupload_test.go +++ b/cmd/attachmentupload/attachmentupload_test.go @@ -1,6 +1,7 @@ package attachmentupload import ( + "net/http" "os" "path" "path/filepath" @@ -8,6 +9,8 @@ import ( "testing" "github.com/mozilla/markfluence/internal/client" + "github.com/mozilla/markfluence/internal/clienttest" + "github.com/mozilla/markfluence/internal/jsonout" "github.com/mozilla/markfluence/internal/project" ) @@ -251,3 +254,56 @@ func TestLocalAttachmentsUnusableNameReportsWhatWasTyped(t *testing.T) { } } } + +// TestPlanFailureCodeSeparatesServerFromLocal is the guard on the flipped +// fallback. plan() checksums every local file, so an unreadable one is an IO +// failure rather than the NETWORK that CodeFor answers for anything without an +// HTTP status -- but the fallback must not swallow a real server failure on the +// way: the attachment listing plan() makes first can be refused. +func TestPlanFailureCodeSeparatesServerFromLocal(t *testing.T) { + dir := t.TempDir() + good := writeFile(t, dir, "img.png") + + tests := []struct { + name string + handler http.HandlerFunc + files []string + want jsonout.Code + }{ + { + "a refused listing is AUTH", + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"caller cannot access Confluence"}`)) + }, + []string{good}, + jsonout.CodeAuth, + }, + { + "a file the checksum cannot read is IO", + func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"results":[]}`)) + }, + []string{filepath.Join(dir, "gone.png")}, + jsonout.CodeIO, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := clienttest.New(t, tt.handler) + atts := make([]client.LocalAttachment, 0, len(tt.files)) + for _, f := range tt.files { + atts = append(atts, client.LocalAttachment{ + Path: f, Filename: filepath.Base(f), Source: filepath.Base(f), + }) + } + _, err := plan(c, "123", atts) + if err == nil { + t.Fatal("plan should have failed") + } + if got := planCode(err); got != tt.want { + t.Errorf("code = %q, want %q (error: %v)", got, tt.want, err) + } + }) + } +} diff --git a/cmd/create/create.go b/cmd/create/create.go index b9391ab..4b48603 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -564,9 +564,13 @@ func publishOne(r record, res *createResult, pageID string, version int, c *clie } res.url = c.PageURL(result, pageID) + // CodeOr, not CodeFor: SyncAttachments opens every asset to checksum and + // upload it, so a file the converter saw but cannot now read fails here -- + // the S7 residual named above -- and CodeFor would report a local read + // failure as NETWORK. A server failure still classifies by its status. actions, err := c.SyncAttachments(pageID, pageContent.Attachments) if err != nil { - return res.fail(err, jsonout.CodeFor(err)) + return res.fail(err, jsonout.CodeOr(err, jsonout.CodeIO)) } for _, a := range actions { res.attachments = append(res.attachments, jsonout.Attachment{Action: a.Action, Filename: a.Filename}) diff --git a/cmd/update/update.go b/cmd/update/update.go index a9ee7d5..26a0a7f 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -228,7 +228,7 @@ func processFile( if dryRun { actions, err := c.PlanAttachments(pageID, pageContent.Attachments) if err != nil { - return r.fail(err, jsonout.CodeFor(err)) + return r.fail(err, jsonout.CodeOr(err, jsonout.CodeIO)) } for _, a := range actions { r.attachments = append(r.attachments, jsonout.Attachment{Action: a.Action, Filename: a.Filename}) @@ -240,9 +240,12 @@ func processFile( return r } + // CodeOr, not CodeFor: planning an upload checksums every local asset, so + // a file that cannot be read fails here and is an IO failure, not a + // network one. Same for PlanAttachments in the dry-run above. actions, err := c.SyncAttachments(pageID, pageContent.Attachments) if err != nil { - return r.fail(err, jsonout.CodeFor(err)) + return r.fail(err, jsonout.CodeOr(err, jsonout.CodeIO)) } for _, a := range actions { r.attachments = append(r.attachments, jsonout.Attachment{Action: a.Action, Filename: a.Filename}) diff --git a/internal/attachfile/attachfile.go b/internal/attachfile/attachfile.go index 2ff5360..04eeb71 100644 --- a/internal/attachfile/attachfile.go +++ b/internal/attachfile/attachfile.go @@ -222,7 +222,11 @@ func Write(c *client.ConfluenceClient, a client.Attachment, opts Options) Outcom // masking a download that never actually completed. _ = f.Close() _ = rootFS.Remove(rel) - res.Status, res.Err, res.Code = StatusFailed, err, jsonout.CodeFor(err) + // CodeOr, not CodeFor: DownloadAttachment writes to f as it goes, so a + // full or unwritable destination fails here and is an IO failure. (An + // attachment record with no download link takes the same fallback, + // which is a known imprecision: it is neither request nor local.) + res.Status, res.Err, res.Code = StatusFailed, err, jsonout.CodeOr(err, jsonout.CodeIO) return res } res.Status = StatusDownloaded From ebc2971fa4f5b3aaf30ff09a2844e6a809e18980 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Mon, 7 Sep 2026 09:43:42 -0400 Subject: [PATCH 7/8] docs: record the two client error types and CodeOr CLAUDE.md's internal/client bullet: the package returns an *HTTPError once there is a status and an unexported requestError when there is none, FromRequest answers for both, and the rule is scoped to the request -- the writer, the file, and the environment stay untyped on purpose. The plan is amended where implementation diverged from it: the pagination helpers turned out to have no wrap site (resolveNext swallows its own url.Parse failure), attachment-upload can test the local direction after all because no converter stands in the way, and the page_id-resolves-to- nothing split that fix's test surfaced is recorded as out of scope. Refs #133 --- CLAUDE.md | 2 +- _plans/035_error-code-classification.md | 54 ++++++++++++++++++------- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9df7f67..bb8c8ae 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -64,7 +64,7 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `internal/attachfile` — `Resolve` (where an attachment goes under a destination root, **including the traversal clamp**) and `Write` (download it there, honoring force/dry-run). Shared by `attachment-download` and `export`; the clamp must never exist in two copies. - `internal/pagetree` — `Walk`, the traversal of pages *and folders* under a node, plus `WalkSpace` (the same traversal seeded from a space's root pages, via `client.ListSpaceRootPages`) and `AllDepths`. Both go through one `walker`, so the depth rule and the visited guard exist in a single copy. It is a package rather than command-local because listing a subtree and exporting one (#59) need the identical walk, and its rules must not exist in two copies: siblings arrive from two requests (`/child/page`, `/child/folder`) and are **merged by `extensions.position`**, or the output loses the order Confluence displays; a folder **counts as a level** like a page, which is only reasonable because folders are reported rather than silently traversed; and the walk descends folders even when only pages matter, since a folder may hold the only pages in a subtree. `nodeURL` uses `SiteURL()` — a v1 child row carries `webui` but no `base`. A visited set guards the unbounded case. - `internal/pageref` — `Resolve`, the single page-argument resolver: a numeric id, a Confluence page **or folder** URL (`pagePathRE` matches both `/pages/` and `/folder/`, since `children` takes a folder and a folder URL is what a browser hands you — the id is all it returns, so a command that can only use a page reports its own not-found), or a `.md` file whose frontmatter has a `page_id` (stat'd first, so `123.md` is a file). Every command taking a page uses it. `message.go` also owns the wording for the two ways a *frontmatter* `page_id` is wrong — `NotFoundMessage` (caller supplies the remedy, which differs per command) and `NotNumericMessage` — because `create`, `update`, and `fix` all report them and a reader should recognize the same problem across all three. They return strings, not errors: `create` wraps the text in its typed `pageIDFailure` (which also carries the `--json` fields), the others want a plain error. Anything checking a `page_id` before a request uses `IsDigits`, since the API answers a non-numeric id with a 400 whose body says nothing useful. -- `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). A **folder** — the Cloud content type that can parent a page — has its own v2 route, `GetFolderOrNil` against `/wiki/api/v2/folders/{id}`, because every v2 *page* route answers a folder id with 404; enumerating children, if it is ever added, must be v1, since v2 cannot list inside a folder at all and its page-children route silently omits folders ([docs/confluence/folders.md](docs/confluence/folders.md)). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send`. `HTTPError.Error()` appends a **hint** for the three auth failures whose status misleads, matched on the response *body* rather than deduced from the status and always **appended** to it, never replacing it. The one that matters: **a rejected credential is a 404 on every v2 route**, so a revoked token used to make `read` answer `page ... not found` about a page that exists. `RejectedCredential` tells it apart by the fact that every genuine v2 404 *names* what it could not find and the auth one does not, `notFound` gates the three `…OrNil` helpers on it so they stop reading it as "absent", and `jsonout.CodeFor` checks it before the status switch so `--json` reports `AUTH` rather than `NOT_FOUND`. A 403 that is not one of the two measured credential phrasings gets no hint, because that is what a genuine permission denial looks like ([docs/confluence/api.md](docs/confluence/api.md#scopes)). **Retry rules**: 429 for any method; 502/503/504 for idempotent methods; **any other 5xx only when the response carries `Retry-After`** — that is how a 500 becomes retryable, and it is why `parseRetryAfter` reports the header's *presence* apart from its delay (`Retry-After: 0` means "retry now", not "no header"). The exponential delay is jittered, a server-supplied `Retry-After` never is. Decisions go to a package-level hook (`SetRetryLogger`, set once in `root.go` beside `ui.SetDebug`) and fire whichever way they went, because `internal/client` prints nothing and a silent twelve-minute retry storm is otherwise indistinguishable from a hang. **A versioned PUT is not as idempotent as its method**: `SetContentProperty` retry-once on top (recovers a lost create-POST response) and `UpdatePage`'s `updateLanded` both exist for the same reason — a write whose response was lost gets re-sent, and the re-sent version is refused. `updateLanded` requires version *and* title *and* body to match what was sent, since a concurrent edit could have produced the version alone and claiming success over someone else's content is worse than a false failure ([docs/confluence/api.md](docs/confluence/api.md)). `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; only the current comment form is parsed — an attachment stamped by a markfluence predating a comment-format change reads as unmanaged and is re-uploaded once, the same as any hand-uploaded file — except that a *recorded path disagreeing with the local source* is an update even when the checksum matches, so a mangled path repairs itself instead of surviving every later publish; a comment with no source recorded at all is not a disagreement. Every text part of the upload form must go through `writeTextField`, never `multipart.Writer.WriteField`, which emits no charset and gets decoded as Latin-1), `_links.next` pagination. **Three pagination schemes, and picking the wrong one truncates silently.** v1 *child/attachment* collections page through the generic `listV1` helper by `start`/`limit` offset, never `_links.next` (absent when the results fit one page, so it cannot terminate a loop); `ListAttachments`, `ListChildPages`, and `ListChildFolders` all go through it. v2 collections page through `listV2` by the cursor in `_links.next`, which is a `/wiki`-prefixed absolute path `resolveNext` handles unchanged; `ListContentProperties` and `SearchPagesByTitle` share it. **`/wiki/rest/api/search` is neither**: it ignores `start` outright, its `next` is context-relative so it needs the `/wiki` prefix `resolveNext` does not add, a short page does *not* mean the end, and `totalSize` can be nonzero against an empty `results` — so `searchCQL` terminates only on a missing `next` and nothing may branch on `totalSize` ([docs/confluence/search.md](docs/confluence/search.md)). `searchCQLBounded` adds a row bound under it (`SearchCQL` is that call with no bound, which is why `find` is unaffected): it asks for `max+1` and reports the surplus as `more`, since `totalSize` cannot supply a count. Full text goes through `SearchText`/`SearchRawCQL`, which return the cleaned `SearchMatch` the way `FindByTitle` returns `TitleMatch` — and **every field of a match comes from the row's `content` object**, because the row-level `title` is HTML-escaped *and* wrapped in `@@@hl@@@` markers where `content.title` is neither. The `excerpt` exists only at row level, so `cleanExcerpt` strips those markers, unescapes once, and collapses to one line — in the client, so the human and `--json` paths cannot disagree about it. `excerpt=highlight` is passed explicitly and **re-attached when following the cursor** (the `next` link carries `cql` and `limit` but not `excerpt`, and `doJSON` appends params with a bare `?`); an unrecognized value there yields an empty excerpt with a 200, so a rename by Atlassian degrades to no excerpts rather than an error. A row with no `content` object is skipped and **counted** — `type = space` answers with hundreds of them, and a silent skip would report a successful empty result. A bare v1 child row already carries `webui`, `status`, and `extensions.position`, so child listing needs no `expand`. `DownloadAttachment` goes through `send` (inheriting retry/backoff) against `_links.download`; **never** add a `CheckRedirect` that forwards headers — it would leak site credentials to Atlassian's media host, which neither needs nor wants them. `config.go` holds `Resolve` and the `.env` reader. Why each of these is shaped this way, with the evidence: [docs/confluence/api.md](docs/confluence/api.md) and [attachments.md](docs/confluence/attachments.md). +- `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). A **folder** — the Cloud content type that can parent a page — has its own v2 route, `GetFolderOrNil` against `/wiki/api/v2/folders/{id}`, because every v2 *page* route answers a folder id with 404; enumerating children, if it is ever added, must be v1, since v2 cannot list inside a folder at all and its page-children route silently omits folders ([docs/confluence/folders.md](docs/confluence/folders.md)). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send`. `HTTPError.Error()` appends a **hint** for the three auth failures whose status misleads, matched on the response *body* rather than deduced from the status and always **appended** to it, never replacing it. The one that matters: **a rejected credential is a 404 on every v2 route**, so a revoked token used to make `read` answer `page ... not found` about a page that exists. `RejectedCredential` tells it apart by the fact that every genuine v2 404 *names* what it could not find and the auth one does not, `notFound` gates the three `…OrNil` helpers on it so they stop reading it as "absent", and `jsonout.CodeFor` checks it before the status switch so `--json` reports `AUTH` rather than `NOT_FOUND`. **Two error types on the request path, and one predicate for them**: an `*HTTPError` once a response has a status, an unexported `requestError` when there is none (a transport failure, a request that would not build, a body that would not decode), and `FromRequest` answers whether an error is either. That is what lets a caller tell a server failure from a local one — `jsonout.CodeOr(err, fallback)` is the whole point of it, since `CodeFor` alone reports every non-`HTTPError` as `NETWORK` and so turns `no title given` into a network problem (#133). The rule is deliberately scoped to the request: `DownloadAttachment` writing to the caller's writer, `uploadAttachment` opening the caller's file, and `Resolve` reading the environment stay untyped, because tagging them would misreport an unreadable file as a network failure. The wrapper carries no message of its own, so `Error()` is the inner text verbatim and nothing a reader sees changed. A 403 that is not one of the two measured credential phrasings gets no hint, because that is what a genuine permission denial looks like ([docs/confluence/api.md](docs/confluence/api.md#scopes)). **Retry rules**: 429 for any method; 502/503/504 for idempotent methods; **any other 5xx only when the response carries `Retry-After`** — that is how a 500 becomes retryable, and it is why `parseRetryAfter` reports the header's *presence* apart from its delay (`Retry-After: 0` means "retry now", not "no header"). The exponential delay is jittered, a server-supplied `Retry-After` never is. Decisions go to a package-level hook (`SetRetryLogger`, set once in `root.go` beside `ui.SetDebug`) and fire whichever way they went, because `internal/client` prints nothing and a silent twelve-minute retry storm is otherwise indistinguishable from a hang. **A versioned PUT is not as idempotent as its method**: `SetContentProperty` retry-once on top (recovers a lost create-POST response) and `UpdatePage`'s `updateLanded` both exist for the same reason — a write whose response was lost gets re-sent, and the re-sent version is refused. `updateLanded` requires version *and* title *and* body to match what was sent, since a concurrent edit could have produced the version alone and claiming success over someone else's content is worse than a false failure ([docs/confluence/api.md](docs/confluence/api.md)). `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; only the current comment form is parsed — an attachment stamped by a markfluence predating a comment-format change reads as unmanaged and is re-uploaded once, the same as any hand-uploaded file — except that a *recorded path disagreeing with the local source* is an update even when the checksum matches, so a mangled path repairs itself instead of surviving every later publish; a comment with no source recorded at all is not a disagreement. Every text part of the upload form must go through `writeTextField`, never `multipart.Writer.WriteField`, which emits no charset and gets decoded as Latin-1), `_links.next` pagination. **Three pagination schemes, and picking the wrong one truncates silently.** v1 *child/attachment* collections page through the generic `listV1` helper by `start`/`limit` offset, never `_links.next` (absent when the results fit one page, so it cannot terminate a loop); `ListAttachments`, `ListChildPages`, and `ListChildFolders` all go through it. v2 collections page through `listV2` by the cursor in `_links.next`, which is a `/wiki`-prefixed absolute path `resolveNext` handles unchanged; `ListContentProperties` and `SearchPagesByTitle` share it. **`/wiki/rest/api/search` is neither**: it ignores `start` outright, its `next` is context-relative so it needs the `/wiki` prefix `resolveNext` does not add, a short page does *not* mean the end, and `totalSize` can be nonzero against an empty `results` — so `searchCQL` terminates only on a missing `next` and nothing may branch on `totalSize` ([docs/confluence/search.md](docs/confluence/search.md)). `searchCQLBounded` adds a row bound under it (`SearchCQL` is that call with no bound, which is why `find` is unaffected): it asks for `max+1` and reports the surplus as `more`, since `totalSize` cannot supply a count. Full text goes through `SearchText`/`SearchRawCQL`, which return the cleaned `SearchMatch` the way `FindByTitle` returns `TitleMatch` — and **every field of a match comes from the row's `content` object**, because the row-level `title` is HTML-escaped *and* wrapped in `@@@hl@@@` markers where `content.title` is neither. The `excerpt` exists only at row level, so `cleanExcerpt` strips those markers, unescapes once, and collapses to one line — in the client, so the human and `--json` paths cannot disagree about it. `excerpt=highlight` is passed explicitly and **re-attached when following the cursor** (the `next` link carries `cql` and `limit` but not `excerpt`, and `doJSON` appends params with a bare `?`); an unrecognized value there yields an empty excerpt with a 200, so a rename by Atlassian degrades to no excerpts rather than an error. A row with no `content` object is skipped and **counted** — `type = space` answers with hundreds of them, and a silent skip would report a successful empty result. A bare v1 child row already carries `webui`, `status`, and `extensions.position`, so child listing needs no `expand`. `DownloadAttachment` goes through `send` (inheriting retry/backoff) against `_links.download`; **never** add a `CheckRedirect` that forwards headers — it would leak site credentials to Atlassian's media host, which neither needs nor wants them. `config.go` holds `Resolve` and the `.env` reader. Why each of these is shaped this way, with the evidence: [docs/confluence/api.md](docs/confluence/api.md) and [attachments.md](docs/confluence/attachments.md). - `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, root *project.Root, index *linkindex.Index, baseURL, spaceKey, version string) (*ConfluencePage, error)`. `root` bounds which images and parent references may be read (S1/S2) and is what an image's recorded `Source` is relative to; `index` is the tree-wide link/anchor index for `root` (`internal/linkindex.Build`), built once and shared across every file converted under it rather than rebuilt per conversion — both are discovered/built by the caller (`internal/project`/`internal/linkindex`), which is why this package stays client-free. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `aclink.go` is the *inverse* direction's one element with enough shape to need its own file — ``, which the editor writes for every internal link and `MdToConfluence` never emits, so nothing in the regression suite covers it. One rule decides its whole mapping: **convert when the markdown republishes to a link resolving to the same target, pass the storage through when it would not** — so a page link and a space link convert, while a mention (80% of all real usage), an attachment link (only images are uploaded, so a relative href would be dead) and an unresolvable target stay raw, which the shield republishes byte-identical. A page target is a **title, never an id**, so `PageLinkTargets` reports what needs resolving and `StorageOptions.PageLinks` carries the answers back. An `ac:anchor` is **percent-encoded** where `confluenceSlug` output is not: decode it before matching a heading, leave it encoded inside a URL. A same-page anchor recovers its heading from the document rather than inverting the slug, which is impossible — `confluenceSlug` turns both a space and a hyphen into `-`. The survey the mapping rests on, and the `xml.HTMLAutoClose` trap that made `` crash the parser outright (#88), are in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); `attachname.go` owns the source-path→attachment-name mapping, which is now the path's **base name** and nothing else (#59/`_plans/029`): the name is the attachment's identity, so an encoded path moved the name every time the file moved and orphaned the old attachment, and the path is recorded in the comment anyway. The mapping is therefore lossy, and what the bijection used to buy is an explicit refusal — two assets in one document whose base names agree return a typed `NameCollisionError` from `MdToConfluence`, which is a *failure* and not a `Broken` entry, since nothing blocks a publish on `Broken`. `check` catches that error and reports it as `Broken` anyway, because there it is a document defect like a dead link rather than a converter failure. A stored name is never interpreted in the other direction either: `sourceFor` reads the recorded path or uses the name verbatim. What names Confluence accepts is in [docs/confluence/attachments.md](docs/confluence/attachments.md); `destination.go` owns the **other** codec, destination↔path (`decodeDestination`/`encodeDestination`), shared by images *and* doc links — a markdown destination is a URL, so decode inbound (**before** `withinRoot`, or an encoded `..%2F` slips the clamp) and encode outbound in `storage_to_md.go` (or `export` emits markdown that no longer parses, and `sourceFor`'s absolute-path refusal is undone by the next read); an undecodable destination is a literal `%` in a filename, not an error; the reasoning is in [docs/confluence/links-and-anchors.md](docs/confluence/links-and-anchors.md); `images.go` (resolution stays page-relative like GitHub; the documentation root — cwd — bounds what may be published, and an image above it is `IMAGE BROKEN`), `links.go` (GitHub/Confluence slugs, doc-link + anchor rewriting against `internal/linkindex`'s tree-wide index; `resolveDocKey` resolves a destination to the index's root-relative key and reports `escapes` — a purely lexical check on the *query* side, since the index itself needs no clamp: an escaping key can never be in it, built by walking downward from root). A doc-link target is one of four severities, #42: missing entirely or escaping root is **Broken** (`LINK BROKEN: … (not found|outside the documentation root)`) and replaces the whole `` element — tags and visible text alike — with that literal message, matching `images.go`'s precedent for a missing image (`renderLink` needs a small per-node flag, `linkBrokenText`, since goldmark still invokes a container node's renderer on the matching leaving call regardless of `WalkSkipChildren` on entering, and there is no `` to write in the broken case); existing on disk with no `page_id` yet is unchanged — a **warning**, the normal state of an unpublished tree; a `#fragment` matching no heading on an otherwise-resolving target also **warns**, gated on `linkindex.Index.FileExists` so a missing/escaping target isn't double-reported. `tables.go` (the `` tag, stamped with `data-layout="align-start"` so tables auto-size and left-align — this must stay if column widths are ever emitted, or a `` silently induces a layout; plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour`; `storage_to_md.go`'s `cellTexts` reverses this, reading `data-highlight-colour` back into a `bg:` marker (`cellBGNames`, the reverse of `tables.go`'s swatch map — a hex outside the 21 swatches round-trips as the literal hex, and where two names share a hex the British spelling wins, matching Confluence's own `-colour`), and a column's GFM alignment becomes a `

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

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

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

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