Skip to content

refactor(rest): extract the ADR-0112 error/fault-classification prologue out of rest-server.ts - #8887

Merged
qq9340100 merged 2 commits into
mainfrom
claude/issue-8850-rest-error-prologue-extract
Aug 15, 2026
Merged

refactor(rest): extract the ADR-0112 error/fault-classification prologue out of rest-server.ts#8887
qq9340100 merged 2 commits into
mainfrom
claude/issue-8850-rest-error-prologue-extract

Conversation

@qq9340100

@qq9340100qq9340100 commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Fixes#8850
Fixes#8884

The module-level code ahead of RestServer is ADR-0112's concern — how a thrown thing becomes an HTTP answer — not the REST server's. It reads no class state and sat in that file for historical rather than structural reasons. Moved to packages/rest/src/error-response.ts.

⛔ Not the ADR-0076 D11 decomposition, which the maintainer ruling of 2026-08-15 on #5949 closed (option B). No registerXxxEndpoints method moved, no line inside the RestServer class body was touched, and no line count is offered as justification.

Both dispatch premises verified by symbol, not by line number

"Module-level functions only, ending before the first class declaration." Holds. The first class is export class RestServer — still at line 1981 on cc5c07b80, but re-derived by symbol rather than inherited. Everything above it is module-level.

"Zero coupling to the RestServer class." Holds, and it was checked rather than assumed. No moved function reads class state, and the coupling runs strictly the other way: the class calls seven of them (sendError 5, sendFieldVisibilityFault 5, handleRouteError 36, logUnexpectedRouteError 7, isExpectedRouteError 2, applyDroppedFieldsHeader 2, mapDataError 9), and those seven are the module's entire export surface beyond mapDataError. The other eleven symbols had zero call sites in the class and stay module-private, exactly as they were.

One correction to the card, offered because it is measurable and does not change the verdict: the extracted region is 404 non-comment code lines, not the ~732 the card estimated. The region was fixed by the card's own symbol list, not by that number.

What moved

The card's eighteen symbols plus their module constants (CLIENT_MESSAGE_MAX, NATIVE_ERROR_NAME_RE, RELATION_DOES_NOT_EXIST), as one contiguous block. The console shim both files need went to packages/rest/src/log.ts rather than being copied — the alternative was two definitions of the same two lines.

droppedFieldsHeaderValue / applyDroppedFieldsHeader are the one pair in that list that is not error classification: they decorate a successful write (#3431). They travelled with the block because they are the rest of "what this boundary puts on the wire", and splitting them out would have meant a third module for two functions. Flagged here so the choice is reviewed rather than discovered.

Evidence that this is a move

  • The moved region is byte-identical. Programmatically compared against the pre-move file — 1289 lines, 0 differences. The only edits are six export prefixes and one comment relocation, below.
  • rest-server.ts's diff is 16 insertions / 1296 deletions across 4 hunks — three import-block edits and one re-export line. Nothing else in the file was reformatted or re-ordered (Characterize registerMetadataEndpointsInner (2,328 lines, 20% of rest-server.ts) — read and propose, do not split blind #8851 is reading this file concurrently).
  • The export surface is unchanged. AST-compared: rest-server.ts exports the same 13 names on origin/main and on this branch, none added, none removed — mapDataError and the zodIssuesToFields re-export included. packages/rest/src/index.ts is byte-identical. The three test files that import { mapDataError } from './rest-server.js' and the one that imports zodIssuesToFields are untouched.

The one deliberate non-identical edit:mapDataError's docblock had drifted away from its function — it was stranded above an unrelated import roughly 300 lines up, presumably left behind when zodIssuesToFields moved to spec in #8124, and mapDataError itself carried no docblock at all. It is reattached to the function it describes. Comment only; recorded here so a reviewer does not have to find it. The other pre-existing docblock drift in the region (missingRelationIsObject's block sitting above declaredHttpStatus) was deliberately left as-is — both moved to the same file in the same relative order, so nothing about it got worse, and repairing it would have added a second unreviewable comment edit.

Two gate declarations — second commit

Both are consequences of the move, and neither changes what any gate enforces. No baseline touched, no ratchet raised, no threshold moved.

check-route-envelope.mjs — closing #8884

error-response.ts emits REST bodies (the sendError / handleRouteError doors) but sits outside the *-routes.ts discovery convention, so it would have been audited by nothing — the exact cost the checker's own header records paying once already for rest-server.ts. Named in OFF_CONVENTION_MODULES plus a { responses: 2, ok: 0, err: 0 } entry, measured at extraction rather than chosen.

