refactor(types,rest,services,plugin-sharing): one shared writer for the response envelope, and error.code is enforced at compile time (#3973) - #4229
Merged
Conversation
…he response envelope, and error.code is enforced at compile time (#3973) `BaseResponseSchema` declares one envelope for every REST body the platform emits. It declared it once; the code that *wrote* it was copied per route module. After #3843 and #3983 converted the last drifting one, seven modules each carried their own two-line `sendOk` / `sendError` pair — the envelope's shape in fourteen places rather than one. `check:route-envelope` proved those copies agreed, which is why this is a cleanup rather than a bug fix. But a guard proves agreement; it does not create it. An eighth module starts by copying the pair again — not hypothetically: `share-link-routes.ts` was found already drifting by the repo-wide scan, and its drift had broken `client.shareLinks.create()` / `.list()` (#3983). Placement was #3973's open question, not design. `packages/spec` is schemas-only (Prime Directive #2), and the callers span `rest`, four `services/*` and one `plugins/*`. `@objectstack/types` depends on nothing but `@objectstack/spec`, so every caller can reach it, and it is already where the repo puts a helper the HTTP boundaries share — `looksLikeInternalErrorLeak` (#3867) sits one file over and made the same argument first. The builders take a structural `{ status(n), json(body) }`, so the package imports no HTTP contract at all. `error.code` is now checked by the compiler. All seven copies typed it `string`, so an invented code was caught only at runtime, by a conformance suite parsing a driven body — i.e. only on routes some test happened to drive. The shared `sendError` types it as ADR-0112's closed `ErrorCode` union, so an unregistered code fails to compile at every call site at once. This cost no call-site churn: every code the seven modules emit was already registered. Nothing changes on the wire. The seven pairs were identical modulo the optional `status` and `extra` parameters this one unions, and each module's driven conformance suite still parses its real bodies against the real spec schemas. One internal call site was rewritten: `package-routes` passed `details` positionally and now passes `{ details }`, producing the same `error.details`. The guard got stronger. A module routing everything through the shared pair builds none itself, so the seven now declare `0 / 0 / 0` where they declared `2 / 1 / 1`, and the pair is pinned separately at `2 / 1 / 1` so the invariant is total for the surface rather than per-module. What the count asserts is no longer "your two builders are the enveloped ones" but "you have no builders". Two out-of-scope findings filed rather than folded in (Prime Directive #10): #4224 (settings passes four keys `ApiErrorSchema` does not declare — the reason `extra` stays `Record<string, unknown>`) and #4225 (three admin routes answer 503 naming the wrong service — carried out of #3973 so closing it does not bury the note). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lope-builders-802cd8
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
size/l
documentation
Contributor
📓 Docs Drift CheckThis PR changes 7 package(s): 17 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…lope-builders-802cd8
Two of main's commits landed in files this branch rewrites, and both are the follow-ups this branch's own issues asked for: #4237 (#4224) — settings error bodies moved onto the declared `details` slot, and its LOCAL sendError tightened `extra` to ApiError's own optional fields. #4234 (#4225) — the datasource-admin 503 became `resolve(res, service, method)`, so the name that performs the lookup is the name that writes the message. Resolution keeps main's semantics and this branch's consolidation: main's call sites and its `resolve` helper survive verbatim; the local sendOk/sendError copies both commits still carried are deleted in favour of the shared pair. `extra` is tightened at the SHARED builder rather than in the one module that was fixed — `Pick<ApiError, 'category'|'httpStatus'|'details'|'requestId'>`. #4224's fix removed the only reason it was `Record<string, unknown>`, and doing it here makes an undeclared sibling of `code` a compile error in all seven modules at once instead of only the module someone happened to fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lope-builders-802cd8
os-zhuang
pushed a commit
that referenced
this pull request
Jul 31, 2026
Resolves the #3973/#4229 overlap: the per-module sendOk/sendError pairs this branch had annotated were consolidated into @objectstack/types on main, so the local definitions go and the #4249 notes move to the module docs. With the shared sendError typing `code` as the closed ErrorCode union, SERVICE_ERROR_CODE is now Record<ServiceName, ErrorCode> — the mis-attribution class this branch fixes is a compile error going forward. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Di9hefxJMFD3rxiP2G2Eug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3973.
BaseResponseSchemadeclares one envelope for every REST body the platform emits. It declared it once; the code that wrote it was copied per route module. After #3843 and #3983 converted the last drifting one, seven modules each carried their own two-linesendOk/sendErrorpair — the envelope's shape in fourteen places rather than one.#3973's option 1 (consolidating i18n's four inline success builders) had already shipped. This is option 2, whose stated blocker was placement, not design.
Placement, answered by precedent
packages/specpackages/core@objectstack/types@objectstack/spec, so every caller can reach it@objectstack/typesis already where the repo puts a helper the HTTP boundaries share:error-leak.ts(#3867) sits one file over and made the identical argument first — "do not ship driver internals to clients" is a property of the HTTP boundary, not of one router. Writing the declared envelope is the same kind of property.The builders take a structural
{ status(n), json(body) }, so the package imports no HTTP contract at all —IHttpResponsesatisfies it, and so does theany-typedresthe three older modules carry.error.codeis now checked by the compilerAll seven copies typed the parameter
code: string. ADR-0112 (#3841) closed the vocabulary —ErrorCodeisStandardErrorCode ∪ ERROR_CODE_LEDGER— but an invented code was still caught only at runtime, by a conformance suite parsing a driven body, i.e. only on routes some test happened to drive.Verified with a probe rather than inferred from a silent typecheck:
TS2345on an unregistered code, andTS2578on a valid code under@ts-expect-error(proving the directive-detector is active, so the test's suppression is real). This cost zero call-site churn — every code the seven modules emit was already registered.The guard got stronger, not just updated
scripts/check-route-envelope.mjscounts response write sites per module. A module routing everything through the shared pair builds none itself:2 / 1 / 10 / 0 / 02 / 1 / 1, pinnedSo the invariant is now total for the surface rather than per-module, and what the count asserts changed from "your two builders are the enveloped ones" to "you have no builders". A new route that hand-rolls a body still moves it off zero and fails. A
SHARED_BUILDERcheck makes deleting or relocating the one writer a loud failure instead of silently leaving nothing pinned. Two self-test cases were added for the consolidated shape.Nothing changes on the wire
The seven pairs were identical modulo the optional
statusandextraparameters this one unions. Every module's driven conformance suite still parses its real bodies against the real spec schemas. One internal call site was rewritten:package-routespasseddetailspositionally and now passes{ details }, producing the sameerror.detailsit always did.Per-module rationale (what each emitted pre-#3843, why its codes were minted, why
/external/validatekeeps its computedok) was preserved and lifted into each module header, not deleted with the builders.Verification
pnpm test— 132/132 turbo tasks greenpnpm check:route-envelope+ self-test,pnpm build,pnpm lint,pnpm check:error-code-casingorigin/mainmerged and everything re-run after (per AGENTS.md multi-agent discipline [WIP] Create a new release version #9/chore: version packages #10)Out-of-scope findings filed rather than folded in (Prime Directive #10)
ApiErrorSchemadoes not declare, conformant only becausez.objectstrips them #4224 —settings-routespassesnamespace/key/reason/fieldsas siblings ofcode, whichApiErrorSchemadoes not declare; they are conformant only becausez.objectstrips them. This is precisely whyextrastaysRecord<string, unknown>rather than the declared fields — tightening it is a wire change, deliberately out of scope for a consolidation that changes no bytes.admin-routeshandlers resolveexternalService()but answer 503 saying the datasource-admin service is unavailable. This was the "adjacent, one line" note at the bottom of Six route modules now each carry their own copy of the envelope builders, and i18n's four are still inline #3973; filed separately so closing that issue does not bury it.🤖 Generated with Claude Code