From b9636d71460056639f3f027bdd3d80aeed8b1b53 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 09:13:01 +0000 Subject: [PATCH 1/2] =?UTF-8?q?test(spec):=20envelopeViolations=20?= =?UTF-8?q?=E2=80=94=20the=20conformance=20check=20BaseResponseSchema=20ca?= =?UTF-8?q?nnot=20express?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every conformance suite from the #3843 line leads with `BaseResponseSchema.safeParse(body)` under a comment calling it "the contract itself, imported — not a restatement of it". That overclaimed, and the gap is demonstrable: BaseResponseSchema.safeParse({ success: true }) // passes BaseResponseSchema.safeParse({ success: true, data: link, link }) // passes The schema declares no `data` (each response type adds its own via `.extend({ data })`) and a plain z.object strips unknown keys rather than rejecting them. So it catches the one drift it was added for — a missing or non-boolean `success`, the flag `unwrapResponse` keys on — and nothing else. The second body is exactly the duplicate-payload drift /share-links shipped until #4038/#4049 removed it, and safeParse passed it the whole time. What was actually catching that drift was the assertions each suite hand-wrote beside it, which works only for as long as whoever writes the next suite remembers to. `envelopeViolations(body)` returns every departure as readable reasons, empty when conformant: `success` must be a boolean; a success body must carry `data` (`undefined` only — null/[]/{}/0/'' are payloads, not absences); a failure body must carry `error` with a string code and message; and no top-level key outside success/data/error/meta, which is the general form of the duplicate-payload drift. It deliberately does NOT check the shape of `data` — that is each route's own payload schema, and conflating the two is what let SettingsNamespacePayload describe a whole body before #3843 and only `data` after it. Ten suites now assert it beside safeParse, and their comments say what each of the two actually proves. Verified the pairing is load-bearing: reintroducing the /share-links duplicate key fails the new assertion and still passes the old one. Verified the rules are not too strict by running every existing suite unchanged — spec 6951, rest 505, runtime 914, storage 220, settings 189, datasource 154, i18n 62, sharing 225, client 200, all green, nothing flagged. Placed in contract.zod.ts beside the schema it completes, alongside the other plain predicates spec/api already exports (standardErrorCodeForHttpStatus, readServiceSelfInfo). No new package. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --- .changeset/envelope-violations-predicate.md | 46 ++++++ .../share-link-envelope.conformance.test.ts | 13 +- ...al-datasource-envelope.conformance.test.ts | 13 +- .../src/package-envelope.conformance.test.ts | 13 +- .../src/error-envelope.conformance.test.ts | 3 +- .../i18n-success-envelope.conformance.test.ts | 5 +- .../__tests__/envelope.conformance.test.ts | 13 +- .../src/error-envelope.conformance.test.ts | 6 +- .../src/envelope.conformance.test.ts | 13 +- .../src/error-envelope.conformance.test.ts | 10 +- .../src/success-envelope.conformance.test.ts | 8 +- packages/spec/src/api/contract.zod.ts | 81 ++++++++++ .../spec/src/api/envelope-violations.test.ts | 138 ++++++++++++++++++ 13 files changed, 345 insertions(+), 17 deletions(-) create mode 100644 .changeset/envelope-violations-predicate.md create mode 100644 packages/spec/src/api/envelope-violations.test.ts diff --git a/.changeset/envelope-violations-predicate.md b/.changeset/envelope-violations-predicate.md new file mode 100644 index 0000000000..bba7773850 --- /dev/null +++ b/.changeset/envelope-violations-predicate.md @@ -0,0 +1,46 @@ +--- +"@objectstack/spec": minor +--- + +**`envelopeViolations` — the conformance check `BaseResponseSchema` cannot express.** + +Every conformance suite from the #3843 line leads with +`BaseResponseSchema.safeParse(body)`, under a comment claiming it is "the contract +itself, imported — not a restatement of it". That overclaimed, and the gap is +demonstrable: + +```ts +BaseResponseSchema.safeParse({ success: true }) // passes +BaseResponseSchema.safeParse({ success: true, data: link, link }) // passes +``` + +The schema declares no `data` — each response type adds its own via +`.extend({ data })` — and a plain `z.object` strips unknown keys rather than +rejecting them. So it catches the one drift it was added for (a missing or +non-boolean `success`, the flag `unwrapResponse` keys on) and nothing else. The +second body above is exactly the duplicate-payload drift `/share-links` shipped +until #4038 / #4049 removed it, and `safeParse` passed it the whole time. + +`envelopeViolations(body)` returns every departure from the declared envelope as +readable reasons, empty when conformant: + +- `success` must be a **boolean** +- a success body must carry `data` (`undefined` only — `null`, `[]`, `{}`, `0` + and `''` are payloads, not absences) +- a failure body must carry `error` with a string `code` and `message` — the + nested form, not the pre-#3675 bare string +- no top-level key outside `success` / `data` / `error` / `meta`, which is the + general form of the duplicate-payload drift + +It deliberately does **not** check the shape of `data`: that is each route's own +payload schema, and conflating the two is what let `SettingsNamespacePayload` +describe a whole body before #3843 and only `data` after it. + +The ten conformance suites now assert it beside `safeParse`, and their comments +say what each of the two actually proves. Reintroducing the `/share-links` +duplicate key is caught by the new assertion and still passes the old one — which +is the demonstration that the pairing is the point. + +Placed in `contract.zod.ts` beside the schema it completes, alongside the other +plain predicates `spec/api` already exports (`standardErrorCodeForHttpStatus`, +`readServiceSelfInfo`). No new package. diff --git a/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts b/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts index 64108aeaf0..5f56ff8df4 100644 --- a/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts +++ b/packages/plugins/plugin-sharing/src/share-link-envelope.conformance.test.ts @@ -34,7 +34,7 @@ */ import { describe, expect, it, vi } from 'vitest'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; import { registerShareLinkRoutes } from './share-link-routes'; import type { ShareLinkService } from './share-link-service'; @@ -214,9 +214,15 @@ describe('share-link envelope (#3983) — success bodies', () => { const { status, body } = await c.run(); expect(status).toBe(c.status); - // The contract itself, imported — not a restatement of it. + // The envelope SKELETON, imported. It is not the whole contract: it declares + // no `data` and strips unknown keys, so on its own it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key. What it DOES + // catch is the missing `success` flag — the drift this line was added for. const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(true); expect(body.error).toBeUndefined(); c.expectData(body.data); @@ -424,6 +430,9 @@ describe('share-link envelope (#3983) — error bodies', () => { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); diff --git a/packages/rest/src/external-datasource-envelope.conformance.test.ts b/packages/rest/src/external-datasource-envelope.conformance.test.ts index cd222f62a6..475b128cd7 100644 --- a/packages/rest/src/external-datasource-envelope.conformance.test.ts +++ b/packages/rest/src/external-datasource-envelope.conformance.test.ts @@ -33,7 +33,7 @@ */ import { describe, it, expect, vi } from 'vitest'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import type { IHttpServer, RouteHandler } from '@objectstack/spec/contracts'; import { registerExternalDatasourceRoutes } from './external-datasource-routes.js'; @@ -127,9 +127,15 @@ describe('external-datasource envelope (#3843) — success bodies', () => { const { status, body } = await c.run(); expect(status).toBe(c.status); - // The contract itself, imported — not a restatement of it. + // The envelope SKELETON, imported. It is not the whole contract: it declares + // no `data` and strips unknown keys, so on its own it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key. What it DOES + // catch is the missing `success` flag — the drift this line was added for. const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(true); expect(body.error).toBeUndefined(); @@ -205,6 +211,9 @@ describe('external-datasource envelope (#3843) — error bodies', () => { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); diff --git a/packages/rest/src/package-envelope.conformance.test.ts b/packages/rest/src/package-envelope.conformance.test.ts index 6f32b2e2fe..1ebb4f26a5 100644 --- a/packages/rest/src/package-envelope.conformance.test.ts +++ b/packages/rest/src/package-envelope.conformance.test.ts @@ -36,7 +36,7 @@ */ import { describe, it, expect } from 'vitest'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import type { RouteHandler } from '@objectstack/spec/contracts'; import { registerPackageRoutes } from './package-routes.js'; @@ -164,9 +164,15 @@ describe('packages envelope (#3843) — success bodies', () => { const { status, body } = await c.run(); expect(status).toBe(c.status); - // The contract itself, imported — not a restatement of it. + // The envelope SKELETON, imported. It is not the whole contract: it declares + // no `data` and strips unknown keys, so on its own it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key. What it DOES + // catch is the missing `success` flag — the drift this line was added for. const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(true); expect(body.error).toBeUndefined(); @@ -297,6 +303,9 @@ describe('packages envelope (#3843) — error bodies', () => { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); diff --git a/packages/runtime/src/error-envelope.conformance.test.ts b/packages/runtime/src/error-envelope.conformance.test.ts index 184ed398d1..dacdebfbe0 100644 --- a/packages/runtime/src/error-envelope.conformance.test.ts +++ b/packages/runtime/src/error-envelope.conformance.test.ts @@ -27,7 +27,7 @@ import { describe, it, expect, vi } from 'vitest'; import { readFileSync } from 'node:fs'; -import { ApiErrorSchema, BaseResponseSchema, DispatcherErrorCode } from '@objectstack/spec/api'; +import { ApiErrorSchema, BaseResponseSchema, DispatcherErrorCode, envelopeViolations } from '@objectstack/spec/api'; import { HttpDispatcher } from './http-dispatcher.js'; import { buildApiError, splitSemanticCode } from './error-envelope.js'; @@ -45,6 +45,7 @@ function expectConformantError(response: { status: number; body: any } | undefin const body = response!.body; expect(BaseResponseSchema.safeParse(body).success).toBe(true); + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); const parsed = ApiErrorSchema.safeParse(body.error); diff --git a/packages/runtime/src/i18n-success-envelope.conformance.test.ts b/packages/runtime/src/i18n-success-envelope.conformance.test.ts index 412a33046a..4c9dd1926d 100644 --- a/packages/runtime/src/i18n-success-envelope.conformance.test.ts +++ b/packages/runtime/src/i18n-success-envelope.conformance.test.ts @@ -41,7 +41,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HttpDispatcher } from './http-dispatcher.js'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import { GetLocalesResponseSchema, GetTranslationsResponseSchema, @@ -86,6 +86,9 @@ describe('/i18n success-envelope conformance (dispatcher domain)', () => { function expectEnvelope(body: unknown) { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect((body as { success?: boolean }).success).toBe(true); } diff --git a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts index cd75bf5aea..7deacb041f 100644 --- a/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts +++ b/packages/services/service-datasource/src/__tests__/envelope.conformance.test.ts @@ -35,7 +35,7 @@ */ import { describe, it, expect, vi } from 'vitest'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import { HonoHttpServer } from '@objectstack/plugin-hono-server'; import { registerDatasourceAdminRoutes } from '../admin-routes.js'; @@ -128,9 +128,15 @@ describe('datasource-admin envelope (#3843) — success bodies', () => { const { status, body } = await c.run(); expect(status).toBe(c.status); - // The contract itself, imported — not a restatement of it. + // The envelope SKELETON, imported. It is not the whole contract: it declares + // no `data` and strips unknown keys, so on its own it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key. What it DOES + // catch is the missing `success` flag — the drift this line was added for. const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(true); expect(body.error).toBeUndefined(); @@ -205,6 +211,9 @@ describe('datasource-admin envelope (#3843) — error bodies', () => { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); diff --git a/packages/services/service-i18n/src/error-envelope.conformance.test.ts b/packages/services/service-i18n/src/error-envelope.conformance.test.ts index c2df1d13eb..6f4833f7a5 100644 --- a/packages/services/service-i18n/src/error-envelope.conformance.test.ts +++ b/packages/services/service-i18n/src/error-envelope.conformance.test.ts @@ -31,7 +31,7 @@ */ import { describe, it, expect, vi } from 'vitest'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; import { I18nServicePlugin } from './i18n-service-plugin'; @@ -167,6 +167,9 @@ describe('i18n error envelope (#3675)', () => { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); @@ -186,6 +189,7 @@ describe('i18n error envelope (#3675)', () => { const { status, body } = await drive(routes, `${BASE}/locales`); expect(status).toBe(500); expect(BaseResponseSchema.safeParse(body).success).toBe(true); + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.error.message).toBe('Internal error'); }); }); diff --git a/packages/services/service-settings/src/envelope.conformance.test.ts b/packages/services/service-settings/src/envelope.conformance.test.ts index 22f6a93eab..71f90b0f33 100644 --- a/packages/services/service-settings/src/envelope.conformance.test.ts +++ b/packages/services/service-settings/src/envelope.conformance.test.ts @@ -32,7 +32,7 @@ */ import { describe, expect, it, vi } from 'vitest'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import { SettingsNamespacePayloadSchema } from '@objectstack/spec/system'; import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; import { SettingsService } from './settings-service'; @@ -152,9 +152,15 @@ describe('settings envelope (#3843) — success bodies', () => { const { status, body } = await c.run(); expect(status).toBe(200); - // The contract itself, imported — not a restatement of it. + // The envelope SKELETON, imported. It is not the whole contract: it declares + // no `data` and strips unknown keys, so on its own it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key. What it DOES + // catch is the missing `success` flag — the drift this line was added for. const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(true); expect(body.error).toBeUndefined(); @@ -269,6 +275,9 @@ describe('settings envelope (#3843) — error bodies', () => { const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); diff --git a/packages/services/service-storage/src/error-envelope.conformance.test.ts b/packages/services/service-storage/src/error-envelope.conformance.test.ts index dba6db01d7..8722022bd1 100644 --- a/packages/services/service-storage/src/error-envelope.conformance.test.ts +++ b/packages/services/service-storage/src/error-envelope.conformance.test.ts @@ -38,7 +38,7 @@ import { describe, it, expect } from 'vitest'; import { promises as fs } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; import { LocalStorageAdapter } from './local-storage-adapter'; import { StorageMetadataStore } from './metadata-store'; @@ -286,9 +286,15 @@ describe('storage error envelope (#3675)', () => { const { status, body } = await c.run(); expect(status).toBe(c.status); - // The contract itself, imported — not a restatement of it. + // The envelope SKELETON, imported. It is not the whole contract: it declares + // no `data` and strips unknown keys, so on its own it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key. What it DOES + // catch is the missing `success` flag — the drift this line was added for. const parsed = BaseResponseSchema.safeParse(body); expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // The declared envelope in full — `safeParse` alone passes a body with no + // `data`, or a payload duplicated into a stray top-level key (#4049). + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(false); expect(body.error.code).toBe(c.code); diff --git a/packages/services/service-storage/src/success-envelope.conformance.test.ts b/packages/services/service-storage/src/success-envelope.conformance.test.ts index 699e127fad..7cf16455d0 100644 --- a/packages/services/service-storage/src/success-envelope.conformance.test.ts +++ b/packages/services/service-storage/src/success-envelope.conformance.test.ts @@ -55,7 +55,7 @@ import { describe, it, expect } from 'vitest'; import { promises as fs } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; -import { BaseResponseSchema } from '@objectstack/spec/api'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; import { PresignedUrlResponseSchema, FileUploadResponseSchema, @@ -276,9 +276,13 @@ describe('storage success envelope (#3689)', () => { const { status, body } = await c.run(); expect(status).toBe(200); - // The shared envelope, imported — not a restatement of it. + // The shared envelope skeleton, imported. Note what it does NOT prove: it + // declares no `data` and strips unknown keys, so it passes `{ success: true }` + // and passes a payload duplicated into a stray top-level key (#4049). const base = BaseResponseSchema.safeParse(body); expect(base.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + // …so this is the assertion that actually says "the declared envelope". + expect(envelopeViolations(body), `not the declared envelope: ${JSON.stringify(body)}`).toEqual([]); expect(body.success).toBe(true); expect(body.error).toBeUndefined(); diff --git a/packages/spec/src/api/contract.zod.ts b/packages/spec/src/api/contract.zod.ts index b01cebc351..1779a18ff1 100644 --- a/packages/spec/src/api/contract.zod.ts +++ b/packages/spec/src/api/contract.zod.ts @@ -37,6 +37,24 @@ export const ApiErrorSchema = lazySchema(() => z.object({ requestId: z.string().optional().describe('Request ID for tracking'), })); +/** + * The envelope SKELETON — deliberately not the whole response contract. + * + * It does not declare `data`: each response type adds its own via + * `BaseResponseSchema.extend({ data: … })` (see `export.zod.ts`, + * `automation-api.zod.ts`). It is also a plain `z.object`, so unknown keys are + * stripped and pass rather than failing. + * + * Both are intentional, and both mean `safeParse` alone does NOT prove a body is + * envelope-conformant. It catches the big one — a missing or non-boolean + * `success`, which is what `ObjectStackClient.unwrapResponse` keys on and what + * #3675 / #3689 / #3843 were about — but it accepts `{ success: true }` with no + * payload at all, and it accepts a payload duplicated into a stray top-level key + * (`{ success: true, data: link, link }`, the drift #4038 / #4049 removed). + * + * Use {@link envelopeViolations} when you mean "is this actually the declared + * envelope"; use this schema when you mean "does it parse as one". + */ export const BaseResponseSchema = lazySchema(() => z.object({ success: z.boolean().describe('Operation success status'), error: ApiErrorSchema.optional().describe('Error details if success is false'), @@ -48,6 +66,69 @@ export const BaseResponseSchema = lazySchema(() => z.object({ }).optional().describe('Response metadata'), })); +/** The only keys a declared REST body may carry at its top level. */ +const ENVELOPE_KEYS = new Set(['success', 'data', 'error', 'meta']); + +/** + * Every way `body` departs from the declared envelope, as readable reasons. + * Empty array ⇒ conformant. + * + * This is the check {@link BaseResponseSchema} cannot express on its own, and it + * exists because the gap was not theoretical: the `/share-links` dispatcher + * domain shipped `{ success: true, data: link, link }` for as long as nobody + * looked, and `safeParse` passed it the whole time (#4038). The conformance + * suites each hand-wrote their own version of the missing assertions; this is + * that check, once, so a new module inherits it instead of depending on whoever + * writes the suite remembering to. + * + * The rules, and why each one: + * + * • `success` must be a **boolean** — the flag `unwrapResponse` keys on. A body + * without it is handed to callers raw, which is how one SDK method returned + * two different shapes depending on which surface served the route (#3636). + * • a success body must carry `data` — `{ success: true }` alone tells a caller + * nothing and parses fine today. + * • a failure body must carry `error` with a string `code` and `message` — the + * nested form, not the pre-#3675 bare string. + * • no top-level key outside `success` / `data` / `error` / `meta` — this is the + * general form of the duplicate-payload drift. A second spelling of the + * payload beside `data` is how a producer keeps two dialects alive at once. + * + * Deliberately NOT checked: the shape of `data` itself. That is each route's own + * payload schema, and conflating the two is what let `SettingsNamespacePayload` + * describe a whole body before #3843 and only `data` after it. + */ +export function envelopeViolations(body: unknown): string[] { + if (body === null || typeof body !== 'object' || Array.isArray(body)) { + return [`body is ${Array.isArray(body) ? 'an array' : String(body === null ? 'null' : typeof body)}, not an envelope object`]; + } + const b = body as Record; + const out: string[] = []; + + if (typeof b.success !== 'boolean') { + out.push(`success is ${b.success === undefined ? 'missing' : `\`${typeof b.success}\``}, must be a boolean`); + } else if (b.success) { + if (b.data === undefined) out.push('success body carries no `data`'); + if (b.error !== undefined) out.push('success body carries an `error`'); + } else { + const err = b.error; + if (err === undefined || typeof err !== 'object' || err === null) { + out.push(`failure body's \`error\` is ${err === undefined ? 'missing' : `a ${typeof err}`}, must be an object`); + } else { + const e = err as Record; + if (typeof e.code !== 'string') out.push('error.code is missing or not a string'); + if (typeof e.message !== 'string') out.push('error.message is missing or not a string'); + } + } + + for (const key of Object.keys(b)) { + if (!ENVELOPE_KEYS.has(key)) { + out.push(`stray top-level key \`${key}\` — the payload belongs under \`data\``); + } + } + return out; +} + // ========================================== // 2. Request Payloads (Inputs) // ========================================== diff --git a/packages/spec/src/api/envelope-violations.test.ts b/packages/spec/src/api/envelope-violations.test.ts new file mode 100644 index 0000000000..345820efe9 --- /dev/null +++ b/packages/spec/src/api/envelope-violations.test.ts @@ -0,0 +1,138 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `envelopeViolations` — the conformance check `BaseResponseSchema` cannot express. + * + * The schema does not declare `data`, and a plain `z.object` strips unknown keys + * rather than rejecting them. So `safeParse` catches a missing `success` flag — + * the drift #3675 / #3689 / #3843 were about — and nothing else. + * + * That gap was not theoretical. The `/share-links` dispatcher domain shipped + * `{ success: true, data: link, link }` for as long as nobody looked, and + * `safeParse` passed it the whole time (#4038, removed in #4049). Each conformance + * suite had hand-written its own version of the missing assertions, which works + * only for as long as whoever writes the next suite remembers to. + * + * The pairing below is the point of this file: every case asserts BOTH what the + * schema says and what the predicate says, so the two never silently converge and + * the reader can see exactly which check is load-bearing for which drift. + */ + +import { describe, expect, it } from 'vitest'; +import { BaseResponseSchema, envelopeViolations } from './contract.zod'; + +const parses = (b: unknown) => BaseResponseSchema.safeParse(b).success; +const conformant = (b: unknown) => envelopeViolations(b).length === 0; + +describe('envelopeViolations — conformant bodies', () => { + it('accepts a success body', () => { + expect(envelopeViolations({ success: true, data: { id: 'a' } })).toEqual([]); + }); + + it('accepts `data` that is falsy, empty, or null — those are payloads', () => { + // Only `undefined` means "no payload". A route legitimately answering an + // empty list or a null record must not be called a violation. + for (const data of [[], {}, null, 0, '', false]) { + expect(envelopeViolations({ success: true, data }), JSON.stringify(data)).toEqual([]); + } + }); + + it('accepts a failure body with the nested error', () => { + expect(envelopeViolations({ success: false, error: { code: 'NOT_FOUND', message: 'gone' } })).toEqual([]); + }); + + it('accepts `meta` beside the payload — the dispatcher helper emits it', () => { + expect(envelopeViolations({ success: true, data: 1, meta: { timestamp: 't' } })).toEqual([]); + // `deps.success` builds `{ success, data, meta }` with meta possibly undefined. + expect(envelopeViolations({ success: true, data: 1, meta: undefined })).toEqual([]); + }); +}); + +describe('envelopeViolations — what the schema already catches', () => { + it('a missing success flag: both reject', () => { + // The pre-#3983 bare body. This is the one drift `safeParse` does catch, + // because `success` is required and it is the flag `unwrapResponse` keys on. + const body = { links: [] }; + expect(parses(body)).toBe(false); + expect(conformant(body)).toBe(false); + }); + + it('a bare-string error: both reject', () => { + // The pre-#3675 dialect, where `body.error.message` read `undefined`. + const body = { error: 'boom' }; + expect(parses(body)).toBe(false); + expect(conformant(body)).toBe(false); + }); +}); + +describe('envelopeViolations — what the schema MISSES', () => { + // Each of these parses clean. That is why the suites cannot lead with + // `safeParse` and call it a contract check. + + it('a success body with no data at all', () => { + const body = { success: true }; + expect(parses(body)).toBe(true); // ← schema is satisfied + expect(conformant(body)).toBe(false); + expect(envelopeViolations(body)).toContain('success body carries no `data`'); + }); + + it('the duplicate-payload drift #4049 removed', () => { + // `{ success: true, data: link, link }` — the payload under BOTH the + // envelope's `data` and a legacy top-level key, so two dialects stay alive + // at once and no consumer has to choose. + const body = { success: true, data: { id: 'l1' }, link: { id: 'l1' } }; + expect(parses(body)).toBe(true); // ← schema is satisfied + expect(envelopeViolations(body)).toEqual([ + 'stray top-level key `link` — the payload belongs under `data`', + ]); + }); + + it('a payload left at the top level beside the flag', () => { + // The shape `GET /ai/agents` still answers in, minus the flag (#4053). + const body = { success: true, data: [], agents: [] }; + expect(parses(body)).toBe(true); + expect(conformant(body)).toBe(false); + }); + + it('a failure body with no error', () => { + const body = { success: false }; + expect(parses(body)).toBe(true); + expect(envelopeViolations(body)).toContain("failure body's `error` is missing, must be an object"); + }); + + it('a failure body whose error lacks code or message', () => { + expect(envelopeViolations({ success: false, error: {} })).toEqual([ + 'error.code is missing or not a string', + 'error.message is missing or not a string', + ]); + }); + + it('a success body that also carries an error', () => { + expect(envelopeViolations({ success: true, data: 1, error: { code: 'X', message: 'y' } })) + .toContain('success body carries an `error`'); + }); +}); + +describe('envelopeViolations — non-objects', () => { + it('names what it got, rather than throwing', () => { + for (const body of [null, undefined, 42, 'str', true]) { + const v = envelopeViolations(body); + expect(v.length, String(body)).toBe(1); + expect(v[0]).toMatch(/not an envelope object/); + } + expect(envelopeViolations([1, 2])[0]).toMatch(/is an array/); + }); +}); + +describe('envelopeViolations — reports every violation, not just the first', () => { + it('a body can be wrong in several ways at once', () => { + // A reader fixing one reason should see the rest in the same run rather than + // rediscovering them one CI round at a time. + const v = envelopeViolations({ success: true, link: 1, links: 2 }); + expect(v).toEqual([ + 'success body carries no `data`', + 'stray top-level key `link` — the payload belongs under `data`', + 'stray top-level key `links` — the payload belongs under `data`', + ]); + }); +}); From 04debd7abe333515cd132ac2e17595b8a79f0bc3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 09:34:09 +0000 Subject: [PATCH 2/2] chore(spec): regenerate the API-surface snapshot for envelopeViolations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:api-surface` is a ratchet on `@objectstack/spec`'s public exports, and the new predicate is an addition to it: ./api + envelopeViolations (function) 0 breaking (removed/narrowed), 1 added. One line, alphabetically beside the other predicates that module already exports. Two things this caught that are worth writing down. The gate lives in the TypeScript Type Check job, which runs TEN steps — I had found four of them by grepping a window of lint.yml that cut off before the rest, and ran only those. The full list is: spec tsc, check:docs, check:skill-refs, check:react-blocks, a workspace build, examples typecheck, downstream-contract typecheck, check:api-surface, check:skill-examples, and the two i18n checks. All ten pass locally now. And `gen:api-surface` reads `dist/index.d.ts`, so it fails outright under `OS_SKIP_DTS=1` — the flag I had been using to keep local builds fast. Rebuilding spec with declarations is a prerequisite for regenerating the snapshot, not an optional speed-up. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CYbS3kS8xzsHNXFTzp4e2z --- packages/spec/api-surface.json | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index c6b6f0c95c..e80dd41173 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3088,6 +3088,7 @@ "WorkflowTransitionRequestSchema (const)", "WorkflowTransitionResponse (type)", "WorkflowTransitionResponseSchema (const)", + "envelopeViolations (function)", "getAuthEndpointUrl (function)", "getDefaultRouteRegistrations (function)", "readServiceSelfInfo (function)",