responses: 2 is the first non-zero write-site count in that table. It is pinned rather than waived because this module is the opposite of rest-server.ts on the axis that made that file dialectOnly: its write sites are the two doors, so the count is structurally stable and moves only if a third door appears — which is precisely what deserves review. It carries a ratchet for the same reason rest-server.ts does: responses: 2 is outstanding drift by the table's own definition, with #7035 option 1 as its end state.

⚠️ Read the 0 / 0 on both dialects for what it is: both sites write resolved.body, an identifier, and the dialect counters only see object literals — so the flat shapes mapDataError builds are not counted here, and were not counted in rest-server.ts before the move either. What the zeros pin is that no new literal body appears at this boundary unnoticed.

rest-server.ts's own ratchet is untouched: stringError 44 / siblingCode 69, before and after.

check-durability-degradation-log-level.mjs — a visibility regression, diagnosed before it was fixed

CI went red on this gate at the first commit: two saveMetaItem route catches "degrade quietly". The diagnosis, in order:

  1. Is main green? Yes — measured on a clean origin/main worktree at cc5c07b80: exit 0, 25 seams. So this PR caused it.

  2. How? Not assumed — read off the gate's own --list output on main:

    packages/rest/src/rest-server.ts:6229 guards saveMetaItem()@6215 -> loud (error@120 via handleRouteError())
    packages/rest/src/rest-server.ts:7000 guards saveMetaItem()@6990 -> loud (error@120 via handleRouteError())
    

    error@120 was logError. The gate reached it by following handleRouteError as a same-file helper (collectLoggedLevels resolves callees through a per-file functionBodies map). Moving that function to its own module made the inference unavailable. The gate's visibility regressed; the behaviour did not move a line.

The fix is the declaration the gate's own failure message prescribes, not a log: handleRouteError is declared in FAILURE_PROPAGATION_CALLEES as via: 'effect'. It resolves the error once and then always writes res.status(resolved.status).json(resolved.body) — no branch returns without answering the caller — and it is the only definition of that name in the repo, so the callee-scoped declaration is unambiguous.

Worth stating plainly: this classification is stricter than the one it replaces.handleRouteError logs only when isExpectedRouteError() is false, so "unconditionally loud" was always slightly generous — a durability failure mapping to an expected status printed nothing. What is unconditional is the answer to the caller, and that is what the entry claims. catchDeliversFailure() still proves every path out of the catch reaches it. Seam count is 25 on main and 25 here — nothing left the gate's view.

Verification — all at final head 294788317

Union re-derived after the final commit with node scripts/pm/dispatch-gates.mjs over all five changed paths. It now names check:durability-log-level (it matches on the script path, which the first derivation could not see) and matches check:route-envelope through error-response.ts itself.

checkresult
pnpm --filter '@objectstack/rest^...' build (closure)exit 0
pnpm --filter '@objectstack/rest' typecheckexit 0
pnpm --filter '@objectstack/rest' test118 files, 1948 tests, all passed
pnpm --filter '@objectstack/rest' buildexit 0, DTS included
pnpm check:route-envelopeexit 0 — 10 modules audited; rest-server.tsunmoved at 44 / 69
pnpm check:durability-log-levelexit 0 — 25 seams, same count as main
pnpm check:authz-resolverexit 0
pnpm check:cross-package-test-inputsexit 0
pnpm check:nul-bytesexit 0, 5894 files

Reverse verification — for a move, the meaningful question is whether the suites exercise the new module rather than a stale copy. Ablated UNCLASSIFIED_FAULT's status 500 → 599 in error-response.ts and predicted RED. Observed RED across five suites (rest-5xx-status-passthrough, rest-expected-error-logging, rest-hook-refusal-status-passthrough, rest-hook-script-fault-envelope, rest-unclassified-fault-status). Restored from the commit and verified byte-identity by git hash-object against HEAD, with a clean git status --porcelain.

Suites covering the moved code (19 in packages/rest): rest-unclassified-fault-status, rest-unknown-object-heuristic, rest-5xx-status-passthrough, rest-5xx-message-sanitization, rest-4xx-message-truncation, rest-hook-refusal-status-passthrough, rest-hook-script-fault-envelope, rest-expected-error-logging, rest-meta-outage-vs-miss, rest-unique-violation-dialects, external-write-forbidden-envelope, package-door-5xx-message-sanitization, rest-dropped-fields, rest-update-path-id-not-a-dropped-field, zod-union-fields, rest.test.ts, and three more.

