Classify an error code by the failure's origin

[willkg]

Fixes #133.

The bug, in both directions

create's preflight 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". The worst case is the one RejectedCredential exists for: a revoked token answers every v2 route with a 404 naming nothing, GetPageOrNil deliberately does not read that one as "absent", and CodeFor asks RejectedCredential before its status switch precisely so it reports AUTH — which create threw away. The same token already reported AUTH from phase 3, so one credential produced two codes depending on which phase noticed. cmd/fix got the status half right via its own locateCode, which is what made this worth fixing rather than tolerating.

The same defect runs the other way on the attachment paths, and #127 walked past it. client.planAttachments calls fileChecksum(att.Path) — a local os.Open — so a SyncAttachments/PlanAttachments/ForceUploadAttachments failure classified through bare CodeFor reported NETWORK for a file on disk. Four sites did that, including attachment-upload, whose entire input is local files and which already tells IO from VALIDATION upstream in localAttachmentsCode before losing the distinction one call later. create's publishOne comment names this exact condition as one of S7's residuals.

What this does

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. The issue suggested lifting fix's locateCode into internal/jsonout, but a bare type check on *HTTPError only fixes the status half: doJSON builds an HTTPError only once it has a status, so a dial failure, a TLS error, or an undecodable response still took the local fallback — and NETWORK vs VALIDATION is the distinction a consumer actually branches on to decide whether retrying is worth anything.

So internal/client types its own request-path errors instead, and callers ask a predicate rather than marking call sites. The rule: an error a client method returns because the request failed is typed; an error that came from the caller's own data is not. *HTTPError once there is a status, an unexported requestError when there is none, and FromRequest answers for both. jsonout.CodeOr(err, fallback) classifies by CodeFor when FromRequest holds and takes the fallback otherwise — a parameter rather than a hardcoded VALIDATION, so the call site states which local meaning it means.

The alternative was per-call-site markers in create and fix (the convertFailure/badInput idiom already in the tree). Rejected because the obligation would land on every future call site and fail silently when forgotten — a new client call in preflight without the wrapper reports VALIDATION, which is this bug reintroduced. Typing at the source puts the guarantee in one function a test can hold, the same reasoning that keeps the traversal clamp in internal/attachfile rather than in two commands.

Deliberately not a claim that 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 tagging them would misreport an unreadable file as a network problem — the same lie in a new place. The wrapper carries no message of its own, so Error() is the inner text verbatim: no human output moves and no existing string assertion changes. The only observable difference is the code field.

_plans/035_error-code-classification.md has the full decision record, including why the new type is unexported (FromRequest is the whole new surface, and unexported→exported is the reversible direction) and why a malformed 200 stays NETWORK.

Scope

The mirror sites are fixed here rather than 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. 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, all eight codes valid on every result shape. No README change: it lists the eight values and never claims which one a given failure carries. Exit codes do not move — the README scopes exit 2 to credential resolution, not the server rejecting one.

Two things found during the work and left out, both recorded in the plan's Out of scope: a page_id that resolves to nothing is a local error classified NOT_FOUND by update but VALIDATION by create/fix, which is a judgment about what that condition means rather than this rule; and an error-code-names-the-cause guarantee, which would have to land Partial since it rests on 118 code-assignment sites being individually right.

Verification

make check clean. Both #133 tests were confirmed to fail against the old code first (withid.md code = "VALIDATION", want AUTH), and the statusless rows were mutation-tested against the type check they replace.

Two independent code reviews ran on the branch and converged on one real gap, now fixed in the last commit: searchCQLBounded's page-count bail returned an untagged error — the one request-path failure that is not a response — which sat outside the invariant FromRequest documents. Harmless today, since find/search still use bare CodeFor, but it is #133's shape waiting for the first search failure routed through CodeOr. The reviews also caught that neither create's nor fix's test covered a request failure with no status (added) and that a new comment described DownloadAttachment as streaming to the file when it buffers and writes once (corrected).

What stays unpinned, deliberately: three of the four mirror sites. create and update cannot reach fileChecksum's error through the converter — a missing asset is reported IMAGE BROKEN and never becomes an attachment — so provoking it needs a chmod 000 that behaves differently as root, or a client hook existing only for a test. attachfile.Write is pinnable only through the no-download-link path, which would cement an imprecision the code documents as one. The rule itself is pinned by the jsonout table on a real fs.PathError, and attachment-upload exercises the local direction end to end through the command's own planCode.