fix(service-storage,service-i18n): emit the declared error envelope, not a bare { error } (#3675) - #3687
Merged
Conversation
…not a bare `{ error }` (#3675)
#3636 aligned the SUCCESS bodies of the autonomously-mounted service routes,
because those were the ones breaking `ObjectStackClient.unwrapResponse`. The
error bodies were left alone and stayed a bare `{ error: '<message>' }` — with
the code, where one existed at all, as a SIBLING of `error` rather than a field
of it — against a contract (`BaseResponseSchema` + `ApiErrorSchema`) declaring
`{ success: false, error: { code, message } }`. So the same SDK method returned
two different error shapes depending on which provider mounted the route.
All 32 sites (27 in `storage-routes.ts`, 5 in `i18n-service-plugin.ts`) now go
through one `sendError` helper per module: the nested-`error` shape the sibling
services already use (`settings-routes.ts`, `share-link-routes.ts`), plus the
`success` flag those two still omit and the contract requires.
The codes moved, and that is the breaking part. `AUTH_REQUIRED`,
`ATTACHMENT_DOWNLOAD_DENIED` and `FILE_DOWNLOAD_DENIED` were at `body.code` and
are now at `body.error.code`. The SDK is unaffected — it already reads
`errorBody?.code || errorBody?.error?.code`, one of four shapes its error path
sniffs for, which is the consumer-side shim Prime Directive #12 says to cure at
the producer. The console's attachment panel was NOT: it read the top level
only, so every gated download would have degraded to "Download failed (403)".
Fixed in objectui to read both dialects, since a console build ships
independently of the server it talks to.
Guarded both ways. A new `error-envelope.conformance.test.ts` in each service
drives every distinct error branch through the real registrar and parses the
body against the real `BaseResponseSchema` imported from `packages/spec` — not
a local restatement that could drift from the schema it claims to check — and
scans the module source so a new route cannot quietly reintroduce the bare
shape. Without the second half the suite would only ever cover the branches
that existed the day it was written. Both halves are mutation-checked.
The route ledgers (#3563 → #3656) could not have caught this: they audit which
routes exist and whether the SDK can address them, not what comes back.
Recorded as §12 of the audit doc, with the four error dialects found live.
Measured and deliberately left alone: the dispatcher does not conform either —
it puts the HTTP status in `error.code` where the contract declares a semantic
string, and parks the real code in `details` to work around its own occupied
field. Now pinned to exactly one deviating field by a test rather than prose.
Also unchanged: service-storage's success bodies are still three shapes of
their own, none carrying `success: true` — non-additive, so it gets its own
issue instead of a quiet ride along with this one.
Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 24 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…lared envelope (#3675) `wire-format.mdx` §7 describes the flat `{ error, code }` envelope, which is what the kernel REST server emits for `/api/v1/data/*`. With the storage and i18n routes moved to the declared `BaseResponse` shape, a reader following §7 would look for `body.code` on those routes and find nothing. Says which envelope belongs to which surface, and that SDK callers need not branch because `ObjectStackClient` normalizes both. Co-Authored-By: Claude <noreply@anthropic.com>
os-zhuang
marked this pull request as ready for review
July 27, 2026 14:43
This was referenced Jul 28, 2026
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…ing; the HTTP status moves to `httpStatus` (#3842) (#3971) `HttpDispatcher.error()` took the HTTP status as its `code` argument and wrote it into the field `ApiErrorSchema` reserves for a semantic string, so `error.code` came back as 400/403/503 — a number duplicating the response status and occupying the one slot a caller branches on. The real code then had to go elsewhere, and did, three elsewheres: `details.code`, `details.type` and `error.type`. `code` is now the semantic string, `httpStatus` carries the number, and `details` is genuine context only. Every code already on the wire moves verbatim — PERMISSION_DENIED, ROUTE_NOT_FOUND, PASSWORD_EXPIRED, PROJECT_MEMBERSHIP_REQUIRED, VALIDATION_FAILED, unauthenticated. This moves a field; #3841 still owns renaming any of them. A branch with no code of its own gets one derived from the status via one declared map in the spec (`HttpStatusErrorCodeMap` / `standardErrorCodeForHttpStatus`), drawing only on catalogued `StandardErrorCode` members. Spec: `ApiErrorSchema` gains optional `httpStatus`; `StandardErrorCode` gains `method_not_allowed` / `precondition_required` (both additive). BREAKING — `DispatcherErrorCode` members go from '404'|'405'|'501'|'503' to the four semantic spellings the removed `error.type` declared, and `DispatcherErrorResponseSchema.error.code` becomes a string; that schema had declared the opposite of `ApiErrorSchema` for the same field, which is what let the deviation stand. Also aligned, being the same wire surface: `dispatcher-plugin`'s `errorResponseBase` (which used to discard a thrown error's `.code` outright, having nowhere to put it) and its inline 404, plus the MCP 405. All bodies now come from one builder, guarded both ways by `error-envelope.conformance.test.ts` — every branch driven and parsed against the schema imported from the spec, plus a source scan so a new branch cannot reintroduce a numeric `code`. Deletes the #3687 pin, which asked to be deleted rather than updated once the dispatcher was fixed.
This was referenced Jul 30, 2026
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
2 participants
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 #3675. Paired with objectstack-ai/objectui#2869 — that one is load-bearing, see “What nearly broke”.
The drift
#3636 aligned the success bodies of the autonomously-mounted service routes, because those were the ones breaking
ObjectStackClient.unwrapResponse. The error bodies stayed a bare{ error: '<message>' }— with the code, where one existed at all, as a sibling oferrorrather than a field of it — against a contract (BaseResponseSchema+ApiErrorSchema) declaring{ success: false, error: { code, message } }.So the same SDK method returned two different error shapes depending on which provider mounted the route: a caller reading
body.error.messagegot the real message from the dispatcher andundefinedfrom these services.All 32 sites (27 in
storage-routes.ts, 5 ini18n-service-plugin.ts) now go through onesendErrorhelper per module — the nested-errorshape the sibling services already use (settings-routes.ts,share-link-routes.ts), plus thesuccessflag those two still omit and the contract requires.Which target, and why not the dispatcher
The issue proposed aligning to the dispatcher. Checking first turned up four live error dialects, and the declared contract matches none of them:
contract.zod.ts(declared){ success, error: { code: string, message, … } }http-dispatcher.ts{ success: false, error: { message, code: <HTTP number>, details } }rest-server.ts{ error: <string>, code: <SEMANTIC_STRING> }settings-routes.ts,share-link-routes.ts{ error: { code, message } }— nosuccess{ error: <string> }, sometimes+ codeat the top levelSo I aligned to the contract, not the dispatcher. The dispatcher puts the HTTP status in
error.codewhere a semantic string is declared — and then parks the real code indetails(this.error(msg, 403, { code: 'PERMISSION_DENIED' })) to work around its own occupied field, which is the tell that the number is in the wrong place. Both halves of the target shape have in-repo precedent, so nothing here is invented.That deviation is now pinned to exactly one field by a test in
http-dispatcher.test.tsrather than described in prose.toEqual(['code'])is deliberately exact: if a second field starts deviating it fails, and if the dispatcher is ever fixed it also fails and the pin should be deleted. Not fixed here —error.codeis read by the SDK, the console and the dogfood suite.Note the
rest-server.tsrow is documented, not accidental:wire-format.mdx§7 anderror-catalog.mdxboth describe the flat envelope as what/api/v1/data/*emits. This PR does not touch it.What nearly broke
AUTH_REQUIRED,ATTACHMENT_DOWNLOAD_DENIEDandFILE_DOWNLOAD_DENIEDmoved frombody.codetobody.error.code. #3675 flagged “check@object-uierror-surfacing paths first” as a precaution; it turned out to be a real hit.errorBody?.code || errorBody?.error?.code, one of four shapes its error path sniffs for. That shim is the consumer-side symptom Prime Directive Add comprehensive test suite for Zod schema validation #12 says to cure at the producer.RecordAttachmentsPanelread(await res.json())?.codestructurally, so every gated download would have degraded from “You don't have access to download this attachment.” to the generic “Download failed (403)”. Fixed in objectui#2869, reading both dialects — a console build ships independently of the server it talks to, so tolerance is correct here rather than a synchronized cutover, and it makes the merge order of the two PRs irrelevant.cloudrepo — checked, no consumers. It mounts these services but never reads their error bodies.Also re-pointed: 5 assertions in
storage-routes.test.tsand 3 in the attachments dogfood matrix. The other 4 dogfood assertions in that file (FILES_DISABLED,ATTACHMENT_PARENT_ACCESS,PERMISSION_DENIED) are produced byrest-server.ts, which this PR does not touch, so they deliberately still read the top-levelcode.Guards
A new
error-envelope.conformance.test.tsin each service, guarding both directions:BaseResponseSchemaimported frompackages/spec, not a local restatement that could drift from the schema it claims to check.Both halves mutation-checked: reintroducing one bare error site fails the scan; dropping the console's nested read fails the objectui cases.
One case is there for a reason worth naming:
ApiErrorSchemarequiresmessage, so a thrown non-Errorwould otherwise emit an invalid body.throw 'a bare string'is covered.Scope held
{ data }, bare{ url },{ ok, key }, none carryingsuccess: true). Unlike Route audit tranche 3: autonomously-mounted service routes (service-storage / service-i18n) — #3587 follow-up #3636's success fix this one is not additive — normalizing bare{ url }moves data and breaksgetDownloadUrl's reader — so it gets its own issue instead of a quiet ride along with this one.error-catalog.mdxdocumentsStandardErrorCodeas lowercase snake_case (validation_error); every SCREAMING_SNAKE code here is either pre-existing (AUTH_REQUIRED,ATTACHMENT_DOWNLOAD_DENIED,FILE_DOWNLOAD_DENIED— renaming them would break the console and the dogfood suite) or matches its immediate neighbours (settings-routes.tsemitsSETTINGS_FORBIDDEN,INTERNAL). This PR moves codes into the declared envelope; reconciling the two vocabularies is a separate spec decision.rest-server.tsand the two sibling services keep their dialects.Not an oversight in the ledgers
The route ledgers (#3563 → #3656) could never have caught this: they audit which routes exist and whether the SDK can address them, not what comes back. That boundary is now recorded as §12 of the audit doc — “the route exists” and “the route answers in the declared shape” are two questions, and the second needs the contract imported into the assertion.
wire-format.mdx§7 gains a short subsection: its flat envelope is the data-route form, and the service-mounted routes now answer in the declared one.Tests
@objectstack/service-storage@objectstack/service-i18n@objectstack/runtime@objectstack/clientDogfood assertions are re-pointed but not run locally (they need a live stack); they will run in CI.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K35y3ovfWtCkBHYFCqUfAt