Honest gap, reported rather than papered over:sendFieldVisibilityFault — the ADR-0106 D6 tier-3 refusal — has no test anywhere in the repo. Recorded in #8885, which is out of scope here and stays open.

Finding filed and left open

#8885 remains open and is not addressed by this PR: FIELD_VISIBILITY_UNRESOLVED and RECORD_NOT_ACCESSIBLE are put on the wire but appear in neither StandardErrorCode nor ERROR_CODE_LEDGER, and the first has zero coverage while being documented to operators in production-readiness.mdx. Blocked-by #8846, and a wire-code question rather than a move.

No changeset

skip-changeset applied: no user-facing published change. No export added, removed or narrowed; no status, code, body, header or log line differs for any caller. The two gate edits are build-time declarations that ship to nobody.


Generated by Claude Code

…gue out of rest-server.ts
The module-level code ahead of `RestServer` is ADR-0112's concern — how a
thrown thing becomes an HTTP answer — not the REST server's. It reads no class
state and lived in that file for historical rather than structural reasons.
Moved verbatim to `packages/rest/src/error-response.ts`: mapDataError,
resolveErrorResponse, DATA_STORE_FAULT, UNCLASSIFIED_FAULT, isScriptFaultMessage,
declaredHttpStatus, truncateClientMessage, sendError, sendFieldVisibilityFault,
logWithheldServerFault, isExpectedDataStatus, isExpectedQueryRejection,
isExpectedRouteError, logUnexpectedRouteError, handleRouteError,
droppedFieldsHeaderValue, applyDroppedFieldsHeader, missingRelationIsObject,
plus their module constants. The console shim both files need moved to
`packages/rest/src/log.ts` rather than being copied.
No behaviour change: every moved line is byte-identical, the only edits being
six `export` prefixes and mapDataError's docblock reattached to the function it
describes (it had drifted above an unrelated import). `mapDataError` and
`zodIssuesToFields` are still exported from `./rest-server.js` unchanged.
Not the ADR-0076 D11 decomposition, which the 2026-08-15 ruling on #5949 closed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
@vercel

vercelBot commented Aug 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 15, 2026 2:36pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest.

9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/connect-mcp.mdx(via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx(via @objectstack/rest)
  • content/docs/api/index.mdx(via @objectstack/rest)
  • content/docs/permissions/authentication.mdx(via @objectstack/rest)
  • content/docs/permissions/system-context.mdx(via packages/rest)
  • content/docs/plugins/index.mdx(via @objectstack/rest)
  • content/docs/plugins/packages.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx(via packages/rest)

3 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/rest)
  • content/docs/releases/v12.mdx(via @objectstack/rest)
  • content/docs/releases/v17.mdx(via @objectstack/rest)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

…oss the prologue extraction
Two declarations, both made necessary by the file move in the previous commit and
neither changing what any gate ENFORCES.
check-route-envelope.mjs: `error-response.ts` emits REST bodies (the sendError /
handleRouteError doors) but sits outside the `*-routes.ts` discovery convention,
so it would have been audited by nothing. Named in OFF_CONVENTION_MODULES with a
`{ responses: 2, ok: 0, err: 0 }` entry measured at extraction. rest-server.ts's
own ratchet is untouched and still reads stringError 44 / siblingCode 69.
check-durability-degradation-log-level.mjs: the two saveMetaItem route catches
qualified on main as `loud (error@120 via handleRouteError())` — the gate followed
handleRouteError as a SAME-FILE helper down to logError. Moving that function to
its own module made the inference unavailable (helper resolution is file-scoped by
construction), so the gate lost visibility while the behaviour did not move a line.
Declared handleRouteError in FAILURE_PROPAGATION_CALLEES as `via: 'effect'`: it
answers the caller unconditionally, which is a STRICTER claim than the loudness it
replaces, since it logs only for unexpected errors. Seam count is 25 before and
after — nothing left the gate's view.
No baseline touched, no ratchet raised, no threshold moved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NaS1PAHJcPfAA2acnV53Tn
@qq9340100
qq9340100 marked this pull request as ready for review August 15, 2026 15:05
@qq9340100
qq9340100 added this pull request to the merge queueAug 15, 2026
Merged via the queue into main with commit 8664a2cAug 15, 2026
29 checks passed
@qq9340100
qq9340100 deleted the claude/issue-8850-rest-error-prologue-extract branch August 15, 2026 15:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/xlskip-changesetPR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants

@qq9340100@claude