refactor(service-settings)!: settings error bodies use the declared details slot (#4224) - #4237
Merged
Merged
Conversation
…details` slot (#4224) Four `/api/settings/*` error branches spread ad-hoc context as SIBLINGS of `code` and `message` inside `error` — `namespace`, `key`, `reason`, `fields`. `ApiErrorSchema` declares none of them. The bodies passed every gate anyway: the schema is a plain `z.object`, so unknown keys were STRIPPED rather than rejected, and `envelopeViolations` inspects only the body's top level. They were conformant by stripping, not by declaration — and the same module already used `error.details` correctly one branch over (`SETTINGS_ACTION_FAILED`), so this was one file speaking two dialects. All four move into `details`, values unchanged: SETTINGS_FORBIDDEN error.namespace -> error.details.namespace UNKNOWN_KEY error.namespace/.key -> error.details.{namespace,key} SETTINGS_LOCKED error.namespace/.key/.reason -> error.details.{namespace,key,reason} SETTINGS_VALIDATION error.namespace/.fields -> error.details.{namespace,fields} `SETTINGS_VALIDATION.fields` also changes shape, which is the decision the issue asked for: `fields` is the name ADR-0114 (#3977) closed for `FieldError[]`, so keeping a `Record<key, message>` under it would leave one spelling meaning two shapes. It becomes the declared array — `code` is `required` or `invalid_format` from the closed field-level catalog, `label` and `constraint.pattern` carry what the message interpolates. The constraint kind is knowable at the throw site and nowhere after it, so the exported `SettingsValidationError.fields` changes with it rather than being reconstructed from prose at the route. It stays `details.fields` rather than a top-level `error.fields` because `fields` is declared on `EnhancedApiErrorSchema`, not on the base `ApiErrorSchema` these routes emit — the same reason `validation-failure.ts` puts the dispatcher's array there. `sendError`'s last parameter is tightened from `Record<string, unknown>` to `ApiError`'s own optional fields, and `code` from `string` to the closed ADR-0112 `ErrorCode` union. That is what keeps it fixed: an undeclared sibling is now a compile error at the call site instead of a key that evaporates at the schema boundary. The conformance suite gains the assertion neither existing gate could make — every key of `body.error` must be one `ApiErrorSchema.shape` declares, derived from the schema rather than restated, so it tracks the contract both ways. Both new pins were verified to fail against the pre-#4224 shape. Consumer note: `error.key` had one live reader that the issue's `packages/` + `examples/` grep could not see — objectui's `SettingsView` save handler. Its tolerant dual-read ships in objectui#3079 and should land first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tv2NYMBTrzVVRWiWWuyec5
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 5 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…ields` and where errors carry context (#4224) The docs-drift check flagged this page against the #4224 change. Re-verified: it documents error CODES, not wire shapes, so nothing on it had become false. Two lines added where the change is load-bearing rather than left implicit — that `fields` is now the declared `FieldError[]` with `code` naming the constraint, and that the REST surface carries per-branch context in `error.details` rather than beside `error.code` / `error.message`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tv2NYMBTrzVVRWiWWuyec5
os-zhuang
marked this pull request as ready for review
July 31, 2026 00:40
xuyushun441-sys
pushed a commit
that referenced
this pull request
Jul 31, 2026
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>
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
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 #4224.
The finding, confirmed
Four
/api/settings/*error branches spread ad-hoc context as siblings ofcodeandmessageinsideerror.ApiErrorSchemadeclarescode,message,category?,httpStatus?,details?,requestId?— and none ofnamespace,key,reason,fields.The bodies passed every gate anyway, for two independent reasons that happened to line up:
ApiErrorSchemais a plainz.object, so unknown keys are stripped rather than rejected, andenvelopeViolationsinspects only the body's top level. Conformant by stripping, not by declaration. The same module already usederror.detailscorrectly one branch over (SETTINGS_ACTION_FAILED), so this was one file speaking two dialects rather than a missing capability — as the issue says.A survey of the sibling route modules confirms settings was the outlier:
i18n,storageandshare-linktake no extra parameter at all, andpackage-routes.tsalready names its onedetails.FROM → TO
Values unchanged in every case; only the position moves.
SETTINGS_FORBIDDENerror.namespaceerror.details.namespaceUNKNOWN_KEYerror.namespace,error.keyerror.details.{namespace,key}SETTINGS_LOCKEDerror.namespace,error.key,error.reasonerror.details.{namespace,key,reason}SETTINGS_VALIDATIONerror.namespace,error.fieldserror.details.{namespace,fields}The decision the issue asked for
error.detailsis the home, andSETTINGS_VALIDATION.fieldsjoins the field-level vocabulary properly rather than dodging it.ADR-0114 (#3977) landed the vocabulary D6 deferred:
fieldsnow meansFieldError[]—{ field, code, message, label?, constraint? }withcodefrom a closed 27-member catalog. That makes the middle option the worst one: putting the existingRecord<key, message>under the namefieldsinsidedetailswould leave one spelling meaning two shapes, which is the exact drift these ADRs exist to prevent. So the map becomes the declared array:required— an empty required specifierinvalid_format— a value that misses its declaredpattern, which travels asconstraint.patternIt stays
details.fieldsrather than a top-levelerror.fieldsbecausefieldsis declared onEnhancedApiErrorSchema, not on the baseApiErrorSchemathese routes emit.validation-failure.tsputs the dispatcher's array in the same place for the same reason, and objectui'sextractFieldErrorsalready readsdetails.fields— so settings validation failures become renderable per-field in the console with no per-surface special case.The constraint kind is knowable at the throw site and nowhere after it (the route only ever saw prose), so the exported
SettingsValidationError.fieldschanges with the wire rather than being reconstructed from message text.What keeps it fixed
sendError's last parameter is tightened fromextra?: Record<string, unknown>toPick<ApiError, 'category' | 'httpStatus' | 'details' | 'requestId'>— the loose edge the issue names — andcodefromstringto the closed ADR-0112ErrorCodeunion (all seven codes this module emits are already in the standard catalog or the ledger). An undeclared sibling is now a compile error at the call site instead of a key that quietly evaporates at the schema boundary.The conformance suite gains the assertion neither existing gate could make: every key of
body.errormust be oneApiErrorSchema.shapedeclares. It is derived from the schema rather than restated, so a field added to the contract is allowed the moment it is declared and one removed stops being allowed — a hand-written list's failure mode is that it silently keeps blessing a retired key.On not generalising this into
envelopeViolationsTempting, since that is where the check structurally belongs — but wrong here.
DispatcherErrorResponseSchemais a declared superset ofApiErrorSchemathat legitimately carriesroute/hint/serviceas siblings, andbuildApiErrorhas anextraparameter for exactly that. A blanket "no undeclared key insideerror" rule in the shared checker would fail those conformant bodies. The generalisation needs a per-module notion of which error schema applies; that is a larger change than this issue, so the assertion stays local and honest — settings emits the baseApiErrorand nothing more.Breaking, and the cross-repo half
The issue's "write-only today" note is right for three of the four keys but not the fourth. Its grep covered
packages/andexamples/in this repo;error.keyhas a live reader one repo over — objectui'sSettingsViewsave handler renders the locked key in a toast.So this ships tolerant-read-first (the objectui#2869 pattern ADR-0112 records):
error.details, tolerating both shapes (objectstack#4224) objectui#3079 — console readserror.details?.key ?? error.keyLanding this one first shows
Locked by environment: undefinedto anyone on a console built before it. The changeset carries the full FROM → TO mapping and the one-line consumer fix, per AGENTS.md.Verification
pnpm --filter @objectstack/service-settings test— 13 files, 194 tests, greenpnpm --filter @objectstack/spec test— 276 files, 7174 tests, greenpnpm check:route-envelope— 7 conformant, 0 ratchetedpnpm check:error-code-casing— clean across 2492 filesApiErrorSchemadoes not declare, conformant only becausez.objectstrips them #4224 shape — reintroducing the sibling spread on one branch turns the suite red witherror carries keys ApiErrorSchema does not declare: ['namespace','key'], so these are pins rather than assertions that cannot failpackages/specexports added or removed, sogen:api-surfaceis untouched@objectstack/verifyhas one pre-existing failure in this environment (Failed to resolve entry for package "@objectstack/runtime", an unbuilt sibling) — reproduced with these changes stashed, unrelated🤖 Generated with Claude Code
Generated by Claude Code