fix(rest,service-settings,service-datasource)!: four more route modules emit the declared envelope, and the guard is shared (#3843) - #3972
Merged
Conversation
…es emit the declared envelope, and the guard is shared (#3843) #3675 and #3689 moved service-storage and service-i18n onto BaseResponseSchema. Each scoped itself to one service, and neither asked whether the same drift existed elsewhere. It did — in four more modules, two of them carrying the *older* shape #3675 had already declared wrong: settings-routes.ts nested error, no `success` on any of 5 bodies admin-routes.ts { error: '<string>' }, `message` a SIBLING external-datasource-routes.ts { error: '<string>' } + a private `ok` package-routes.ts 3 of 16 bodies had the flag; 2 failures had no `error` at all All four now build every body through a sendOk / sendError pair. Success payloads keep their keys and move under `data`; error bodies become { success: false, error: { code, message } }, so `body.error.message` finally reads the service's message instead of `undefined` — the asymmetry #3675 opened on. Codes are CARRIED OVER, not renamed: admin-routes and external-datasource-routes keep their lowercase snake codes so #3841 can pick one vocabulary for all ~240 at once. Only package-routes needed minted codes, because its `error` strings were human messages with no code to carry. POST /external/validate keeps its `ok`: unlike the `{ ok: true, key }` #3689 retired, that one is a computed verdict over the federated objects, and the request can succeed while the verdict is false. The guard is shared now, not copied. New private @objectstack/route-envelope-conformance exports one pure checkRouteEnvelope(source) => finding[]. Its load-bearing assertion is structural rather than per-route — it COUNTS the `.json(` call sites, which stays fixed at the number of builders no matter how many routes exist, so a future route that hand-rolls a body fails the guard. That is coverage a driven test cannot give. The scan existed three times as an open-coded regex block; all three now delegate to it, as do the four new per-module suites. The shared version also fixes a real bug in those copies: they stripped comments with String.replace, which also ate `//` inside string literals and truncated the rest of that line, `.json(` calls included. A guard that under-counts call sites passes while drift ships. The shared scanner tokenizes and pins that case in its own suite. One ratchet, stated rather than hidden: i18n-service-plugin.ts is declared at jsonCallSites: 5, successBuilders: 4. Its error half is consolidated, but its four read routes build correct envelopes inline. Those numbers pin today's structure (a new inline body fails) and drop to 2/1 on consolidation. Consumers were taught both shapes first so the repos are not coupled by merge order — objectui's packages readers were already tolerant; its datasource page and the generic `type: 'api'` action runner now unwrap the envelope and read error.message (the latter previously toasted "[object Object]"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 3 package(s): 107 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
github-actions
Bot
added
documentation
…eck script (#3843 review) Review feedback: a whole workspace package for one ~150-line pure function is over-engineering, and its name broke the repo's one naming rule. Both points stand. `@objectstack/route-envelope-conformance` was also the only package in 40+ whose directory name did not match its package name (the sole prior exception, objectstack-vscode, is a VS Code extension requirement). Replaced by `scripts/check-route-envelope.mjs` + `pnpm check:route-envelope`, wired into lint.yml beside the nine sibling `check:*` guards — which is what this repo's guard convention actually is. framework has zero custom ESLint plugins (objectui has the eslint-rules/ precedent, framework does not) and no root vitest for a rule's own tests, so the check-script form fits without inventing new infrastructure. Net: one workspace package, one build node, five devDependencies and one tsconfig removed; one script added. Going repo-wide is not just tidier — it is a stronger guard, and it proved that immediately by finding two modules #3843's hand survey missed: * plugin-sharing/share-link-routes.ts — the FIFTH drifting module. No body carries `success`, and one answers `{ ok: true }`, the private second word #3689 retired from storage. Filed #3983, pinned by the guard's ratchet. (#3675's own changeset had cited this file as an example of the good shape; that was true of its `error` object and nothing else.) * metadata/routes/hmr-routes.ts — declared EXEMPT with a reason (dev-only SSE endpoint, not on the SDK surface), not skipped. A per-package scan structurally cannot notice a module nobody thought to convert. Three states are declared deliberately — conformant / ratcheted / exempt — the honest classification ADR-0049 asks for; a module the scan finds but the table does not declare is an ERROR, never a default, since applying the 2/1/1 default to an unknown module would let a new one pass by coincidence. The scan now parses the TypeScript AST instead of matching regexes, which fixes two real bugs the three copied blocks had: 1. comments were stripped with String.replace, whose line-comment pattern also ate `//` inside string literals and truncated the rest of that line — response writes included. A guard that under-counts passes while drift ships. 2. `.json(` does not mean "write a response". hmr-routes.ts calls `c.req.json()` twice to READ a request body; a textual count reports that as two unenveloped responses. Comments and literals are not AST tokens and request-vs-response is a property of the callee, so both disappear and the 110-line tokenizer is gone. The script carries a --self-test pinning each case: the nine sibling guards have none, but both of these bugs survived a review of the regex version. The driven-body half stays in each module's suite, where it belongs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
…-route-modules-v2zoky
…nd refresh the console pin (#3843) Merging main brought in ADR-0112 batch 1 (#3841 (#3988)) and #3842, both of which landed while this PR was in review. Two of this PR's stated decisions were premised on #3841 being UNDECIDED, so they no longer hold. `ApiErrorSchema.code` is now the closed `ErrorCode` union, not `z.string()`, so the nine lowercase codes this PR deliberately carried over failed schema parse — which failed its own conformance suites (6 in service-datasource, 1 in service-settings). Re-spelled per ADR-0112, with generic conditions going to the STANDARD catalog rather than becoming registered synonyms of it, which is what the ledger asks for: datasource_admin_unavailable → SERVICE_UNAVAILABLE (standard) external_service_unavailable → SERVICE_UNAVAILABLE (standard) not_found / PACKAGE_NOT_FOUND → RESOURCE_NOT_FOUND (standard) PUBLISH_FIELDS_MISSING → MISSING_REQUIRED_FIELD (standard) INTERNAL → INTERNAL_ERROR (standard) datasource_admin_error → DATASOURCE_ADMIN_ERROR (registered) external_import_error → EXTERNAL_IMPORT_ERROR (registered) PUBLISH_MANIFEST_INVALID → PACKAGE_MANIFEST_INVALID (registered) PUBLISH_FAILED → PACKAGE_PUBLISH_FAILED (registered) PACKAGE_DELETE_PARTIAL / PACKAGE_DELETE_FAILED / SETTINGS_ACTION_FAILED Which service is unavailable is carried by `message` — the ledger explicitly asks generic conditions to reuse the catalog instead of registering a per-service 503. The seven registered codes go into ERROR_CODE_LEDGER under their owning packages, including a new @objectstack/service-datasource entry. Also bumps .objectui-sha a136322 → e651c93 now that objectui#2972 has merged, so the bundled Setup console carries the envelope tolerance this change needs. The pin only moves two commits because main had already refreshed it to a136322 — an earlier attempt from this branch's stale base would have swept in eleven unrelated frontend commits including two breaking ones. The route-envelope guard, the conformance suites and their doc comments are updated to match. Guard: 8 modules, 5 conformant / 2 ratcheted / 1 exempt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
os-zhuang
marked this pull request as ready for review
July 30, 2026 02:09
…es (#3843) CI's TypeScript Type Check job runs `check:docs`, which failed: registering seven codes in ERROR_CODE_LEDGER widens the `ErrorCode` union, and every generated reference page that renders an `ApiErrorSchema`-shaped field embeds that union inline. content/docs/references/** is AUTO-GEN (AGENTS.md) — regenerated with `gen:schema && gen:docs` rather than hand-edited. The diff is exactly this change: error-code-ledger.mdx gains the seven codes, and ten pages re-render the widened union in place. 251 generated files back in sync. I had run lint and the package test suites but not this job locally — the gate is in the typecheck job rather than the test job, which is easy to miss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z
…-route-modules-v2zoky
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
…ode vocabulary (#3843 follow-up) (#4009) Comment-only follow-up to #3972 (#3843). No behaviour change. Two sentences in package-envelope.conformance.test.ts's header were written while #3841 was still undecided, and #3843 landed after ADR-0112 settled it: - "why #3841 still owns the vocabulary" — it does not; ADR-0112 (#3988) closed it and ApiErrorSchema.code is a closed union now. - "this module needed MINTED codes" — half true. It had nothing to carry over (its `error` strings were human messages), but only four of the codes here are registered; the three generic conditions reuse the standard catalog, which is what the ledger asks for. Rewritten to state the actual split and why it makes the assertions below load-bearing: an unregistered code fails parse, so BaseResponseSchema.safeParse is what would catch an invented one. package-routes.ts already carried the corrected note; this was the test file missed beside it. Carries an empty changeset — the PR gate requires one, and this releases nothing.
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 #3843. Takes the issue's own recommendation — option 3 then option 1: lift the guard first, then convert what it reports.
Merge order — done
objectstack-ai/objectui#2972 is merged (
e651c93), and this branch now carries the.objectui-shabumpa136322 → e651c93, so the bundled Setup console ships with the envelope tolerance this change needs. Nothing further is gated.The pin only moves two commits because main had already refreshed it to
a136322. An earlier bump from this branch's stale base would have swept in eleven unrelated frontend commits including two breaking ones — caught and reverted before pushing.The four modules
service-settings/settings-routes.tserror, nosuccesson any of 5 bodiesservice-datasource/admin-routes.ts{ error: '<string>' },messagea sibling oferrorrest/external-datasource-routes.ts{ error: '<string>' }+ a privateokrest/package-routes.tssuccess; 2 failures had noerrorat allEvery body now goes through a
sendOk/sendErrorpair. Success payloads keep their keys and move underdata; error bodies become{ success: false, error: { code, message } }, sobody.error.messagefinally reads the service's message instead ofundefined— the asymmetry #3675 opened on, still live two issues later.Two failures that never said why now do:
DELETE /api/v1/packages/:idanswered a bare{ success: false }and a bare{ success: false, failed, cleanups }. They are nowPACKAGE_DELETE_FAILED/PACKAGE_DELETE_PARTIAL, with the per-item arrays undererror.details.Codes: ADR-0112, not carried over
Generic conditions reuse the standard catalog rather than becoming registered synonyms of it, which is what the ledger asks for:
Which service is unavailable is carried by
message. The seven registered codes are added toERROR_CODE_LEDGERunder their owning packages, including a new@objectstack/service-datasourceentry.One deliberate non-change
POST /external/validatekeeps itsok. Unlike the{ ok: true, key }#3689 retired from storage — a private second word forsuccess— thisokis a computed verdict over the federated objects (results.every(r => r.ok)). The request can succeed while the verdict is false, so they are not the same field; it moves insidedatarather than being dropped. The guard's private-success-word rule is scoped took:literals for exactly this reason, and a test pins both flags disagreeing.The guard — repo-wide, not per-package
scripts/check-route-envelope.mjs+pnpm check:route-envelope, wired intolint.ymlbeside the siblingcheck:*guards, which is what this repo's guard convention actually is.The load-bearing assertion is structural rather than per-route: it counts the response write sites per module. When every body goes through the builder pair that count is fixed at two and does not grow with the route list — so a future route that hand-rolls a body fails the guard. That is the coverage a driven-body test can never give, since it can only drive the routes that existed the day it was written.
Going repo-wide is the substantive part, not the tidying. A per-package scan — which is what all three predecessors were — structurally cannot notice a module nobody thought to convert. Running it across the repo found two immediately, neither in #3843's hand survey:
plugin-sharing/share-link-routes.ts— the fifth drifting module. No body carriessuccess, and one answers{ ok: true }: the private second word service-storage success bodies are three shapes, none carryingsuccess: true— the other half of #3675 #3689 retired from storage, one module over. Filed asshare-link-routes.tsis the fifth drifting envelope module — no body carriessuccess, and one answers the{ ok: true }#3689 retired #3983 and pinned by the guard's ratchet. (service-i18n error responses are bare{ error }, not the declared{ success: false, error: {...} }envelope #3675's changeset had cited this file as an example of the good shape — true of itserrorobject and nothing else.)metadata/routes/hmr-routes.ts— declared exempt, with a reason (dev-only SSE endpoint at/api/v1/dev/*, not on the SDK surface), not skipped.Three states are declared deliberately — conformant / ratcheted / exempt — the honest classification ADR-0049 asks for. A route module the scan finds but the table does not declare is an error, never a default: applying
2 / 1 / 1to an unknown module would let a new one pass by coincidence.It parses the TS AST instead of matching regexes, which fixed two real bugs the copies had:
String.replace, whose line-comment pattern also ate//inside string literals and truncated the rest of that line — response writes included. A guard that under-counts call sites passes while drift ships, which is precisely the failure mode this issue is about..json(does not mean "write a response".hmr-routes.tscallsc.req.json()twice to READ a request body; a textual count reports that as two unenveloped responses.Comments and literals are not AST tokens, and request-vs-response is a property of the callee, so both disappear and the 110-line tokenizer is gone. The script carries a
--self-testpinning each case — the sibling guards have none, but both of these bugs survived a review of the regex version.The i18n ratchet, stated rather than hidden.
i18n-service-plugin.tsis declared atresponses: 5, ok: 4, err: 1pointing at #3973. Its error half is consolidated (#3675), but each of its four read routes builds{ success: true, data }inline. Those bodies are correct — not envelope drift — but an unconsolidated builder is a weaker guard. The numbers pin today's structure and drop to2 / 1 / 1when #3973 lands.Consumers
objectui's
packagesreaders were already tolerant (payload?.data ?? payload); its datasource page and the generictype: 'api'action runner now unwrap the envelope and readerror.message— the latter previously toasted[object Object]for any nested error. Merged as objectstack-ai/objectui#2972 and pinned here.SDK methods go through
unwrapResponse, which already returnsbody.datawhen the flag is present, sopackages.list()anddatasources.external.*resolve to exactly the object they always did.Verification (after merging main + full rebuild)
pnpm check:route-envelopepnpm check:console-shapnpm lint@objectstack/spec@objectstack/rest@objectstack/service-settings@objectstack/service-datasource@objectstack/service-storage@objectstack/service-i18nOne note on method, since it nearly produced a wrong bug report: a stale
@objectstack/coredist maderest-meta-auth.test.tsread'unauthenticated'where ADR-0112 had already changed the source to'UNAUTHENTICATED'. That looked like a pre-existing break on main until a fullpnpm buildcleared it. The numbers above are post-rebuild.Follow-ups filed
admin-routes.tsroutes reported the admin service unavailable when the missing dependency is the external datasource service (now bothSERVICE_UNAVAILABLE, distinguished bymessage).share-link-routes.tsis the fifth drifting envelope module — no body carriessuccess, and one answers the{ ok: true }#3689 retired #3983 —share-link-routes.ts, the fifth drifting module, found by the repo-wide guard.