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/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)", 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`', + ]); + }); +});