diff --git a/.changeset/unknown-key-strictness-data-step.md b/.changeset/unknown-key-strictness-data-step.md new file mode 100644 index 0000000000..7cec7ffe89 --- /dev/null +++ b/.changeset/unknown-key-strictness-data-step.md @@ -0,0 +1,44 @@ +--- +"@objectstack/spec": minor +--- + +Hooks and datasources reject unknown keys (#4001 data step). + +Closes the last two entries in the strictness ledger that still carried a +provisional classification. Both were confirmed authorable the same way: they sit +in `BUILTIN_METADATA_TYPE_SCHEMAS`, so one shape backs `defineStack()` parsing, +`/api/v1/meta/types/:type`, and the Studio form. + +Now strict: + +- `HookSchema` + its `retryPolicy`, and both hook-body branches + (`ExpressionBodySchema`, `ScriptBodySchema`). A misspelt `capabilities` + stripped to the empty default and the sandbox threw at invocation time instead + of at parse; a misspelt `timeoutMs`/`memoryMb` silently downgraded the body to + the enclosing hook's limits. +- `DatasourceSchema` + `pool` / `healthCheck` / `ssl` / `retryPolicy`, + `ExternalDatasourceSettingsSchema` + its `validation` block, + `DatasourceCapabilities`, and `DriverDefinitionSchema`. + +Deliberately still tolerant: + +- `HookContextSchema` and its `session` / `provenance` / `user` blocks — the + runtime shape the engine hands a handler. Strictness there would turn an + engine-internal enrichment (as `provenance` was in #3712) into a breaking + change for anyone parsing a context they were given. +- `datasource.config` and `readReplicas` — per-driver by construction; the + driver's own `configSchema` validates them. + +Errors are self-fixing: connection keys written one level too high (`host`, +`port`, `filename`, `url`, …) are prescribed into `config`; a top-level +`password` is pointed at `external.credentialsRef` rather than merely relocated; +and the two near-miss spellings that cross between shapes carry aliases +(hook-level `timeout` vs body-level `timeoutMs`; hook `retryPolicy.backoffMs` vs +datasource `retryPolicy.baseDelayMs`). + +Note for anyone reading the earlier steps: strictness does not change the +published JSON Schema. `build-schemas.ts` converts with `io: 'output'`, where zod +emits `additionalProperties: false` for `.strip()` objects too — verified by +regenerating both ways (`Datasource.json` is byte-identical). The JSON Schema was +already advertising `additionalProperties: false` while the parse silently +dropped keys; this aligns the parse with the published contract. diff --git a/content/docs/releases/v17.mdx b/content/docs/releases/v17.mdx index 0ab0823350..1073be5022 100644 --- a/content/docs/releases/v17.mdx +++ b/content/docs/releases/v17.mdx @@ -358,6 +358,38 @@ schema to the two highest-risk authorable surfaces, per the triage in schema carries `additionalProperties: false` into the Studio form and `registerFlow()` config validation, so a mis-keyed approval `config` is rejected at registration too. +- **Hooks** — `HookSchema`, its `retryPolicy`, and both hook-body branches + (`expression` and `js`). A body was the worst place to lose a key quietly: a + misspelt `capabilities` stripped to the empty default and the sandbox then + threw at *invocation* time, on whichever code path first touched `ctx.api` — + far from the typo. A misspelt `timeoutMs`/`memoryMb` silently downgraded the + body to the enclosing hook's looser limits. Two near-miss spellings now carry + aliases because they are genuinely easy to cross: hook-level `timeout` vs + body-level `timeoutMs`, and hook `retryPolicy.backoffMs` vs datasource + `retryPolicy.baseDelayMs`. `HookContextSchema` — the runtime shape the engine + hands your handler — stays tolerant and always will. +- **Datasources** — `DatasourceSchema` with its `pool` / `healthCheck` / `ssl` / + `retryPolicy` blocks, the ADR-0015 `external` federation settings and their + `validation` policy, `DatasourceCapabilities`, and `DriverDefinitionSchema`. + `config` and `readReplicas` stay **open** records: their shape is per-driver + and the driver's own `configSchema` validates them. That openness is why the + top level had to close — a connection key written one level too high (`host` + next to `driver` instead of inside `config`) was stripped, and the datasource + then connected on driver defaults rather than failing. Those keys now + prescribe the move into `config`; a top-level `password` is instead pointed at + `external.credentialsRef`, because relocating an inlined secret is not the fix. + A dropped key in `capabilities` was quieter still: an unregistered capability + reads as `false`, so the engine stopped pushing that work down to the driver + and recomputed it in memory. + +One clarification, since these flips are easy to over-read: making a schema +strict does **not** change its published JSON Schema. `build-schemas.ts` +converts with `io: 'output'`, and in output mode zod emits +`additionalProperties: false` for a `.strip()` object too — the post-parse shape +genuinely has no extra keys. So the JSON Schema was already advertising +`additionalProperties: false` while the zod parse quietly accepted and discarded +unknown keys. These flips align the parse with the contract that was already +published; they do not widen it. Every rejection is written to be self-fixing: it names the offending key and, where recognisable, the canonical spelling (`steps` → `nodes`, edge diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.md b/docs/audits/2026-07-unknown-key-strictness-ledger.md index 1bd1542226..635b4362a1 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.md @@ -157,10 +157,10 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). | `field.zod.ts` | 11 | authorable | partially strict | | `filter.zod.ts` / `query.zod.ts` | 11+10 | open | query dialect — user data flows through; validated semantically elsewhere | | `driver-nosql.zod.ts` / `driver.zod.ts` / `driver-sql.zod.ts` | 10+9+2 | wire | driver capability contracts | -| `datasource.zod.ts` | 9 | authorable (p) | stack-authored config — **candidate** | +| `datasource.zod.ts` | 9 | authorable | **strict as of #4001 data step** — all 9: `DatasourceSchema` (+ `pool` / `healthCheck` / `ssl` / `retryPolicy`), `ExternalDatasourceSettingsSchema` (+ `validation`), `DatasourceCapabilities`, `DriverDefinitionSchema`. `config` + `readReplicas` stay `z.record` by construction (per-driver shapes; the driver's own `configSchema` validates them) — which is precisely why the top level had to close: a connection key written one level too high was stripped, and the datasource then connected on driver defaults instead of failing | | `analytics.zod.ts` | 8 | mixed (p) | | | `document.zod.ts` | 8 | wire (p) | | -| `hook.zod.ts` / `hook-body.zod.ts` | 6+2 | authorable (p) | `defineHook` — **candidate** | +| `hook.zod.ts` / `hook-body.zod.ts` | 6+2 | mixed | **strict as of #4001 data step** for the AUTHORING shapes: `HookSchema` (+ `retryPolicy`) and both body branches (`ExpressionBodySchema` / `ScriptBodySchema`). `HookContextSchema` and its `session` / `provenance` / `user` blocks are the RUNTIME shape the engine hands a handler — they stay tolerant, and must: strictness there would make an engine-internal enrichment (as `provenance` was in #3712) a breaking change for anyone parsing a context they were given. The file's old blanket `authorable (p)` was too wide — verification split it | | `mapping.zod.ts` | 3 | authorable (p) | | | `external-catalog.zod.ts` | 4 | wire (p) | | | `field-value.zod.ts` / `seed.zod.ts` / `validation.zod.ts` | 1 ea | mixed (p) | | @@ -213,13 +213,11 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). ## Next steps (verify-then-enforce, one shape at a time) -1. `data/hook.zod.ts`, `data/datasource.zod.ts` — `defineHook` / stack config - (both still provisional (p) classifications — verify before tightening). -2. The `@objectstack/lint` unknown-key WARNING layer: non-breaking, shippable +1. The `@objectstack/lint` unknown-key WARNING layer: non-breaking, shippable in a minor, and it extends AI-detectable coverage to every remaining authorable site at once while accumulating evidence (which keys real tenant projects actually carry) for a v18 strict close-out. -3. Promote this ledger to a machine-checked gate (pattern of +2. Promote this ledger to a machine-checked gate (pattern of `packages/spec/liveness/` + `check:liveness`) once enough of the surface is classified that the table above is enforceable rather than descriptive. @@ -241,5 +239,28 @@ Done in the app step, PR A: the seven audit-dead AppSchema keys tombstoned migration entry), clearing the enforce-or-remove precondition for the app strict step (PR B). +Done in the data step: `data/hook.zod.ts` + `data/hook-body.zod.ts` + +`data/datasource.zod.ts` — the last two entries that carried a provisional +`(p)` classification. Verification changed the answer for one of them: the +blanket `authorable (p)` on `hook.zod.ts` was too wide, because +`HookContextSchema` in the same file is a runtime shape and stays tolerant. +Both types were confirmed authorable the same way — they sit in +`BUILTIN_METADATA_TYPE_SCHEMAS`, so one shape backs `defineStack()` parsing, +`/api/v1/meta/types/:type`, and the Studio form. + +Measured while doing it, and worth recording because it contradicts the +assumption the earlier steps were written under: **strictness does not change +the published JSON Schema.** `build-schemas.ts` converts with the default +`io: 'output'`, and in output mode zod emits `additionalProperties: false` for +a `.strip()` object too — the post-parse shape genuinely has no extra keys. +Verified by regenerating both ways: `Datasource.json` is byte-identical before +and after. So the JSON Schema had been advertising `additionalProperties: false` +while the zod parse quietly accepted and dropped unknown keys. These flips align +the parse with the contract that was already published, rather than widening it. +(Note this cuts against the `approval.zod.ts` note above, which reads as though +the flip carried strictness INTO the JSON schema. It did not; approval's schema +would have said `false` regardless. The registration-time rejection it describes +is real, but it came from the published schema, not from the flip.) + Long tail stays gated on a verification pass per shape — never a one-shot "make all ~453 sites strict" (ADR-0054 ratchet; #4001's own recommendation). diff --git a/packages/spec/src/data/datasource.zod.ts b/packages/spec/src/data/datasource.zod.ts index f77ceee130..a5015edbc1 100644 --- a/packages/spec/src/data/datasource.zod.ts +++ b/packages/spec/src/data/datasource.zod.ts @@ -8,6 +8,261 @@ import { z } from 'zod'; * Can be a built-in driver or a plugin-contributed driver (e.g., "com.vendor.snowflake"). */ import { lazySchema } from '../shared/lazy-schema'; +import { strictUnknownKeyError } from '../shared/suggestions.zod'; + +/* + * ── Unknown-key strictness (#4001 data step) ──────────────────────────────── + * + * Every AUTHORING shape in this module is `.strict()`. `datasource` is a + * registered metadata type (BUILTIN_METADATA_TYPE_SCHEMAS), so one shape backs + * `defineDatasource()`, `defineStack({ datasources })`, the + * `/api/v1/meta/datasource` endpoint, and the Setup → Datasources form. + * + * TWO ESCAPE HATCHES STAY OPEN, and must: + * - `config` is per-driver by construction (a sqlite `filename` and a + * postgres `host`/`port` share no shape), so it stays `z.record`. The + * driver's own `configSchema` is what validates it. + * - `readReplicas` carries the same per-driver config objects. + * + * That openness is exactly why the TOP level had to close. Before this, a + * connection key written one level too high — `host` next to `driver` instead + * of inside `config` — was stripped in silence, and the datasource then + * connected on driver defaults (localhost, default port) rather than failing. + * A misplaced `password` is the same bug wearing a worse hat, which is why it + * is prescribed toward `external.credentialsRef` rather than merely relocated. + */ + +/** Keys {@link DriverDefinitionSchema} declares (drift-guarded by datasource.test.ts). */ +const DRIVER_DEFINITION_KEYS = ['id', 'label', 'description', 'icon', 'configSchema', 'capabilities'] as const; + +/** Keys {@link ExternalDatasourceSettingsSchema} declares (drift-guarded by datasource.test.ts). */ +const EXTERNAL_SETTINGS_KEYS = [ + 'label', 'allowedSchemas', 'allowWrites', 'validation', + 'credentialsRef', 'queryTimeoutMs', 'requirePermission', +] as const; + +/** Keys the external `validation` block declares (drift-guarded by datasource.test.ts). */ +const EXTERNAL_VALIDATION_KEYS = ['onMismatch', 'checkOnBoot', 'checkIntervalMs'] as const; + +/** Keys {@link DatasourceSchema} declares (drift-guarded by datasource.test.ts). */ +const DATASOURCE_KEYS = [ + 'name', 'label', 'driver', 'config', 'pool', 'readReplicas', 'capabilities', + 'healthCheck', 'ssl', 'retryPolicy', 'description', 'active', 'autoConnect', + 'schemaMode', 'external', 'origin', +] as const; + +/** Keys the datasource `pool` block declares (drift-guarded by datasource.test.ts). */ +const POOL_KEYS = ['min', 'max', 'idleTimeoutMillis', 'connectionTimeoutMillis'] as const; + +/** Keys the datasource `healthCheck` block declares (drift-guarded by datasource.test.ts). */ +const HEALTH_CHECK_KEYS = ['enabled', 'intervalMs', 'timeoutMs'] as const; + +/** Keys the datasource `ssl` block declares (drift-guarded by datasource.test.ts). */ +const SSL_KEYS = ['enabled', 'rejectUnauthorized', 'ca', 'cert', 'key'] as const; + +/** Keys the datasource `retryPolicy` block declares (drift-guarded by datasource.test.ts). */ +const DATASOURCE_RETRY_POLICY_KEYS = ['maxRetries', 'baseDelayMs', 'maxDelayMs', 'backoffMultiplier'] as const; + +/** A connection detail written one level too high — it belongs inside `config`. */ +const belongsInConfig = (key: string) => + `\`${key}\` is a driver connection detail — it belongs inside \`config\`, not at the top ` + + `level. Move it to \`config: { ${key}: … }\`; the driver's own configSchema validates it there.`; + +const driverDefinitionUnknownKeyError = strictUnknownKeyError({ + surface: 'this driver definition', + knownKeys: DRIVER_DEFINITION_KEYS, + aliases: { + name: 'id', + driver: 'id', + title: 'label', + config: 'configSchema', + schema: 'configSchema', + capability: 'capabilities', + }, + history: 'Until #4001 these were dropped silently — the driver still registered.', +}); + +const externalSettingsUnknownKeyError = strictUnknownKeyError({ + surface: "this datasource's external settings", + knownKeys: EXTERNAL_SETTINGS_KEYS, + aliases: { + schemas: 'allowedSchemas', + allowedschema: 'allowedSchemas', + writable: 'allowWrites', + allowwrite: 'allowWrites', + credentials: 'credentialsRef', + secretref: 'credentialsRef', + timeoutms: 'queryTimeoutMs', + querytimeout: 'queryTimeoutMs', + permission: 'requirePermission', + }, + guidance: { + password: + '`password` must never be inlined. Put the secret in the secrets store and reference ' + + 'it with `credentialsRef` (e.g. `credentialsRef: "secret:warehouse/password"`).', + readOnly: + '`readOnly` is not an external-settings key. Use `allowWrites: false` here for the ' + + 'datasource-wide gate, or `capabilities.readOnly` to describe the driver.', + }, + history: 'Until #4001 these were dropped silently — federation ran on the defaults instead.', +}); + +const externalValidationUnknownKeyError = strictUnknownKeyError({ + surface: "this datasource's external validation policy", + knownKeys: EXTERNAL_VALIDATION_KEYS, + aliases: { + onmismatch: 'onMismatch', + mismatch: 'onMismatch', + checkonboot: 'checkOnBoot', + validateonboot: 'checkOnBoot', + interval: 'checkIntervalMs', + checkinterval: 'checkIntervalMs', + }, + history: + 'Until #4001 these were dropped silently — drift checking ran on the defaults ' + + '(fail on mismatch, check at boot) regardless of what was written.', +}); + +const datasourceUnknownKeyError = strictUnknownKeyError({ + surface: 'this datasource', + knownKeys: DATASOURCE_KEYS, + aliases: { + type: 'driver', + connection: 'config', + connectionconfig: 'config', + options: 'config', + enabled: 'active', + pooling: 'pool', + replicas: 'readReplicas', + mode: 'schemaMode', + schema_mode: 'schemaMode', + federation: 'external', + retry: 'retryPolicy', + tls: 'ssl', + }, + guidance: { + host: belongsInConfig('host'), + port: belongsInConfig('port'), + database: belongsInConfig('database'), + user: belongsInConfig('user'), + username: belongsInConfig('username'), + filename: belongsInConfig('filename'), + url: belongsInConfig('url'), + connectionString: belongsInConfig('connectionString'), + password: + '`password` must never be inlined on a datasource. Interpolate it from the environment ' + + 'inside `config`, or for an external datasource reference the secrets store via ' + + '`external.credentialsRef`.', + }, + history: + 'Until #4001 these were dropped silently — a connection key written one level too high ' + + 'left the datasource connecting on driver defaults rather than failing.', +}); + +const poolUnknownKeyError = strictUnknownKeyError({ + surface: "this datasource's pool config", + knownKeys: POOL_KEYS, + aliases: { + minimum: 'min', + maximum: 'max', + minconnections: 'min', + maxconnections: 'max', + idletimeout: 'idleTimeoutMillis', + idletimeoutms: 'idleTimeoutMillis', + connectiontimeout: 'connectionTimeoutMillis', + connectiontimeoutms: 'connectionTimeoutMillis', + acquiretimeoutmillis: 'connectionTimeoutMillis', + }, + history: + 'Until #4001 these were dropped silently — the pool ran on its defaults (min 0, max 10) ' + + 'no matter what was written. Note both timeouts end in `Millis`, not `Ms`.', +}); + +const healthCheckUnknownKeyError = strictUnknownKeyError({ + surface: "this datasource's healthCheck config", + knownKeys: HEALTH_CHECK_KEYS, + aliases: { + active: 'enabled', + interval: 'intervalMs', + intervalmillis: 'intervalMs', + timeout: 'timeoutMs', + timeoutmillis: 'timeoutMs', + }, + history: 'Until #4001 these were dropped silently — health checks ran on the defaults.', +}); + +const sslUnknownKeyError = strictUnknownKeyError({ + surface: "this datasource's ssl config", + knownKeys: SSL_KEYS, + aliases: { + active: 'enabled', + ssl: 'enabled', + tls: 'enabled', + rejectunauthorised: 'rejectUnauthorized', + cacert: 'ca', + certificate: 'cert', + clientcert: 'cert', + privatekey: 'key', + clientkey: 'key', + }, + guidance: { + insecure: + '`insecure` is not an ssl key. To accept a self-signed certificate set ' + + '`rejectUnauthorized: false` — deliberately, and never against a production database.', + }, + history: + 'Until #4001 these were dropped silently — which meant a TLS setting that never took ' + + 'effect looked identical to one that did.', +}); + +const datasourceRetryPolicyUnknownKeyError = strictUnknownKeyError({ + surface: "this datasource's retryPolicy", + knownKeys: DATASOURCE_RETRY_POLICY_KEYS, + aliases: { + retries: 'maxRetries', + attempts: 'maxRetries', + backoffms: 'baseDelayMs', + basedelay: 'baseDelayMs', + delayms: 'baseDelayMs', + maxdelay: 'maxDelayMs', + multiplier: 'backoffMultiplier', + backoff: 'backoffMultiplier', + }, + history: + 'Until #4001 these were dropped silently — reconnects ran on the defaults. Note a hook ' + + 'retryPolicy spells its delay `backoffMs`; a datasource spells it `baseDelayMs`.', +}); + +const capabilitiesUnknownKeyError = strictUnknownKeyError({ + surface: 'these datasource capabilities', + knownKeys: [ + 'transactions', 'queryFilters', 'queryAggregations', 'querySorting', + 'queryPagination', 'queryWindowFunctions', 'querySubqueries', 'joins', + 'fullTextSearch', 'readOnly', 'dynamicSchema', + ], + aliases: { + transaction: 'transactions', + filters: 'queryFilters', + filtering: 'queryFilters', + aggregations: 'queryAggregations', + aggregation: 'queryAggregations', + sorting: 'querySorting', + sort: 'querySorting', + pagination: 'queryPagination', + windowfunctions: 'queryWindowFunctions', + subqueries: 'querySubqueries', + join: 'joins', + fulltext: 'fullTextSearch', + search: 'fullTextSearch', + readonly: 'readOnly', + schemaless: 'dynamicSchema', + }, + history: + 'Until #4001 these were dropped silently — and a capability that fails to register ' + + 'reads as FALSE, so the engine quietly stopped pushing that work down to the driver ' + + 'and recomputed it in memory instead.', +}); + export const DriverType = z.string().describe('Underlying driver identifier'); /** @@ -33,7 +288,7 @@ export const DriverDefinitionSchema = lazySchema(() => z.object({ * What this driver supports out-of-the-box. */ capabilities: z.lazy(() => DatasourceCapabilities).optional(), -})); +}, { error: driverDefinitionUnknownKeyError }).strict()); /** * Datasource Capabilities Schema @@ -86,7 +341,7 @@ export const DatasourceCapabilities = z.object({ /** Is scheme-less (needs schema inference)? */ dynamicSchema: z.boolean().default(false), -}); +}, { error: capabilitiesUnknownKeyError }).strict(); /** * Schema Ownership Mode (ADR-0015) @@ -126,14 +381,16 @@ export const ExternalDatasourceSettingsSchema = z.object({ .describe('Validate federated objects against the remote schema at boot.'), checkIntervalMs: z.number().optional() .describe('Optional background drift-check interval in milliseconds.'), - }).default({ onMismatch: 'fail', checkOnBoot: true }).describe('Boot/drift validation policy'), + }, { error: externalValidationUnknownKeyError }).strict() + .default({ onMismatch: 'fail', checkOnBoot: true }).describe('Boot/drift validation policy'), credentialsRef: z.string().optional() .describe('Reference into the secrets store; never inline credentials.'), queryTimeoutMs: z.number().default(30_000) .describe('Hard cap on per-query execution time.'), requirePermission: z.string().optional() .describe('Optional convenience: gate the entire datasource behind a single role.'), -}).describe('External datasource federation settings (schemaMode != "managed")'); +}, { error: externalSettingsUnknownKeyError }).strict() + .describe('External datasource federation settings (schemaMode != "managed")'); export type ExternalDatasourceSettings = z.infer; @@ -167,7 +424,7 @@ export const DatasourceSchema = lazySchema(() => z.object({ max: z.number().default(10).describe('Maximum connections'), idleTimeoutMillis: z.number().default(30000).describe('Idle timeout'), connectionTimeoutMillis: z.number().default(3000).describe('Connection establishment timeout'), - }).optional().describe('Connection pool settings'), + }, { error: poolUnknownKeyError }).strict().optional().describe('Connection pool settings'), /** * Read Replicas @@ -187,7 +444,7 @@ export const DatasourceSchema = lazySchema(() => z.object({ enabled: z.boolean().default(true).describe('Enable health check endpoint'), intervalMs: z.number().default(30000).describe('Health check interval in milliseconds'), timeoutMs: z.number().default(5000).describe('Health check timeout in milliseconds'), - }).optional().describe('Datasource health check configuration'), + }, { error: healthCheckUnknownKeyError }).strict().optional().describe('Datasource health check configuration'), /** SSL/TLS Configuration */ ssl: z.object({ @@ -196,7 +453,7 @@ export const DatasourceSchema = lazySchema(() => z.object({ ca: z.string().optional().describe('CA certificate (PEM format or path to file)'), cert: z.string().optional().describe('Client certificate (PEM format or path to file)'), key: z.string().optional().describe('Client private key (PEM format or path to file)'), - }).optional().describe('SSL/TLS configuration for secure database connections'), + }, { error: sslUnknownKeyError }).strict().optional().describe('SSL/TLS configuration for secure database connections'), /** Retry Policy */ retryPolicy: z.object({ @@ -204,7 +461,7 @@ export const DatasourceSchema = lazySchema(() => z.object({ baseDelayMs: z.number().default(1000).describe('Base delay between retries in milliseconds'), maxDelayMs: z.number().default(30000).describe('Maximum delay between retries in milliseconds'), backoffMultiplier: z.number().default(2).describe('Exponential backoff multiplier'), - }).optional().describe('Connection retry policy for transient failures'), + }, { error: datasourceRetryPolicyUnknownKeyError }).strict().optional().describe('Connection retry policy for transient failures'), /** Description */ description: z.string().optional().describe('Internal description'), @@ -251,7 +508,7 @@ export const DatasourceSchema = lazySchema(() => z.object({ */ origin: z.enum(['code', 'runtime']).default('code') .describe('Datasource provenance (server-managed, read-only)'), -}).superRefine((ds, ctx) => { +}, { error: datasourceUnknownKeyError }).strict().superRefine((ds, ctx) => { if (ds.schemaMode !== 'managed' && !ds.external) { ctx.addIssue({ code: 'custom', diff --git a/packages/spec/src/data/hook-body.zod.ts b/packages/spec/src/data/hook-body.zod.ts index 9374851319..37a1d3bab4 100644 --- a/packages/spec/src/data/hook-body.zod.ts +++ b/packages/spec/src/data/hook-body.zod.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { z } from 'zod'; +import { strictUnknownKeyError } from '../shared/suggestions.zod'; /** * Capability tokens a script body may request. @@ -31,6 +32,74 @@ export const HookBodyCapability = z.enum([ ]); export type HookBodyCapability = z.infer; +/* + * ── Unknown-key strictness (#4001 data step) ──────────────────────────────── + * + * Both body shapes are `.strict()`. A hook body is the one place where a + * silently-stripped key is worst-understood by the author: the body still + * parses, still runs, and the thing they configured simply is not in effect. + * + * The two concrete traps this closes: + * - `capabilities` misspelt on an L2 body strips to the `[]` default, and the + * sandbox then throws at INVOCATION time on the first `ctx.api` call — far + * from the typo, and only on the code path that happens to use it. + * - `timeoutMs` / `memoryMb` misspelt strip to undefined, so the body runs + * under the enclosing hook's limits instead of the tighter ones the author + * wrote. Nothing reports the downgrade. + * + * The L1 schema additionally PRESCRIBES the L2-only keys rather than guessing a + * rename: `capabilities` on an `expression` body is not a typo, it is a + * misunderstanding of which level owns the key. + * + * Placement note: this block sits AFTER `HookBodyCapability` on purpose. + * build-docs.ts takes the file's FIRST JSDoc as the reference page's module + * blurb, so a `/** … *\/` comment above the capability enum silently replaced + * the whole capability-token table in content/docs/references/data/hook-body.mdx. + * Keep declarations that carry JSDoc below the first exported symbol here. + */ + +/** Keys {@link ExpressionBodySchema} declares (drift-guarded by hook-body.test.ts). */ +const EXPRESSION_BODY_KEYS = ['language', 'source'] as const; + +/** Keys {@link ScriptBodySchema} declares (drift-guarded by hook-body.test.ts). */ +const SCRIPT_BODY_KEYS = ['language', 'source', 'capabilities', 'timeoutMs', 'memoryMb'] as const; + +const L2_ONLY_ON_L1 = + 'is an L2 key — it only applies to `language: "js"`. An expression body is a pure ' + + 'formula: it performs no IO, so it has nothing to grant and no sandbox to bound.'; + +const expressionBodyUnknownKeyError = strictUnknownKeyError({ + surface: 'this expression (L1) hook body', + knownKeys: EXPRESSION_BODY_KEYS, + aliases: { expression: 'source', formula: 'source', code: 'source', script: 'source' }, + guidance: { + capabilities: `\`capabilities\` ${L2_ONLY_ON_L1}`, + timeoutMs: `\`timeoutMs\` ${L2_ONLY_ON_L1}`, + memoryMb: `\`memoryMb\` ${L2_ONLY_ON_L1}`, + }, + history: 'Until #4001 these were dropped silently.', +}); + +const scriptBodyUnknownKeyError = strictUnknownKeyError({ + surface: 'this sandboxed JS (L2) hook body', + knownKeys: SCRIPT_BODY_KEYS, + aliases: { + capability: 'capabilities', + caps: 'capabilities', + permissions: 'capabilities', + timeout: 'timeoutMs', + timeoutms: 'timeoutMs', + memory: 'memoryMb', + memorymb: 'memoryMb', + code: 'source', + script: 'source', + body: 'source', + }, + history: + 'Until #4001 these were dropped silently — the body still ran, just not under the ' + + 'limits or grants that were written.', +}); + /** * L1 — Pure expression body. * @@ -46,7 +115,7 @@ export const ExpressionBodySchema = z.object({ language: z.literal('expression'), /** Formula-engine expression. Pure, side-effect-free. */ source: z.string().min(1).describe('Formula expression source'), -}).describe('L1 expression body — pure formula, no IO'); +}, { error: expressionBodyUnknownKeyError }).strict().describe('L1 expression body — pure formula, no IO'); export type ExpressionBody = z.infer; /** @@ -100,7 +169,7 @@ export const ScriptBodySchema = z.object({ * Subject to engine support (isolated-vm enforces, quickjs approximates). */ memoryMb: z.number().int().positive().max(256).optional().describe('Per-invocation memory cap (MB)'), -}).describe('L2 sandboxed JS body — runs inside an isolated VM with declared capabilities'); +}, { error: scriptBodyUnknownKeyError }).strict().describe('L2 sandboxed JS body — runs inside an isolated VM with declared capabilities'); export type ScriptBody = z.infer; /** diff --git a/packages/spec/src/data/hook.zod.ts b/packages/spec/src/data/hook.zod.ts index ca29b6bef6..efd35b9317 100644 --- a/packages/spec/src/data/hook.zod.ts +++ b/packages/spec/src/data/hook.zod.ts @@ -8,7 +8,88 @@ import { ExpressionInputSchema } from '../shared/expression.zod'; * Defines the interception points in the ObjectQL execution pipeline. */ import { lazySchema } from '../shared/lazy-schema'; +import { strictUnknownKeyError } from '../shared/suggestions.zod'; import { HookBodySchema } from './hook-body.zod'; + +/* + * ── Unknown-key strictness (#4001 data step) ──────────────────────────────── + * + * {@link HookSchema} (the AUTHORING shape) and its `retryPolicy` block are + * `.strict()`. `hook` is a registered metadata type — it sits in + * BUILTIN_METADATA_TYPE_SCHEMAS, so the same shape backs `defineStack({ hooks })` + * parsing, the `/api/v1/meta/types/hook` endpoint, and the Studio form. A key it + * silently dropped was invisible on every one of those surfaces. + * + * {@link HookContextSchema} is deliberately NOT strict, and must not become so. + * It is the RUNTIME shape the engine hands to a handler — nobody authors it. + * Strictness there would invert the contract: adding a field to the context + * (as `provenance` was in #3712) would turn an engine-internal enrichment into + * a breaking change for anyone parsing a context they were handed. Same + * reasoning as `RLSUserContextSchema` / `FlowVersionHistorySchema`. + * + * Two near-miss key names, both now aliased because they are genuinely easy to + * cross: + * - hook-level `timeout` vs body-level `timeoutMs` (see hook-body.zod.ts) + * - hook `retryPolicy.backoffMs` vs datasource `retryPolicy.baseDelayMs` + */ + +/** Keys {@link HookSchema} declares (drift-guarded by hook.test.ts). */ +const HOOK_KEYS = [ + 'name', 'label', 'object', 'events', 'handler', 'body', 'priority', + 'async', 'condition', 'description', 'retryPolicy', 'timeout', 'onError', +] as const; + +/** Keys the hook `retryPolicy` block declares (drift-guarded by hook.test.ts). */ +const HOOK_RETRY_POLICY_KEYS = ['maxRetries', 'backoffMs'] as const; + +const hookUnknownKeyError = strictUnknownKeyError({ + surface: 'this hook', + knownKeys: HOOK_KEYS, + aliases: { + hookname: 'name', + objectname: 'object', + objects: 'object', + event: 'events', + fn: 'handler', + callback: 'handler', + order: 'priority', + sequence: 'priority', + background: 'async', + isasync: 'async', + when: 'condition', + predicate: 'condition', + retry: 'retryPolicy', + timeoutms: 'timeout', + errorpolicy: 'onError', + onfailure: 'onError', + }, + guidance: { + enabled: + '`enabled` is not a hook key — a hook has no on/off switch. Gate it with `condition` ' + + '(the hook is skipped when the predicate is false), or remove the hook.', + active: + '`active` is not a hook key — a hook has no on/off switch. Gate it with `condition`, ' + + 'or remove the hook.', + }, + history: 'Until #4001 these were dropped silently — the hook still registered and ran.', +}); + +const hookRetryPolicyUnknownKeyError = strictUnknownKeyError({ + surface: "this hook's retryPolicy", + knownKeys: HOOK_RETRY_POLICY_KEYS, + aliases: { + retries: 'maxRetries', + attempts: 'maxRetries', + basedelayms: 'backoffMs', + backoff: 'backoffMs', + delayms: 'backoffMs', + }, + history: + 'Until #4001 these were dropped silently — the hook retried on the defaults rather ' + + 'than the policy that was written. Note a datasource retryPolicy spells its delay ' + + '`baseDelayMs`; a hook spells it `backoffMs`.', +}); + export const HookEvent = z.enum([ // Read — one event per read, regardless of shape. `beforeFind`/`afterFind` // fire for BOTH `find` and `findOne` (the event attaches to record @@ -141,7 +222,7 @@ export const HookSchema = lazySchema(() => z.object({ retryPolicy: z.object({ maxRetries: z.number().default(3).describe('Maximum retry attempts on failure'), backoffMs: z.number().default(1000).describe('Backoff delay between retries in milliseconds'), - }).optional().describe('Retry policy for failed hook executions'), + }, { error: hookRetryPolicyUnknownKeyError }).strict().optional().describe('Retry policy for failed hook executions'), /** * Execution Timeout @@ -155,7 +236,7 @@ export const HookSchema = lazySchema(() => z.object({ * - log: Log error and continue */ onError: z.enum(['abort', 'log']).default('abort').describe('Error handling strategy'), -})); +}, { error: hookUnknownKeyError }).strict()); /** * Hook Runtime Context diff --git a/packages/spec/src/kernel/metadata-authoring-lint.test.ts b/packages/spec/src/kernel/metadata-authoring-lint.test.ts index ecadf24256..4b7033f490 100644 --- a/packages/spec/src/kernel/metadata-authoring-lint.test.ts +++ b/packages/spec/src/kernel/metadata-authoring-lint.test.ts @@ -27,11 +27,11 @@ describe('coverage derivation (#3786 — no third hand-written list)', () => { // #4148 covered object+field: 2 surfaces. The point of this walker is the // rest. If the derivation regresses to a handful, the "evidence base" for // the #4001 strict tiers quietly becomes a sample again. - expect(lintables.length).toBeGreaterThanOrEqual(15); + expect(lintables.length).toBeGreaterThanOrEqual(14); // `view` matters doubly: it is a UNION (container | ViewItem | overlay), so // its presence pins the union half of the posture logic — a regression that // silently dropped unions would shrink coverage without failing the count. - for (const expected of ['object', 'page', 'agent', 'dashboard', 'action', 'report', 'hook', 'view']) { + for (const expected of ['object', 'page', 'agent', 'dashboard', 'action', 'report', 'view']) { expect(lintableTypes, `expected '${expected}' to be lint-covered`).toContain(expected); } }); @@ -42,7 +42,11 @@ describe('coverage derivation (#3786 — no third hand-written list)', () => { // `app` graduated mid-flight (#4165) while this very test was in review: // the derivation adapted on its own, and the pinned expectation above is // what forced a human to confirm the shrink was a graduation, not a bug. - for (const strict of ['flow', 'permission', 'position', 'tool', 'app']) { + // It did that job again for `hook` + `datasource` (#4001 data step): the + // count fell 16 → 14 and the pin above failed until both were confirmed + // graduations and moved into this list. `hook` also had to leave the + // pinned-coverage list above, where it had been an expected lint target. + for (const strict of ['flow', 'permission', 'position', 'tool', 'app', 'hook', 'datasource']) { expect(lintableTypes, `'${strict}' is .strict(); the lint must not double-report`).not.toContain(strict); } });