diff --git a/.changeset/flow-executors-parse-config.md b/.changeset/flow-executors-parse-config.md new file mode 100644 index 0000000000..2d53f79ee9 --- /dev/null +++ b/.changeset/flow-executors-parse-config.md @@ -0,0 +1,55 @@ +--- +"@objectstack/service-automation": minor +"@objectstack/spec": minor +--- + +feat(automation,spec): flow executors `parse()` their config, and undeclared config keys reject at registration (#4277) + +The #4045 reconciliation left every flat builtin with a Zod config contract that +nothing enforced, and #4059 left `registerFlow` warning about undeclared keys it +could not yet safely reject. #4277 installs both halves of the enforcement: + +**1. Executors parse their config (execute time).** The 12 contract-carrying +builtins — `get_record` / `create_record` / `update_record` / `delete_record`, +`screen`, `map`, `notify`, `http`, `loop` / `parallel` / `try_catch` — now run +`node.config` through their Zod contract before executing +(`service-automation/builtin/parse-config.ts`). A type or missing-`required` +violation refuses the node as a **guard** (`errorClass: 'guard'`, not routable +via `fault` edges — config is metadata; re-running changes nothing), naming +every violated path. `{token}` templates stay legal: string-typed slots parse +the raw template, and `http` — whose executor reads the interpolated config — +parses POST-interpolation, where a whole-token template has already resolved to +its value's real type. Exemption: a legacy flat-graph `loop` (no `config.body`) +predates the ADR-0031 construct and is not parsed. + +**2. Undeclared config keys are rejected at `registerFlow` (registration +time).** The #4059 warning is now an error: a config key the node type's +descriptor `configSchema` does not declare fails registration, with the exact +path, the declared key set, a did-you-mean, and — for keys with documented +history (`screen.visibleIf`, `create_record`/`update_record.fieldValues`) — a +per-key tombstone (the `UNKNOWN_KEY_GUIDANCE` pattern). Unchanged exemptions: +`assignment` is exempt wholesale (its top-level keys ARE the author's variable +names), schemaless types (`decision`/`script`/`wait`/`subflow`/ +`connector_action`) declare nothing so nothing can be undeclared, and keyValue +maps stop the walk (their keys are author data). Every `registerFlow` call site +already try/catches per flow, so a bad stored flow is skipped loudly at boot, +never a crashed kernel. + +**Contract fix folded in:** `LoopConfigSchema.collection` is now +`z.union([z.string().min(1), z.array(z.unknown())])` — the executor has always +accepted an inline array (shared resolve logic with `map.collection`, which +already declared the union), so the string-only declaration under-declared what +it reads. + +**Migration.** If a flow stops registering: the error names the undeclared key +and its path — rename it to the declared key it meant (`visibleIf` → +`visibleWhen`, `fieldValues` → `fields`), or delete it (an undeclared key was +never read, so removing it changes no behavior). If an executor of yours +genuinely reads the key, declare it on the node type's descriptor +`configSchema`. If a node starts refusing at run time: the refusal names each +violated path against the contract — fix the value's type or supply the missing +required key (e.g. `get_record` `limit` must be a number; `screen` +`fields[].options` entries are `{ value, label }` objects; `notify` requires +`recipients` + `title`). Retry-policy defaults now come from the contract: a +`try_catch` `retry` block that omits `retryDelayMs` gets the documented 1000ms +base delay where the executor historically used 0. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index c259753389..ce7c943be3 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -138,7 +138,7 @@ Each node performs a specific action in the flow. | `id` | `string` | ✅ | Unique node identifier | | `type` | `string` | ✅ | Node type — a built-in id from the table above **or** a plugin-registered one. Per ADR-0018 the spec does not gate this with a closed enum; it is checked against the live action registry at `registerFlow()` | | `label` | `string` | ✅ | Display label | -| `config` | `object` | optional | Type-specific configuration (an open record — the registered executor's `configSchema` owns its shape) | +| `config` | `object` | optional | Type-specific configuration — the registered executor's `configSchema` owns its shape. Keys that schema does not declare are rejected at `registerFlow()`, and the built-in executors `parse()` the value against their Zod contract before running (#4277) | | `connectorConfig` | `object` | optional | `{ connectorId, actionId, input }` for a `connector_action` node | | `position` | `{ x, y }` | optional | Visual position on canvas | | `timeoutMs` | `number` | optional | Per-node execution timeout | @@ -148,8 +148,14 @@ Each node performs a specific action in the flow. The flow, node, edge, and variable **shells are `.strict()`** — a key they do not -declare is a parse error naming the likely intended key, not a silent strip. (A -node's `config` stays open, so plugin node types keep their own vocabulary.) +declare is a parse error naming the likely intended key, not a silent strip. A +node's `config` is open at the flow-parse level (plugin node types keep their own +vocabulary), but it is not unchecked: `registerFlow()` rejects config keys the +node type's published `configSchema` does not declare — naming the path, the +declared key set, and a did-you-mean — and the contract-carrying builtins +additionally `parse()` their config at execute time, refusing the node on a type +or missing-`required` violation (#4277). A node type that publishes no +`configSchema` declares nothing, so nothing can be undeclared. ### Node Examples diff --git a/content/docs/references/automation/builtin-node-config.mdx b/content/docs/references/automation/builtin-node-config.mdx index 3a599486c9..b332023987 100644 --- a/content/docs/references/automation/builtin-node-config.mdx +++ b/content/docs/references/automation/builtin-node-config.mdx @@ -43,15 +43,21 @@ field-item keys `options`/`defaultValue`/`placeholder`, `map.indexVariable`, ADR-0087 D2 conversion layer like `notify.source` before it. -## What these schemas are (and are not) wired to +## What these schemas are wired to (#4277) -Contract exports only — no engine path `parse()`s a node config with them +Live execute-time contracts: each executor `parse()`s its config against -today, so registering a flow behaves exactly as before. Wiring the executors +its schema before running (`service-automation`'s `parse-config.ts`), so -to parse is deliberately deferred until the #4059 undeclared-key warning has +type and `required` violations refuse the node as a guard. All of these -measured a release's worth of real metadata (#4045 step 3b). +parse the RAW stored config — their typed slots are strings (or `unknown` + +where values interpolate), so `\{token\}` templates pass and resolve at the + +executor's existing interpolation points. Unknown keys are rejected earlier, + +at `registerFlow()` (the tightened #4059 check); the parse here strips them. Deliberately absent: diff --git a/content/docs/references/automation/control-flow.mdx b/content/docs/references/automation/control-flow.mdx index f28a6e9968..4b3599b19b 100644 --- a/content/docs/references/automation/control-flow.mdx +++ b/content/docs/references/automation/control-flow.mdx @@ -107,7 +107,7 @@ const result = FlowRegion.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **collection** | `string` | ✅ | Template/variable resolving to the array to iterate | +| **collection** | `string \| any[]` | ✅ | Template/variable resolving to the array to iterate (an inline array is accepted) | | **iteratorVariable** | `string` | optional | Loop variable holding the current item | | **indexVariable** | `string` | optional | Optional loop variable holding the current index | | **maxIterations** | `integer` | optional | Hard cap on iterations (clamped to the engine ceiling) | diff --git a/content/docs/references/automation/io-node-config.mdx b/content/docs/references/automation/io-node-config.mdx index e595a6b915..2baee1d136 100644 --- a/content/docs/references/automation/io-node-config.mdx +++ b/content/docs/references/automation/io-node-config.mdx @@ -25,21 +25,31 @@ and a Zod copied from the form would make that reconciliation a tautology — it would pass by construction and prove nothing (#4045). -## What these schemas are (and are not) wired to +## What these schemas are wired to (#4277) Like `LoopConfigSchema` / `ParallelConfigSchema` / `TryCatchConfigSchema`, -these are **contract exports**: no engine path `parse()`s a node config with +these are **live execute-time contracts**: each executor `parse()`s its -them today, so registering a flow behaves exactly as before. Wiring the +config against its schema before running (`service-automation`'s -executors to parse — which would finally give node configs the type / +`parse-config.ts`), so type and `required` violations refuse the node as a -`required` / unknown-key enforcement the descriptor `configSchema` never +guard (not routable via `fault` edges). `notify` parses the RAW stored -provided — is deliberately deferred until the #4059 undeclared-key warning +config — its slots are string-typed, so `\{token\}` templates pass and the -has measured a release's worth of real metadata (#4045 step 3b). +post-interpolation guards still own "resolved to nothing". `http` parses + +the INTERPOLATED config, because that is the shape its executor reads — + +a `\{token\}` in a typed slot (`timeoutMs`, `durable`) resolves to its real + +type first. Unknown keys are the registration layer's job: `registerFlow()` + +rejects keys the descriptor `configSchema` does not declare (the tightened + +#4059 check), while the parse here strips them. `connector_action` has no schema here on purpose: its config contract is diff --git a/content/docs/references/automation/node-executor.mdx b/content/docs/references/automation/node-executor.mdx index e87cc1b918..d4b5cb40d7 100644 --- a/content/docs/references/automation/node-executor.mdx +++ b/content/docs/references/automation/node-executor.mdx @@ -70,7 +70,7 @@ Canonical cross-paradigm action/node descriptor (ADR-0018) | **icon** | `string` | optional | Icon id resolved by the designer | | **category** | `Enum<'logic' \| 'data' \| 'io' \| 'human' \| 'control' \| 'custom'>` | ✅ | Palette category | | **paradigms** | `Enum<'flow' \| 'approval'>[]` | ✅ | Authoring surfaces that may offer this action | -| **configSchema** | `any` | optional | JSON Schema for the node config (drives the designer form; only declared expression slots are validated) | +| **configSchema** | `any` | optional | JSON Schema for the node config (drives the designer form; undeclared keys are rejected at registration) | | **supportsPause** | `boolean` | ✅ | Supports async pause/resume | | **supportsCancellation** | `boolean` | ✅ | Supports cancellation | | **supportsRetry** | `boolean` | ✅ | Supports retry on failure | diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index 0a277bd011..5b5d6982eb 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -1384,7 +1384,10 @@ export const InquiryPurgeFlow = defineFlow({ id: 'report', type: 'notify', label: 'Report cleanup', - config: { topic: 'inquiry_purge', message: 'Closed inquiries purged.' }, + // `recipients` + `title` are the notify contract's required pair — this + // node used to omit both, so every purge run FAILED here at execute time + // (found when the executors started parsing their config, #4277). + config: { topic: 'inquiry_purge', recipients: ['admin@objectos.ai'], title: 'Closed inquiries purged', message: 'Closed inquiries purged.' }, }, { id: 'end', type: 'end', label: 'End' }, ], @@ -1538,6 +1541,10 @@ export const TaskDueReminderFlow = defineFlow({ label: 'Remind Owner', config: { topic: 'task.due_soon', + // The contract's required audience — this node used to omit it, so + // every sweep run FAILED here ("at least one recipient is required") + // and the #1874 demo never delivered (surfaced by #4277's parse). + recipients: ['{record.assignee}'], channels: ['inbox'], severity: 'warning', title: 'Task due soon: {record.title}', diff --git a/examples/app-todo/src/flows/task.flow.ts b/examples/app-todo/src/flows/task.flow.ts index be0abc10e4..9f8cb0e69d 100644 --- a/examples/app-todo/src/flows/task.flow.ts +++ b/examples/app-todo/src/flows/task.flow.ts @@ -20,7 +20,10 @@ export const TaskReminderFlow: Flow = { { id: 'start', type: 'start', label: 'Start (Daily 8 AM)', config: { schedule: '0 8 * * *', objectName: 'todo_task' } }, { id: 'get_upcoming_tasks', type: 'get_record', label: 'Get Tasks Due Tomorrow', - config: { objectName: 'todo_task', filter: { due_date: '{tomorrow}', is_completed: false }, outputVariable: 'tasksToRemind', getAll: true }, + // `limit > 1` is the declared way to make this a LIST read (`find`, not + // `findOne`) — the undeclared `getAll` that sat here was never read, so + // this sweep silently fetched a single task (#4277 rejects the key now). + config: { objectName: 'todo_task', filter: { due_date: '{tomorrow}', is_completed: false }, outputVariable: 'tasksToRemind', limit: 200 }, }, { id: 'loop_tasks', type: 'loop', label: 'Loop Through Tasks', @@ -67,10 +70,11 @@ export const OverdueEscalationFlow: Flow = { { id: 'start', type: 'start', label: 'Start (Daily 9 AM)', config: { schedule: '0 9 * * *', objectName: 'todo_task' } }, { id: 'get_overdue_tasks', type: 'get_record', label: 'Get Severely Overdue Tasks', + // `limit > 1` = LIST read; the undeclared `getAll` was never read (#4277). config: { objectName: 'todo_task', filter: { due_date: { $lt: '{3_days_ago}' }, is_completed: false, is_overdue: true }, - outputVariable: 'overdueTasks', getAll: true, + outputVariable: 'overdueTasks', limit: 200, }, }, { @@ -178,11 +182,31 @@ export const QuickAddTaskFlow: Flow = { { id: 'screen_1', type: 'screen', label: 'Task Details', config: { + // Select options are `{ value, label }` pairs (the screen contract's + // shape — bare strings fail the executor's config parse, #4277). fields: [ { name: 'subject', label: 'Task Subject', type: 'text', required: true }, - { name: 'priority', label: 'Priority', type: 'select', options: ['low', 'normal', 'high', 'urgent'], defaultValue: 'normal' }, + { + name: 'priority', label: 'Priority', type: 'select', defaultValue: 'normal', + options: [ + { value: 'low', label: 'Low' }, + { value: 'normal', label: 'Normal' }, + { value: 'high', label: 'High' }, + { value: 'urgent', label: 'Urgent' }, + ], + }, { name: 'dueDate', label: 'Due Date', type: 'date', required: false }, - { name: 'category', label: 'Category', type: 'select', options: ['personal', 'work', 'shopping', 'health', 'finance', 'other'] }, + { + name: 'category', label: 'Category', type: 'select', + options: [ + { value: 'personal', label: 'Personal' }, + { value: 'work', label: 'Work' }, + { value: 'shopping', label: 'Shopping' }, + { value: 'health', label: 'Health' }, + { value: 'finance', label: 'Finance' }, + { value: 'other', label: 'Other' }, + ], + }, ], }, }, @@ -196,13 +220,14 @@ export const QuickAddTaskFlow: Flow = { }, { id: 'success_screen', type: 'screen', label: 'Success', + // The screen contract has no `message`/`buttons` keys — nothing ever + // read them, so this "success screen" was an invisible pass-through + // (#4277 rejects the keys now). `description` + `waitForInput: true` + // is the declared way to pause on a message-only confirmation screen. config: { - message: 'Task "{subject}" created successfully!', - buttons: [ - { label: 'Create Another', action: 'restart' }, - { label: 'View Task', action: 'navigate', target: '/task/{newTaskId.id}' }, - { label: 'Done', action: 'finish' }, - ], + title: 'Task Created', + description: 'Task "{subject}" created successfully!', + waitForInput: true, }, }, { id: 'end', type: 'end', label: 'End' }, diff --git a/packages/services/service-automation/src/builtin/config-parse.test.ts b/packages/services/service-automation/src/builtin/config-parse.test.ts new file mode 100644 index 0000000000..0b7bc74316 --- /dev/null +++ b/packages/services/service-automation/src/builtin/config-parse.test.ts @@ -0,0 +1,207 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Execute-time config `parse()` for the contract-carrying builtins (#4277 — + * the enforcement half #4045 deliberately deferred). + * + * Until #4277 the Zod contracts in `@objectstack/spec/automation` were pure + * exports: nothing parsed a node config with them, so a wrong-typed or + * missing-required key sailed through to whatever the executor's loose reads + * made of it (`limit: "10"` silently ignored, `mode: "view"` silently treated + * as create). These tests pin the tightened behavior: + * + * - a config that violates its contract REFUSES the node as a **guard** — + * the failure is not routable via `fault` edges (a config is metadata; + * re-running changes nothing), and the message names every violated path; + * - `{token}` templates stay legal everywhere they were: string slots parse + * raw templates, and `http` — which reads its config interpolated — parses + * the POST-interpolation shape, where a whole-token template has already + * resolved to its value's real type; + * - the deliberate exemption: a legacy flat-graph `loop` (no `config.body`) + * predates the ADR-0031 construct and is not parsed. + */ + +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; + +function silentLogger() { + const logger: any = { + info() {}, warn() {}, error() {}, debug() {}, + child() { return logger; }, + }; + return logger; +} + +interface HttpSurface { + isHttpDeliveryReady?(): boolean; + enqueueHttp?(input: any): Promise; +} + +function engineWith(messaging?: HttpSurface) { + const engine = new AutomationEngine(silentLogger()); + const ctx: any = { + logger: silentLogger(), + getService(name: string) { + if (name === 'messaging' && messaging) return messaging; + throw new Error('none'); + }, + }; + installBuiltinNodes(engine, ctx); + return engine; +} + +/** start → n1(type, config) → end, with optional extra nodes/edges. */ +function flowWith( + type: string, + config: Record, + extra?: { nodes?: any[]; edges?: any[]; variables?: any[] }, +) { + return { + name: 'f', label: 'F', type: 'autolaunched', status: 'active', version: 1, + variables: extra?.variables ?? [], + nodes: [ + { id: 'start', type: 'start', label: 'S', config: {} }, + { id: 'n1', type, label: 'N', config }, + { id: 'end', type: 'end', label: 'E' }, + ...(extra?.nodes ?? []), + ], + edges: [ + { id: 'e1', source: 'start', target: 'n1', type: 'default' }, + { id: 'e2', source: 'n1', target: 'end', type: 'default' }, + ...(extra?.edges ?? []), + ], + }; +} + +describe('execute-time config parse (#4277)', () => { + it('refuses a wrong-typed declared key, naming the exact path', async () => { + const engine = engineWith(); + // `limit` is declared (so registration accepts it) but must be a number — + // the executor never honored a string here, so this was dead config that + // now fails loudly instead of silently doing nothing. + engine.registerFlow('f', flowWith('get_record', { objectName: 'crm_lead', limit: 'ten' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('get_record'); + expect(result.error).toContain('config.limit'); + expect(result.error).toContain('does not satisfy the get_record contract'); + }); + + it('a parse refusal is a guard — a fault edge does NOT route it', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith( + 'get_record', + { objectName: 'crm_lead', limit: 'ten' }, + { + nodes: [{ id: 'recover', type: 'assignment', label: 'R', config: { recovered: true } }], + edges: [{ id: 'e3', source: 'n1', target: 'recover', type: 'fault' }], + }, + )); + + const result = await engine.execute('f'); + // Routable would mean success-via-recovery; a guard stays fatal (#3863). + expect(result.success).toBe(false); + expect(result.error).toContain('does not satisfy the get_record contract'); + }); + + it('refuses a missing required key (notify without title)', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('notify', { recipients: 'u1' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('notify'); + expect(result.error).toContain('config.title'); + }); + + it('string slots parse RAW templates — a `{token}` recipients/title passes', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('notify', { + recipients: '{who}', title: 'Hi {who}', + }, { variables: [{ name: 'who', type: 'text', isInput: true }] })); + + // No messaging service wired → the node degrades to a skipped success; + // what matters here is that the parse accepted the template strings. + const result = await engine.execute('f', { params: { who: 'u1' } } as any); + expect(result.success).toBe(true); + }); + + it('http parses the INTERPOLATED config — a whole-token template in a typed slot resolves first', async () => { + const enqueued: any[] = []; + const engine = engineWith({ + isHttpDeliveryReady: () => true, + async enqueueHttp(input) { enqueued.push(input); return 'dlv_1'; }, + }); + engine.registerFlow('f', flowWith('http', { + url: 'https://example.test/hook', durable: true, timeoutMs: '{t}', + }, { variables: [{ name: 't', type: 'number', isInput: true }] })); + + const result = await engine.execute('f', { params: { t: 1500 } } as any); + expect(result.success).toBe(true); + // Whole-token interpolation preserved the number, so the contract's + // `timeoutMs: z.number()` saw 1500, not a string. + expect(enqueued[0]?.timeoutMs).toBe(1500); + }); + + it('http still refuses a statically wrong-typed slot', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('http', { url: 'https://example.test', timeoutMs: 'soon' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('config.timeoutMs'); + }); + + it('screen refuses an out-of-enum mode instead of silently treating it as create', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('screen', { objectName: 'crm_lead', mode: 'view' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('config.mode'); + }); + + it('legacy flat-graph loop (no body) is exempt — an empty config still falls through', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('loop', {})); + + const result = await engine.execute('f'); + expect(result.success).toBe(true); + }); + + it('a structured loop (body present) IS parsed — missing collection refuses', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('loop', { + body: { nodes: [{ id: 'b1', type: 'assignment', label: 'B', config: { x: 1 } }], edges: [] }, + })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('loop'); + expect(result.error).toContain('config.collection'); + }); + + it('parallel refuses a non-array branches via the contract', async () => { + const engine = engineWith(); + // A string `branches` slips past registration (validateControlFlow only + // inspects arrays) — the execute-time parse is what catches it now. + engine.registerFlow('f', flowWith('parallel', { branches: 'oops' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('parallel'); + expect(result.error).toContain('config.branches'); + }); + + it('map refuses a missing collection, naming the path', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('map', { flowName: 'child' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('map'); + expect(result.error).toContain('config.collection'); + }); +}); diff --git a/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts b/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts index 9b1591d5d7..97f63f8ff3 100644 --- a/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts +++ b/packages/services/service-automation/src/builtin/config-unknown-keys.test.ts @@ -1,17 +1,22 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * Unknown flow-node `config` keys are reported at registration (#4045). + * Unknown flow-node `config` keys are REJECTED at registration (#4277 — the + * error half of the #4045 unknown-key ladder; #4059 shipped the warn half). * - * `FlowNodeSchema.config` is `z.record(z.unknown())`, so before this a misspelled - * or invented config key was accepted in **total silence**: `visibleIf` instead of - * `visibleWhen` registered cleanly, was never read, and the only symptom was a - * feature that quietly did not happen. That is the diagnostic vacuum that made - * #3528 take three passes and two wrong diagnoses. + * `FlowNodeSchema.config` is `z.record(z.unknown())`, so before #4059 a + * misspelled or invented config key was accepted in **total silence**: + * `visibleIf` instead of `visibleWhen` registered cleanly, was never read, and + * the only symptom was a feature that quietly did not happen. That is the + * diagnostic vacuum that made #3528 take three passes and two wrong diagnoses. * - * Warn, never reject — see `validateNodeConfigKeys` for why. These tests pin both - * halves of that: the warning fires with an actionable suggestion, AND - * registration still succeeds. + * #4059 turned that into a warning while the #4045 reconciliation closed the + * read-but-undeclared population; #4277 tightens it into a rejection. These + * tests pin both halves of the tightened behavior: registration THROWS with an + * actionable prescription (path, declared set, did-you-mean, per-key + * tombstones), AND the deliberate exemptions still register — `assignment` + * (author-named keys), keyValue-map keys (author data), schemaless types + * (nothing declared ⇒ nothing undeclared). */ import { describe, it, expect } from 'vitest'; @@ -50,77 +55,90 @@ function flowWith(type: string, config: Record) { }; } -describe('unknown node config keys (#4045)', () => { - it('flags a misspelled key with a did-you-mean suggestion', () => { - const { logger, warnings } = recordingLogger(); +/** Register and return the rejection message (fails the test if it registers). */ +function rejectionOf(engine: AutomationEngine, type: string, config: Record): string { + try { + engine.registerFlow('f', flowWith(type, config)); + } catch (err) { + return (err as Error).message; + } + throw new Error(`flow with ${type} config ${JSON.stringify(config)} should have been rejected`); +} + +describe('unknown node config keys are rejected (#4277)', () => { + it('rejects a misspelled key inside the field repeater, locating the exact element', async () => { + const { logger } = recordingLogger(); const engine = engineWith(logger); // The typo lives INSIDE the field repeater — where the real #3528 one did. - engine.registerFlow('f', flowWith('screen', { + const msg = rejectionOf(engine, 'screen', { fields: [{ name: 'opportunityName', required: true, visibleIf: 'createOpportunity == true' }], - })); + }); - const hit = warnings.find((w) => w.includes('visibleIf')); - expect(hit, 'the typo should be reported').toBeDefined(); - expect(hit).toContain("node 'n1'"); - expect(hit).toContain('(screen)'); + expect(msg).toContain("node 'n1'"); + expect(msg).toContain('(screen)'); // Located to the exact element, so the author knows WHICH field. - expect(hit).toContain('config.fields[0].visibleIf'); + expect(msg).toContain('config.fields[0].visibleIf'); // The load-bearing half for an agent author: the correct key is named. Note // it comes from the DECLARED SET, not the suggestion — `visibleIf` → - // `visibleWhen` is edit-distance 4 against `nearestName`'s threshold of 3, so - // this exact typo gets no did-you-mean. Printing the declared set is what - // makes the diagnostic actionable regardless. - expect(hit).toContain( + // `visibleWhen` is edit-distance 4 against `nearestName`'s threshold of 3, + // so this exact typo gets no did-you-mean. Printing the declared set is + // what makes the diagnostic actionable regardless. + expect(msg).toContain( 'Declared here: name, label, type, required, options, defaultValue, placeholder, visibleWhen.', ); + // …and this particular key has a documented incident, so it also carries + // its tombstone (the UNKNOWN_KEY_GUIDANCE pattern). + expect(msg).toContain('visibleWhen'); + expect(msg).toContain('#3528'); + // The flow is NOT registered. + await expect(engine.getFlow('f')).resolves.toBeNull(); }); it('adds a did-you-mean when the typo IS within edit distance', () => { - const { logger, warnings } = recordingLogger(); + const { logger } = recordingLogger(); const engine = engineWith(logger); - engine.registerFlow('f', flowWith('screen', { titl: 'Details' })); - - const hit = warnings.find((w) => w.includes('titl')); - expect(hit).toContain('did you mean `title`?'); + const msg = rejectionOf(engine, 'screen', { titl: 'Details' }); + expect(msg).toContain('did you mean `title`?'); // …and still lists the declared set alongside it. - expect(hit).toContain('Declared here:'); + expect(msg).toContain('Declared here:'); }); - it('still registers the flow — warn, never reject', () => { + it('carries the fieldValues → fields tombstone on the CRUD write map', () => { const { logger } = recordingLogger(); const engine = engineWith(logger); - expect(() => engine.registerFlow('f', flowWith('screen', { visibleIf: 'a == true' }))).not.toThrow(); - // And it is really registered, not swallowed. - expect(engine.getFlow('f')).toBeDefined(); + + const msg = rejectionOf(engine, 'create_record', { + objectName: 'crm_lead', + fieldValues: { name: 'x' }, + }); + expect(msg).toContain('unknown config key `fieldValues`'); + // The tombstone names the mechanism, not just the nearest key. + expect(msg).toContain('The write map is `fields`'); }); - it('lists the declared keys when nothing is close enough to suggest', () => { - const { logger, warnings } = recordingLogger(); + it('reports every undeclared key in ONE rejection, not just the first', () => { + const { logger } = recordingLogger(); const engine = engineWith(logger); - engine.registerFlow('f', flowWith('screen', { totallyMadeUp: 42 })); - - const hit = warnings.find((w) => w.includes('totallyMadeUp')); - expect(hit).toBeDefined(); - expect(hit).not.toContain('did you mean'); - // The declared set is always present, so there is always somewhere to go. - expect(hit).toContain('Declared here:'); - expect(hit).toContain('fields'); + const msg = rejectionOf(engine, 'screen', { hideWhen: 'x', submitLabel: 'Go' }); + expect(msg).toContain('hideWhen'); + expect(msg).toContain('submitLabel'); + expect(msg).toContain('2 undeclared config key(s)'); }); - it('reports every undeclared key, not just the first', () => { - const { logger, warnings } = recordingLogger(); + it('the rejection carries the declare-it prescription for genuinely-read keys', () => { + const { logger } = recordingLogger(); const engine = engineWith(logger); - engine.registerFlow('f', flowWith('screen', { hideWhen: 'x', submitLabel: 'Go' })); - - expect(warnings.some((w) => w.includes('hideWhen'))).toBe(true); - expect(warnings.some((w) => w.includes('submitLabel'))).toBe(true); + const msg = rejectionOf(engine, 'screen', { totallyMadeUp: 42 }); + expect(msg).not.toContain('did you mean'); + // The way OUT for a custom executor whose schema lags its reads. + expect(msg).toContain("declare it on the node type's descriptor configSchema"); }); - it('stays quiet on a fully declared config', () => { + it('registers a fully declared config without complaint', async () => { const { logger, warnings } = recordingLogger(); const engine = engineWith(logger); @@ -131,31 +149,43 @@ describe('unknown node config keys (#4045)', () => { defaults: { x: 1 }, })); + await expect(engine.getFlow('f')).resolves.toBeDefined(); expect(warnings.filter((w) => w.includes('unknown config key'))).toEqual([]); }); + it('assignment is exempt WHOLESALE — its top-level keys are author variable names', async () => { + // Pinned as un-reconcilable by the form↔Zod ledger: with no `assignments` + // wrapper, the top-level config keys ARE the variables being assigned, so + // no fixed key set can describe them and the tightened check must not try. + const { logger } = recordingLogger(); + const engine = engineWith(logger); + + expect(() => engine.registerFlow('f', flowWith('assignment', { + approvalStatus: 'pending', + anyVariableNameAtAll: '{record.owner}', + }))).not.toThrow(); + await expect(engine.getFlow('f')).resolves.toBeDefined(); + }); + it('stays quiet for a node type that publishes no configSchema', () => { // `decision` / `script` are deliberately schemaless (config-schemas.test.ts): // nothing is declared, so nothing can be undeclared. - const { logger, warnings } = recordingLogger(); + const { logger } = recordingLogger(); const engine = engineWith(logger); - engine.registerFlow('f', flowWith('decision', { condition: 'a == b', whateverElse: 1 })); - - expect(warnings.filter((w) => w.includes('unknown config key'))).toEqual([]); + expect(() => engine.registerFlow('f', flowWith('decision', { condition: 'a == b', whateverElse: 1 }))) + .not.toThrow(); }); it('does not flag the keys of a keyValue map — those are author data', () => { // The walk descends where the schema declares structure and STOPS at a // free-form map: `filter: { status: 'stale' }` keys are data, not config keys. - const { logger, warnings } = recordingLogger(); + const { logger } = recordingLogger(); const engine = engineWith(logger); - engine.registerFlow('f', flowWith('get_record', { + expect(() => engine.registerFlow('f', flowWith('get_record', { objectName: 'crm_lead', filter: { status: 'stale', anythingAtAll: true }, - })); - - expect(warnings.filter((w) => w.includes('unknown config key'))).toEqual([]); + }))).not.toThrow(); }); }); diff --git a/packages/services/service-automation/src/builtin/crud-nodes.ts b/packages/services/service-automation/src/builtin/crud-nodes.ts index dda00b0184..b3cd596102 100644 --- a/packages/services/service-automation/src/builtin/crud-nodes.ts +++ b/packages/services/service-automation/src/builtin/crud-nodes.ts @@ -1,12 +1,25 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; +import { + defineActionDescriptor, + GetRecordConfigSchema, + CreateRecordConfigSchema, + UpdateRecordConfigSchema, + DeleteRecordConfigSchema, +} from '@objectstack/spec/automation'; +import type { + GetRecordConfigParsed, + CreateRecordConfigParsed, + UpdateRecordConfigParsed, + DeleteRecordConfigParsed, +} from '@objectstack/spec/automation'; import type { IDataEngine } from '@objectstack/spec/contracts'; import type { DroppedFieldsEvent } from '@objectstack/spec/data'; import type { AutomationEngine } from '../engine.js'; import { interpolate, interpolateFilter, type VariableMap } from './template.js'; import { refuseNode } from '../guard-refusal.js'; +import { parseNodeConfig } from './parse-config.js'; import { resolveRunDataContext } from '../runtime-identity.js'; /** @@ -166,12 +179,17 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; // `filters` → `filter` and `object` → `objectName` are handled at // load by the ADR-0087 D2 conversion layer ('flow-node-crud-filter-alias', - // 'flow-node-crud-object-alias'), so the executor reads the canonical - // keys directly (PD #12 fallbacks retired). - const objectName = String(cfg.objectName ?? ''); + // 'flow-node-crud-object-alias'), so the parse sees canonical keys + // (PD #12 fallbacks retired). Parsed BEFORE interpolation: every + // typed slot the contract declares beyond strings is read raw by + // this executor too (`limit` never honored a template), so a + // template in one was already dead config — now it is loud. + const parsed = parseNodeConfig('get_record', node.id, GetRecordConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const objectName = cfg.objectName; if (!objectName) return refuseNode('get_record: objectName required'); const filterResult = resolveNodeFilter( @@ -180,9 +198,9 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): ); if ('error' in filterResult) return refuseNode(filterResult.error); const filter = filterResult.filter; - const fields = cfg.fields as string[] | undefined; - const limit = typeof cfg.limit === 'number' ? cfg.limit : undefined; - const outputVariable = cfg.outputVariable as string | undefined; + const fields = cfg.fields; + const limit = cfg.limit; + const outputVariable = cfg.outputVariable; const data = getData(); if (!data) { @@ -228,12 +246,14 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - const objectName = String(cfg.objectName ?? ''); + const parsed = parseNodeConfig('create_record', node.id, CreateRecordConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const objectName = cfg.objectName; if (!objectName) return refuseNode('create_record: objectName required'); const fields = interpolate(cfg.fields ?? {}, variables, context) as Record; - const outputVariable = cfg.outputVariable as string | undefined; + const outputVariable = cfg.outputVariable; const data = getData(); if (!data) { @@ -309,8 +329,10 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - const objectName = String(cfg.objectName ?? ''); + const parsed = parseNodeConfig('update_record', node.id, UpdateRecordConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const objectName = cfg.objectName; if (!objectName) return refuseNode('update_record: objectName required'); // `filters` → `filter` converted at load (ADR-0087 D2); read canonical. @@ -382,8 +404,10 @@ export function registerCrudNodes(engine: AutomationEngine, ctx: PluginContext): }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - const objectName = String(cfg.objectName ?? ''); + const parsed = parseNodeConfig('delete_record', node.id, DeleteRecordConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const objectName = cfg.objectName; if (!objectName) return refuseNode('delete_record: objectName required'); // `filters` → `filter` converted at load (ADR-0087 D2); read canonical. diff --git a/packages/services/service-automation/src/builtin/http-nodes.ts b/packages/services/service-automation/src/builtin/http-nodes.ts index b6aa7f1f40..b647ab2a4d 100644 --- a/packages/services/service-automation/src/builtin/http-nodes.ts +++ b/packages/services/service-automation/src/builtin/http-nodes.ts @@ -1,11 +1,13 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; +import { defineActionDescriptor, HttpConfigSchema } from '@objectstack/spec/automation'; +import type { HttpConfigParsed } from '@objectstack/spec/automation'; import { randomUUID } from 'node:crypto'; import type { AutomationEngine } from '../engine.js'; import { refuseNode } from '../guard-refusal.js'; import { interpolate } from './template.js'; +import { parseNodeConfig } from './parse-config.js'; /** * HTTP built-in node — canonical `http` (ADR-0018 M3). @@ -93,16 +95,24 @@ export function registerHttpNodes(engine: AutomationEngine, ctx: PluginContext): }), async execute(node, variables, context) { const raw = (node.config ?? {}) as Record; - const cfg = interpolate(raw, variables, context) as Record; + // Parsed AFTER interpolation — unique among the contract-carrying + // builtins, because this executor reads the interpolated config + // wholesale, so that is the shape its contract describes: a `{token}` + // in a typed slot (`timeoutMs`, `durable`) resolves to the value's + // real type before the contract sees it (whole-token interpolation + // preserves types). + const parsed = parseNodeConfig('http', node.id, HttpConfigSchema, interpolate(raw, variables, context)); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; - const url = cfg.url as string | undefined; + const url = cfg.url; if (!url) return refuseNode('http: url is required'); const durable = cfg.durable === true; - const headers = cfg.headers as Record | undefined; + const headers = cfg.headers; const body = cfg.body; - const timeoutMs = typeof cfg.timeoutMs === 'number' ? cfg.timeoutMs : undefined; - const signingSecret = cfg.signingSecret as string | undefined; + const timeoutMs = cfg.timeoutMs; + const signingSecret = cfg.signingSecret; // ── Durable mode: enqueue onto the messaging HTTP outbox ────────── if (durable) { @@ -115,7 +125,7 @@ export function registerHttpNodes(engine: AutomationEngine, ctx: PluginContext): dedupKey: randomUUID(), label: `flow:${node.id}`, url, - method: (cfg.method as string) ?? 'POST', + method: cfg.method ?? 'POST', headers, signingSecret, timeoutMs, @@ -133,7 +143,7 @@ export function registerHttpNodes(engine: AutomationEngine, ctx: PluginContext): } // ── Request/response mode (default; preserves http_request) ─────── - const method = (cfg.method as string) ?? 'GET'; + const method = cfg.method ?? 'GET'; const controller = new AbortController(); const timer = timeoutMs ? setTimeout(() => controller.abort(), timeoutMs) : undefined; try { diff --git a/packages/services/service-automation/src/builtin/loop-node.ts b/packages/services/service-automation/src/builtin/loop-node.ts index ee366da473..da54f18e1e 100644 --- a/packages/services/service-automation/src/builtin/loop-node.ts +++ b/packages/services/service-automation/src/builtin/loop-node.ts @@ -1,11 +1,12 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor, LOOP_MAX_ITERATIONS_CEILING } from '@objectstack/spec/automation'; -import type { FlowRegionParsed } from '@objectstack/spec/automation'; +import { defineActionDescriptor, LOOP_MAX_ITERATIONS_CEILING, LoopConfigSchema } from '@objectstack/spec/automation'; +import type { LoopConfigParsed } from '@objectstack/spec/automation'; import type { AutomationContext } from '@objectstack/spec/contracts'; import type { AutomationEngine, StepLogEntry } from '../engine.js'; import { interpolate } from './template.js'; +import { parseNodeConfig } from './parse-config.js'; /** * `loop` built-in node — a **structured iteration container** (ADR-0031). @@ -59,12 +60,15 @@ export function registerLoopNode(engine: AutomationEngine, ctx: PluginContext): }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - const body = cfg.body as FlowRegionParsed | undefined; + const raw = (node.config ?? {}) as Record; // ── Legacy flat-graph loop (no body) — preserve prior stub behavior. ── - if (body == null) { - const collectionName = typeof cfg.collection === 'string' ? cfg.collection : undefined; + // Deliberately NOT parsed: this shape predates the ADR-0031 construct the + // contract describes (`collection` is required there, but a legacy marker + // loop legally carries none), and the constructs are additive — the + // contract governs only the structured form it defines (#4277). + if (raw.body == null) { + const collectionName = typeof raw.collection === 'string' ? raw.collection : undefined; if (collectionName) { const legacy = variables.get(collectionName); if (Array.isArray(legacy)) { @@ -76,20 +80,20 @@ export function registerLoopNode(engine: AutomationEngine, ctx: PluginContext): } // ── Structured loop container. ── - const iteratorVariable = typeof cfg.iteratorVariable === 'string' && cfg.iteratorVariable - ? cfg.iteratorVariable - : 'item'; - const indexVariable = typeof cfg.indexVariable === 'string' && cfg.indexVariable - ? cfg.indexVariable - : undefined; + const parsed = parseNodeConfig('loop', node.id, LoopConfigSchema, raw); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const body = cfg.body!; + const iteratorVariable = cfg.iteratorVariable; + const indexVariable = cfg.indexVariable || undefined; // Resolve the collection: a `{token}` template, a bare variable name, or - // (defensively) an already-resolved array. + // an inline array (the contract is the union, same as `map`). const rawCollection = cfg.collection; let collection: unknown; if (Array.isArray(rawCollection)) { collection = rawCollection; - } else if (typeof rawCollection === 'string') { + } else { collection = interpolate(rawCollection, variables, context ?? ({} as AutomationContext)); if ((collection == null) && variables.has(rawCollection)) { collection = variables.get(rawCollection); @@ -104,7 +108,7 @@ export function registerLoopNode(engine: AutomationEngine, ctx: PluginContext): } // Hard iteration guard. - const requested = typeof cfg.maxIterations === 'number' ? cfg.maxIterations : LOOP_MAX_ITERATIONS_CEILING; + const requested = cfg.maxIterations ?? LOOP_MAX_ITERATIONS_CEILING; const maxIterations = Math.min(requested, LOOP_MAX_ITERATIONS_CEILING); if (collection.length > maxIterations) { return { diff --git a/packages/services/service-automation/src/builtin/map-node.ts b/packages/services/service-automation/src/builtin/map-node.ts index 051e4baf4f..7c02bae101 100644 --- a/packages/services/service-automation/src/builtin/map-node.ts +++ b/packages/services/service-automation/src/builtin/map-node.ts @@ -1,11 +1,13 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; +import { defineActionDescriptor, MapConfigSchema } from '@objectstack/spec/automation'; +import type { MapConfigParsed } from '@objectstack/spec/automation'; import type { AutomationContext } from '@objectstack/spec/contracts'; import type { AutomationEngine } from '../engine.js'; import { refuseNode } from '../guard-refusal.js'; import { interpolate } from './template.js'; +import { parseNodeConfig } from './parse-config.js'; /** Hard cap on map fan-out — turns a runaway collection into a clean error. */ const MAX_MAP_ITEMS = 10_000; @@ -75,23 +77,26 @@ export function registerMapNode(engine: AutomationEngine, ctx: PluginContext): v }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; // The undeclared `flow` alias this used to accept via a bare `??` has // graduated into the ADR-0087 D2 conversion layer as // 'flow-node-map-flow-alias' (#4045), rewritten at load — including the - // `registerFlow` rehydration seam — so only the canonical key is read - // here (PD #12: no consumer-side fallbacks). - const flowName = typeof cfg.flowName === 'string' ? cfg.flowName : undefined; + // `registerFlow` rehydration seam — so the parse sees only the canonical + // key (PD #12: no consumer-side fallbacks). Parsed BEFORE interpolation; + // `collection` legally arrives as a template string OR an inline array + // (the contract is the union). + const parsed = parseNodeConfig('map', node.id, MapConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const flowName = cfg.flowName; if (!flowName) { return refuseNode(`map '${node.id}': config.flowName (the per-item subflow) is required`); } - const iteratorVariable = - typeof cfg.iteratorVariable === 'string' && cfg.iteratorVariable ? cfg.iteratorVariable : 'item'; - const indexVariable = - typeof cfg.indexVariable === 'string' && cfg.indexVariable ? cfg.indexVariable : undefined; - const outVar = - typeof cfg.outputVariable === 'string' && cfg.outputVariable ? cfg.outputVariable : undefined; + // `|| 'item'` beyond the contract default: an authored EMPTY string still + // falls back (the default only fires on absence). + const iteratorVariable = cfg.iteratorVariable || 'item'; + const indexVariable = cfg.indexVariable || undefined; + const outVar = cfg.outputVariable || undefined; // Resolve the collection (template / bare var / already-an-array). const rawCollection = cfg.collection; @@ -134,7 +139,7 @@ export function registerMapNode(engine: AutomationEngine, ctx: PluginContext): v variables.set(iteratorVariable, item); if (indexVariable) variables.set(indexVariable, idx); - const rawInput = (cfg.input && typeof cfg.input === 'object' ? cfg.input : {}) as Record; + const rawInput = cfg.input ?? {}; const params = interpolate(rawInput, variables, context ?? ({} as AutomationContext)) as Record; // When the mapped item IS a record (has an id), expose it as the @@ -142,7 +147,7 @@ export function registerMapNode(engine: AutomationEngine, ctx: PluginContext): v // targets that item — the natural "approve each row" shape. Otherwise // the item is just data, passed via `params`. const itemIsRecord = item != null && typeof item === 'object' && typeof (item as any).id === 'string'; - const itemObject = typeof cfg.itemObject === 'string' ? cfg.itemObject : (context as any)?.object; + const itemObject = cfg.itemObject ?? (context as any)?.object; const childContext = { ...(context ?? {}), diff --git a/packages/services/service-automation/src/builtin/notify-node.ts b/packages/services/service-automation/src/builtin/notify-node.ts index 5517b37ad9..27803912dc 100644 --- a/packages/services/service-automation/src/builtin/notify-node.ts +++ b/packages/services/service-automation/src/builtin/notify-node.ts @@ -2,9 +2,11 @@ import type { PluginContext } from '@objectstack/core'; import type { AutomationContext } from '@objectstack/spec/contracts'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; +import { defineActionDescriptor, NotifyConfigSchema } from '@objectstack/spec/automation'; +import type { NotifyConfigParsed } from '@objectstack/spec/automation'; import type { AutomationEngine } from '../engine.js'; import { interpolate, stringifyForTemplate, type VariableMap } from './template.js'; +import { parseNodeConfig } from './parse-config.js'; /** * Structural view of `@objectstack/service-messaging`'s service (ADR-0012), @@ -92,7 +94,7 @@ function toStr(value: unknown): string | undefined { * (Prime Directive #12). Same graduation path as `object` → `objectName` (#3796). */ function resolveSource( - cfg: Record, + cfg: Pick, variables: VariableMap, context: AutomationContext, ): { object: string; id: string } | undefined { @@ -184,11 +186,17 @@ export function registerNotifyNode(engine: AutomationEngine, ctx: PluginContext) }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - // The historical aliases (`to`/`subject`/`body`/`url`) are canonicalized // at load by the ADR-0087 D2 conversion 'flow-node-notify-config-aliases' - // (#3796), so only the canonical keys are read here. + // (#3796), so the parse sees only canonical keys. Parsed BEFORE + // interpolation — the contract's slots are string-typed, so `{token}` + // templates pass; the post-interpolation guards below still own + // "title/recipients resolved to nothing" (#3582), which no static + // parse can see. + const parsed = parseNodeConfig('notify', node.id, NotifyConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const recipientCfg = cfg.recipients ?? []; const recipients = toStringList(interpolate(recipientCfg, variables, context)); // stringifyForTemplate (not String()): a sole-token `{$error}` resolves diff --git a/packages/services/service-automation/src/builtin/parallel-node.ts b/packages/services/service-automation/src/builtin/parallel-node.ts index 84afdee0e3..68c475075c 100644 --- a/packages/services/service-automation/src/builtin/parallel-node.ts +++ b/packages/services/service-automation/src/builtin/parallel-node.ts @@ -1,15 +1,11 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; -import type { FlowRegionParsed } from '@objectstack/spec/automation'; +import { defineActionDescriptor, ParallelConfigSchema } from '@objectstack/spec/automation'; +import type { ParallelConfigParsed } from '@objectstack/spec/automation'; import type { AutomationContext } from '@objectstack/spec/contracts'; import type { AutomationEngine, StepLogEntry } from '../engine.js'; - -/** One branch of a parallel block — a region plus an optional label. */ -interface ParallelBranch extends FlowRegionParsed { - name?: string; -} +import { parseNodeConfig } from './parse-config.js'; /** * `parallel` built-in node — a **structured parallel block** with an @@ -65,15 +61,12 @@ export function registerParallelNode(engine: AutomationEngine, ctx: PluginContex }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - const branches = cfg.branches as ParallelBranch[] | undefined; - - if (!Array.isArray(branches) || branches.length < 2) { - return { - success: false, - error: `parallel '${node.id}': config.branches must declare at least 2 branch regions`, - }; - } + // The contract owns the shape guard: `branches` is required, each branch + // a region, and `.min(2)` refuses the degenerate single-branch block the + // hand-written check here used to catch. + const parsed = parseNodeConfig('parallel', node.id, ParallelConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const branches = parsed.config.branches; let branchSteps: StepLogEntry[][]; try { diff --git a/packages/services/service-automation/src/builtin/parse-config.ts b/packages/services/service-automation/src/builtin/parse-config.ts new file mode 100644 index 0000000000..31cffc18b5 --- /dev/null +++ b/packages/services/service-automation/src/builtin/parse-config.ts @@ -0,0 +1,102 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Execute-time `parse()` of a builtin node's config against its Zod contract + * (#4277 — the enforcement half #4045 deliberately deferred). + * + * The contracts in `@objectstack/spec/automation` (`io-node-config.zod.ts`, + * `builtin-node-config.zod.ts`, `control-flow.zod.ts`) were written by reading + * what each executor does with `node.config`, and the form↔Zod ledger tests + * reconcile them against the descriptor forms — but until #4277 nothing + * `parse()`d them, so `configSchema` provided none of the type / `required` + * runtime enforcement it appears to promise (`FlowNodeSchema.config` is + * `z.record(z.unknown())`). This helper is the single seam every + * contract-carrying builtin runs its config through. + * + * ## Where each executor calls this + * + * At the executor's **natural read point**, which is not the same for all: + * `http` interpolates its whole config first and reads the result, so it + * parses POST-interpolation (its contract describes the interpolated shape — + * a `{token}` in a typed slot resolves to its value's real type before the + * contract sees it); everyone else reads the raw stored config key-by-key and + * parses it as stored, where `{token}` templates are still strings and the + * string-typed contract slots accept them. `loop` additionally exempts its + * legacy flat-graph path (no `config.body`) — that shape predates the + * ADR-0031 construct the contract describes. + * + * ## Why failure is a guard refusal, not a routable error + * + * A config that fails its contract is wrong METADATA: re-running the flow + * unchanged can never succeed, and the fix is to edit the node — the exact + * criteria {@link refuseNode} states ("a missing required config key" is its + * canonical example). Routing it through a `fault` edge would let one edge + * silence the contract for a node forever, the same suppression shape #3863 + * closed for the other refuse-to-execute guards. + * + * Unknown keys are NOT this seam's job: Zod's default `.strip()` drops them + * silently here, and `registerFlow()` rejects them loudly at registration + * (the tightened #4059 check). Type + `required` live here; key membership + * lives there. + */ + +import { refuseNode } from '../guard-refusal.js'; + +/** + * Structural view of a Zod schema's `safeParse` — service-automation takes no + * direct `zod` dependency (the contracts arrive already-built via + * `@objectstack/spec`), the same boundary that has the form↔Zod ledger tests + * reading key sets off `.shape` instead of importing zod. + */ +export interface NodeConfigContract { + safeParse(value: unknown): { + success: boolean; + data?: T; + error?: { issues: ReadonlyArray<{ path: ReadonlyArray; message: string }> }; + }; +} + +export type ParsedNodeConfig = + | { ok: true; config: T } + | { ok: false; refusal: { success: false; error: string; errorClass: 'guard' } }; + +/** `config.fields[0].name` — the same path spelling the registration-time + * undeclared-key diagnostic uses, so both layers report locations alike. */ +function formatIssuePath(path: ReadonlyArray): string { + let out = 'config'; + for (const seg of path) { + out += typeof seg === 'number' ? `[${seg}]` : `.${String(seg)}`; + } + return out; +} + +/** + * Parse `config` against the node type's Zod contract. On success returns the + * parsed config — typed, with the contract's defaults applied — which the + * executor reads INSTEAD of the raw record. On failure returns a ready-made + * guard refusal naming every violation with its path, so the step log carries + * the full prescription instead of the first symptom. + */ +export function parseNodeConfig( + nodeType: string, + nodeId: string, + contract: NodeConfigContract, + config: unknown, +): ParsedNodeConfig { + const result = contract.safeParse(config ?? {}); + if (result.success) { + return { ok: true, config: result.data as T }; + } + const issues = (result.error?.issues ?? []) + .map((i) => `${formatIssuePath(i.path)}: ${i.message}`) + .join('; '); + return { + ok: false, + refusal: refuseNode( + `${nodeType} '${nodeId}': config does not satisfy the ${nodeType} contract — ${issues || 'invalid config'}. ` + + `The config is metadata, so re-running changes nothing; fix the node in the flow definition. ` + + `The declared contract is the node type's configSchema (the Studio form) and the ` + + `${nodeType} config Zod in @objectstack/spec/automation (#4277).`, + ), + }; +} diff --git a/packages/services/service-automation/src/builtin/screen-nodes.ts b/packages/services/service-automation/src/builtin/screen-nodes.ts index 93f0cbec0e..ac303e0f99 100644 --- a/packages/services/service-automation/src/builtin/screen-nodes.ts +++ b/packages/services/service-automation/src/builtin/screen-nodes.ts @@ -1,9 +1,11 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; +import { defineActionDescriptor, ScreenConfigSchema } from '@objectstack/spec/automation'; +import type { ScreenConfigParsed } from '@objectstack/spec/automation'; import type { AutomationEngine } from '../engine.js'; import { interpolate } from './template.js'; +import { parseNodeConfig } from './parse-config.js'; /** * Screen / Script built-in nodes — 'screen' and 'script' executors. @@ -98,7 +100,12 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; + // Parsed BEFORE interpolation: the contract's typed slots are strings + // (or `unknown`, for the interpolatable `defaultValue`/`defaults`), so + // `{var}` templates pass the parse and resolve below as always. + const parsedCfg = parseNodeConfig('screen', node.id, ScreenConfigSchema, node.config); + if (!parsedCfg.ok) return parsedCfg.refusal; + const cfg = parsedCfg.config; // `{var}` tokens in screen config resolve against the live flow // variables here (the engine does NOT pre-interpolate node config) — so // a step's title/description/field-default/object-form-default can pull @@ -143,7 +150,7 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext }; } - const rawFields = Array.isArray(cfg.fields) ? (cfg.fields as Array>) : []; + const rawFields = cfg.fields ?? []; const hasFields = rawFields.length > 0; // Suspend to collect input when the screen declares fields, or opts in // explicitly. `waitForInput === false` forces a server pass-through. @@ -152,13 +159,13 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext return { success: true }; } const fields = rawFields.map((f) => ({ - name: String(f.name ?? ''), - label: f.label != null ? String(f.label) : undefined, - type: f.type != null ? String(f.type) : undefined, + name: f.name, + label: f.label, + type: f.type, required: f.required === true, - options: Array.isArray(f.options) ? (f.options as Array<{ value: unknown; label: string }>) : undefined, + options: f.options as Array<{ value: unknown; label: string }> | undefined, defaultValue: f.defaultValue !== undefined ? interpolate(f.defaultValue, variables, context) : undefined, - placeholder: f.placeholder != null ? String(f.placeholder) : undefined, + placeholder: f.placeholder, // Forwarded RAW — deliberately not interpolated. `visibleWhen` is a // predicate the client re-evaluates on every keystroke against the // values collected SO FAR; the server has no view of those, so @@ -172,7 +179,7 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext // validating the full list blocked Submit on a field the user was // never shown — the run then sat paused with no resume request ever // issued (#3528). - visibleWhen: f.visibleWhen != null ? String(f.visibleWhen) : undefined, + visibleWhen: f.visibleWhen, })).filter((f) => f.name.length > 0); return { success: true, diff --git a/packages/services/service-automation/src/builtin/try-catch-node.ts b/packages/services/service-automation/src/builtin/try-catch-node.ts index f148d356bd..6faf3f5b09 100644 --- a/packages/services/service-automation/src/builtin/try-catch-node.ts +++ b/packages/services/service-automation/src/builtin/try-catch-node.ts @@ -1,18 +1,11 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor } from '@objectstack/spec/automation'; -import type { FlowRegionParsed } from '@objectstack/spec/automation'; +import { defineActionDescriptor, TryCatchConfigSchema } from '@objectstack/spec/automation'; +import type { TryCatchConfigParsed } from '@objectstack/spec/automation'; import type { AutomationContext } from '@objectstack/spec/contracts'; import type { AutomationEngine } from '../engine.js'; - -interface RetryPolicy { - maxRetries?: number; - retryDelayMs?: number; - backoffMultiplier?: number; - maxRetryDelayMs?: number; - jitter?: boolean; -} +import { parseNodeConfig } from './parse-config.js'; /** * `try_catch` built-in node — **structured try/catch/retry** (ADR-0031 §Decision 3). @@ -77,23 +70,25 @@ export function registerTryCatchNode(engine: AutomationEngine, ctx: PluginContex }, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - const tryRegion = cfg.try as FlowRegionParsed | undefined; - const catchRegion = cfg.catch as FlowRegionParsed | undefined; - const errorVariable = - typeof cfg.errorVariable === 'string' && cfg.errorVariable ? cfg.errorVariable : '$error'; - const retry = (cfg.retry ?? {}) as RetryPolicy; - - if (tryRegion == null) { - return { success: false, error: `try_catch '${node.id}': config.try region is required` }; - } + // Parse against the ADR-0031 contract. Note the retry defaults now come + // from the CONTRACT (RetryPolicySchema): a declared `retry` block that + // omits `retryDelayMs` gets the documented 1000ms base delay, where this + // executor historically filled in 0 — the declared default is the + // enforced one (#4277). + const parsed = parseNodeConfig('try_catch', node.id, TryCatchConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const tryRegion = cfg.try; + const catchRegion = cfg.catch; + const errorVariable = cfg.errorVariable || '$error'; + const retry = cfg.retry; const ctxOrEmpty = context ?? ({} as AutomationContext); - const maxRetries = typeof retry.maxRetries === 'number' ? retry.maxRetries : 0; - const baseDelay = typeof retry.retryDelayMs === 'number' ? retry.retryDelayMs : 0; - const multiplier = typeof retry.backoffMultiplier === 'number' ? retry.backoffMultiplier : 1; - const maxDelay = typeof retry.maxRetryDelayMs === 'number' ? retry.maxRetryDelayMs : 30000; - const useJitter = retry.jitter === true; + const maxRetries = retry?.maxRetries ?? 0; + const baseDelay = retry?.retryDelayMs ?? 0; + const multiplier = retry?.backoffMultiplier ?? 1; + const maxDelay = retry?.maxRetryDelayMs ?? 30000; + const useJitter = retry?.jitter === true; // Run the try region, retrying with exponential backoff up to maxRetries. let lastError = 'unknown error'; diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 3672145b50..8923778a64 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -35,6 +35,36 @@ interface ConfigSchemaNode { items?: ConfigSchemaNode; additionalProperties?: unknown; } + +/** + * Known-confusable flow-node config keys → precise authoring guidance, + * appended to the undeclared-key rejection (#4277). The `UNKNOWN_KEY_GUIDANCE` + * pattern from `object.zod.ts`, scoped per node type so a key name that is + * wrong on one node but real on another never misfires. + * + * Entries are seeded only from documented incidents — a tombstone with no + * history is noise. The generic rejection already carries the path, the + * did-you-mean and the declared set; an entry here adds the *mechanism* the + * author was reaching for. + */ +const FLOW_NODE_UNKNOWN_KEY_GUIDANCE: Record> = { + create_record: { + fieldValues: + 'The write map is `fields` — `fieldValues` was an AI-authoring dialect that never had a ' + + 'runtime reader; the fix is the authoring source + this rejection, not a runtime alias ' + + '(#2419, rejected by design).', + }, + update_record: { + fieldValues: + 'The write map is `fields` — `fieldValues` was an AI-authoring dialect that never had a ' + + 'runtime reader (#2419, rejected by design).', + }, + screen: { + visibleIf: + 'The visibility predicate is `visibleWhen` (bare CEL, re-evaluated client-side as the ' + + 'user types — #3528, the incident this whole check descends from).', + }, +}; import { runIsUnscopedUserMode, flowTouchesData } from './runtime-identity.js'; import { isGuardRefusal } from './guard-refusal.js'; @@ -1435,10 +1465,12 @@ export class AutomationEngine implements IAutomationService { // executeNode() already throws NO_EXECUTOR at run time for unknown types. this.validateNodeTypes(name, parsed); - // #4045 — warn on config keys the node's descriptor does not declare - // (a `visibleIf` typo is silently accepted today). Warn-only: see - // validateNodeConfigKeys for why hard-failing would gamble on the nine - // builtins whose read-vs-declared drift has not been audited. + // #4277 — REJECT config keys the node's descriptor does not declare + // (the tightened #4059 warning; a `visibleIf` typo used to register in + // silence). Hard-fail with per-key prescriptions: see + // validateNodeConfigKeys for why the #4045 reconciliation made this + // safe, and for the deliberate exemptions (`assignment`, schemaless + // types, keyValue maps). this.validateNodeConfigKeys(name, parsed); // ADR-0032 §Decision 1a — parse-validate every predicate at registration, @@ -2686,48 +2718,81 @@ export class AutomationEngine implements IAutomationService { } /** - * Warn about node `config` keys the node type's descriptor does not declare - * (#4045 — the warn half of the unknown-key ladder). + * REJECT node `config` keys the node type's descriptor does not declare + * (#4277 — the error half of the #4045 unknown-key ladder; #4059 shipped + * the warn half). + * + * `FlowNodeSchema.config` is `z.record(z.unknown())`, so before #4059 a + * misspelled or invented config key was accepted in total silence: + * `visibleIf` instead of `visibleWhen` registered cleanly and then did + * nothing, which is exactly the failure shape that made #3528 take three + * passes to diagnose. The key is never read, so there is no runtime error + * to trace back — the only symptom is a feature that quietly does not + * happen. + * + * **Why an error is safe now.** An undeclared key falls into three + * populations, and when #4059 landed this seam could not tell them apart: + * an author typo (reject), a key the executor genuinely reads that its + * hand-written `configSchema` never declared (`notify.source` was exactly + * this — rejecting those breaks working apps), and dead config nobody + * reads. The #4045 reconciliation closed the second population — every + * read-but-undeclared key across the schema-carrying builtins was either + * declared on its descriptor or graduated into the ADR-0087 conversion + * layer, with ledger tests ratcheting both directions — and the #4059 + * warning measured what remains in live metadata. What survives to this + * check today is typos and dead config, and both are metadata bugs the + * platform's contract-first rule says to reject at the producer, loudly + * (Prime Directive #12; ADR-0032 no-silent-failure). * - * `FlowNodeSchema.config` is `z.record(z.unknown())`, so a misspelled or - * invented config key is accepted in total silence today: `visibleIf` instead - * of `visibleWhen` registers cleanly and then does nothing, which is exactly - * the failure shape that made #3528 take three passes to diagnose. The key is - * never read, so there is no runtime error to trace back — the only symptom - * is a feature that quietly does not happen. + * The rejection carries its prescription (the `UNKNOWN_KEY_GUIDANCE` + * pattern from `object.zod.ts`): every violation names its exact path, the + * declared key set, a did-you-mean when edit distance allows, and — for + * keys with documented history — a per-key tombstone from + * {@link FLOW_NODE_UNKNOWN_KEY_GUIDANCE}. * - * **Warn, never reject.** An undeclared key falls into three populations and - * this seam cannot yet tell them apart: an author typo (which we want to - * reject), a key the executor genuinely reads that its hand-written - * `configSchema` never declared (`notify.source` was exactly this until - * #4045 — rejecting those breaks working apps), and dead config nobody reads - * (harmless). Only 4 of the 13 schema-carrying builtins have been audited for - * the second population, so hard-failing here would gamble on the 9 that have - * not. The warning is what measures that distribution; tightening to an error - * is a later, per-key decision once the data exists, and belongs with a - * tombstone that carries the prescription (the `UNKNOWN_KEY_GUIDANCE` pattern - * in `object.zod.ts`). + * Deliberate exemptions, unchanged from the warn era: + * - **`assignment` is exempt wholesale**: with no `assignments` wrapper + * its top-level config keys ARE the author's variable names + * (logic-nodes.ts normalizes three shapes), so no fixed key set can + * describe it — pinned as un-reconcilable by the form↔Zod ledger. + * - Schemaless types (`decision`, `script`, `wait`, `subflow`, + * `connector_action`) publish no `configSchema` ⇒ nothing is declared, + * so nothing can be undeclared. `wait` / `connector_action` keep their + * contracts on FlowNode SIBLING blocks (`waitEventConfig` / + * `connectorConfig`), which this walk never touches. + * - keyValue maps (`additionalProperties: true`, no fixed `properties`) + * stop the walk: their keys are author data, not config keys. * - * Soft-fail matches {@link validateNodeTypes} directly above — same function, - * same `logger.warn` channel, same reasoning about not breaking flows over a - * diagnostic. Nothing about the published `configSchema` changes, so no - * consumer (designer form generation included) sees a different shape. + * Hard-fail matches {@link validateFlowExpressions} below — a flow whose + * metadata is wrong never registers, and every `registerFlow` call site + * already try/catches per flow, so a bad flow is skipped loudly at boot + * rather than crashing the kernel. */ private validateNodeConfigKeys(flowName: string, flow: FlowParsed): void { + const violations: string[] = []; for (const node of flow.nodes) { + // `assignment` config keys are the author's variable names — see the + // exemption note above. + if (node.type === 'assignment') continue; const schema = this.actionDescriptors.get(node.type)?.configSchema as ConfigSchemaNode | undefined; - // No descriptor, or a deliberately schemaless type (`decision`, - // `script`, `wait`, `subflow`, `connector_action` publish none — - // see config-schemas.test) ⇒ nothing is declared, so nothing can - // be undeclared. if (!schema) continue; - this.warnUndeclaredConfigKeys(flowName, node, schema, node.config, 'config'); + this.collectUndeclaredConfigKeys(node, schema, node.config, 'config', violations); + } + if (violations.length > 0) { + throw new Error( + `Flow '${flowName}' rejected: ${violations.length} undeclared config key(s) (#4277).\n` + + violations.map((v) => ` - ${v}`).join('\n') + + `\nAn undeclared key is never read, so it can only be a typo or dead config — fix the ` + + `flow's metadata (rename or remove the key). If an executor genuinely reads this key, ` + + `declare it on the node type's descriptor configSchema instead; read-but-undeclared ` + + `keys are exactly the drift the #4045 reconciliation closed.`, + ); } } /** - * Walk `value` against `schema` in lockstep, warning on keys the schema does - * not declare. + * Walk `value` against `schema` in lockstep, collecting keys the schema does + * not declare into `violations`. * * Descends **only where the schema declares structure** — an object with fixed * `properties`, or an array whose `items` do. It deliberately stops at a @@ -2741,17 +2806,17 @@ export class AutomationEngine implements IAutomationService { * comparison would miss `visibleIf` — the exact mistake this check exists to * catch — while still reporting the rarer top-level ones. A test pins it. */ - private warnUndeclaredConfigKeys( - flowName: string, + private collectUndeclaredConfigKeys( node: FlowNodeParsed, schema: ConfigSchemaNode, value: unknown, path: string, + violations: string[], ): void { if (schema.type === 'array' && schema.items) { if (!Array.isArray(value)) return; value.forEach((element, index) => { - this.warnUndeclaredConfigKeys(flowName, node, schema.items!, element, `${path}[${index}]`); + this.collectUndeclaredConfigKeys(node, schema.items!, element, `${path}[${index}]`, violations); }); return; } @@ -2764,7 +2829,7 @@ export class AutomationEngine implements IAutomationService { for (const key of Object.keys(value as Record)) { const child = schema.properties[key]; if (child) { - this.warnUndeclaredConfigKeys(flowName, node, child, (value as Record)[key], `${path}.${key}`); + this.collectUndeclaredConfigKeys(node, child, (value as Record)[key], `${path}.${key}`, violations); continue; } // The declared set is printed ALWAYS, not only as a fallback when the @@ -2778,11 +2843,13 @@ export class AutomationEngine implements IAutomationService { // listing them is both cheap and complete, and the suggestion becomes // a bonus for the cases it does catch. const suggestion = nearestName(key, declared); - this.logger.warn( - `[flow '${flowName}'] node '${node.id}' (${node.type}): unknown config key \`${key}\` at ${path}.${key}` + + const guidance = FLOW_NODE_UNKNOWN_KEY_GUIDANCE[node.type]?.[key]; + violations.push( + `node '${node.id}' (${node.type}): unknown config key \`${key}\` at ${path}.${key}` + (suggestion ? ` — did you mean \`${suggestion}\`?` : '') + ` It is not declared by this node type's configSchema, so nothing reads it.` + - ` Declared here: ${declared.join(', ')}.`, + ` Declared here: ${declared.join(', ')}.` + + (guidance ? ` ${guidance}` : ''), ); } } diff --git a/packages/services/service-automation/src/guard-refusal-inventory.test.ts b/packages/services/service-automation/src/guard-refusal-inventory.test.ts index 51089d7aea..fb8a21607c 100644 --- a/packages/services/service-automation/src/guard-refusal-inventory.test.ts +++ b/packages/services/service-automation/src/guard-refusal-inventory.test.ts @@ -79,29 +79,34 @@ function flowWithHandler(name: string, node: Record) { * that starts failing for a DIFFERENT reason does not pass vacuously. */ const GUARDS: Array<{ name: string; why: string; node: Record; expect: string }> = [ + // Since #4277 a missing REQUIRED key is refused by the executor's contract + // parse (parse-config.ts) before the hand-written guard runs, so those + // entries pin the parse refusal's fragment. The classification is the + // point of this inventory and is unchanged: a contract violation is wrong + // metadata, so it must stay a guard — un-routable. { name: 'get_record without objectName', why: 'a required config key — no run can supply it', node: { type: 'get_record', config: { filter: { id: 'x' } } }, - expect: 'objectName required', + expect: 'does not satisfy the get_record contract', }, { name: 'create_record without objectName', why: 'a required config key', node: { type: 'create_record', config: { fields: { a: 1 } } }, - expect: 'objectName required', + expect: 'does not satisfy the create_record contract', }, { name: 'update_record without objectName', why: 'a required config key', node: { type: 'update_record', config: { fields: { a: 1 } } }, - expect: 'objectName required', + expect: 'does not satisfy the update_record contract', }, { name: 'delete_record without objectName', why: 'a required config key', node: { type: 'delete_record', config: { filter: { id: 'x' } } }, - expect: 'objectName required', + expect: 'does not satisfy the delete_record contract', }, { name: 'get_record whose filter lost a condition (#3810)', @@ -128,7 +133,7 @@ const GUARDS: Array<{ name: string; why: string; node: Record; name: 'http without url', why: 'a required config key', node: { type: 'http', config: { method: 'GET' } }, - expect: 'url is required', + expect: 'does not satisfy the http contract', }, { name: 'subflow without flowName', diff --git a/packages/spec/src/automation/builtin-node-config.zod.ts b/packages/spec/src/automation/builtin-node-config.zod.ts index 86f96dc411..6e8e221964 100644 --- a/packages/spec/src/automation/builtin-node-config.zod.ts +++ b/packages/spec/src/automation/builtin-node-config.zod.ts @@ -25,12 +25,15 @@ * `map.input`), and the undeclared `map.flow` alias, which graduated into the * ADR-0087 D2 conversion layer like `notify.source` before it. * - * ## What these schemas are (and are not) wired to + * ## What these schemas are wired to (#4277) * - * Contract exports only — no engine path `parse()`s a node config with them - * today, so registering a flow behaves exactly as before. Wiring the executors - * to parse is deliberately deferred until the #4059 undeclared-key warning has - * measured a release's worth of real metadata (#4045 step 3b). + * Live execute-time contracts: each executor `parse()`s its config against + * its schema before running (`service-automation`'s `parse-config.ts`), so + * type and `required` violations refuse the node as a guard. All of these + * parse the RAW stored config — their typed slots are strings (or `unknown` + * where values interpolate), so `{token}` templates pass and resolve at the + * executor's existing interpolation points. Unknown keys are rejected earlier, + * at `registerFlow()` (the tightened #4059 check); the parse here strips them. * * Deliberately absent: * - `assignment` — its config cannot be described by a fixed key set: with no diff --git a/packages/spec/src/automation/control-flow.test.ts b/packages/spec/src/automation/control-flow.test.ts index 34016a64c6..5485ab671e 100644 --- a/packages/spec/src/automation/control-flow.test.ts +++ b/packages/spec/src/automation/control-flow.test.ts @@ -69,7 +69,17 @@ describe('LoopConfigSchema', () => { }) as { properties?: { collection?: { xExpression?: unknown; description?: unknown } } }; expect(schema.properties?.collection?.xExpression).toBe('template'); // description survives alongside the marker (they share one .meta()). - expect(schema.properties?.collection?.description).toBe('Template/variable resolving to the array to iterate'); + expect(schema.properties?.collection?.description).toBe( + 'Template/variable resolving to the array to iterate (an inline array is accepted)', + ); + }); + + it('accepts an inline array collection — the union map.collection declares (#4277)', () => { + // The executor has always resolved an already-an-array collection (shared + // logic with `map`); the string-only declaration under-declared what it + // reads, which the execute-time parse wiring surfaced. + const parsed = LoopConfigSchema.parse({ collection: [1, 2, 3] }); + expect(parsed.collection).toEqual([1, 2, 3]); }); }); diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index 0f7ad74d2d..9b9194bfaf 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -101,16 +101,19 @@ export type FlowRegionParsed = z.infer; export const LoopConfigSchema = lazySchema(() => z.object({ /** * The collection to iterate. A `{token}` template or bare variable name that - * resolves (at run time) to an array in the flow's variable scope. + * resolves (at run time) to an array in the flow's variable scope, or an + * inline array — the same union `map.collection` declares, because the two + * executors share the resolve logic (#4277 aligned this contract with what + * the executor has always read; the string-only declaration under-declared). */ - // `xExpression: 'template'` marks this as an `interpolate()` `{var}` template - // (not bare CEL), so the flow designer renders a `{var}` picker + mono editor - // and skips the CEL brace-trap (objectui #2670 Phase 3). Flows through - // `z.toJSONSchema` verbatim, same channel as `xRef` / `xEnumDeprecated`. The - // shipped `loop` descriptor carries the same marker on its hand-written - // configSchema literal (service-automation/builtin/loop-node.ts). - collection: z.string().min(1).meta({ - description: 'Template/variable resolving to the array to iterate', + // `xExpression: 'template'` marks the string form as an `interpolate()` + // `{var}` template (not bare CEL), so the flow designer renders a `{var}` + // picker + mono editor and skips the CEL brace-trap (objectui #2670 Phase 3). + // Flows through `z.toJSONSchema` verbatim, same channel as `xRef` / + // `xEnumDeprecated`. The shipped `loop` descriptor carries the same marker on + // its hand-written configSchema literal (service-automation/builtin/loop-node.ts). + collection: z.union([z.string().min(1), z.array(z.unknown())]).meta({ + description: 'Template/variable resolving to the array to iterate (an inline array is accepted)', xExpression: 'template', }), /** Variable name the current item is bound to inside the body. */ diff --git a/packages/spec/src/automation/io-node-config.zod.ts b/packages/spec/src/automation/io-node-config.zod.ts index 6cb1e52c3d..25465d5cd4 100644 --- a/packages/spec/src/automation/io-node-config.zod.ts +++ b/packages/spec/src/automation/io-node-config.zod.ts @@ -15,15 +15,20 @@ * and a Zod copied from the form would make that reconciliation a tautology — * it would pass by construction and prove nothing (#4045). * - * ## What these schemas are (and are not) wired to + * ## What these schemas are wired to (#4277) * * Like `LoopConfigSchema` / `ParallelConfigSchema` / `TryCatchConfigSchema`, - * these are **contract exports**: no engine path `parse()`s a node config with - * them today, so registering a flow behaves exactly as before. Wiring the - * executors to parse — which would finally give node configs the type / - * `required` / unknown-key enforcement the descriptor `configSchema` never - * provided — is deliberately deferred until the #4059 undeclared-key warning - * has measured a release's worth of real metadata (#4045 step 3b). + * these are **live execute-time contracts**: each executor `parse()`s its + * config against its schema before running (`service-automation`'s + * `parse-config.ts`), so type and `required` violations refuse the node as a + * guard (not routable via `fault` edges). `notify` parses the RAW stored + * config — its slots are string-typed, so `{token}` templates pass and the + * post-interpolation guards still own "resolved to nothing". `http` parses + * the INTERPOLATED config, because that is the shape its executor reads — + * a `{token}` in a typed slot (`timeoutMs`, `durable`) resolves to its real + * type first. Unknown keys are the registration layer's job: `registerFlow()` + * rejects keys the descriptor `configSchema` does not declare (the tightened + * #4059 check), while the parse here strips them. * * `connector_action` has no schema here on purpose: its config contract is * empty. The executor reads only the declared `FlowNodeSchema.connectorConfig` diff --git a/packages/spec/src/automation/node-executor.zod.ts b/packages/spec/src/automation/node-executor.zod.ts index 94c301cc58..49273e47c7 100644 --- a/packages/spec/src/automation/node-executor.zod.ts +++ b/packages/spec/src/automation/node-executor.zod.ts @@ -243,12 +243,18 @@ export const ActionDescriptorSchema = lazySchema(() => z.object({ * `config`. Drives Studio form generation. Optional — actions with no config * omit it. * - * **What actually validates against this, and what does not** (#4027). An - * earlier revision of this doc claimed `registerFlow()` checks that `config` - * satisfies `configSchema`. It does not, and never did — `FlowNodeSchema.config` - * is `z.record(z.unknown())`, so no layer rejects an undeclared or misspelled - * config key at author time. What *is* enforced: + * **What actually validates against this, and what does not** (#4027, + * tightened by #4277). `FlowNodeSchema.config` is `z.record(z.unknown())`, + * so the flow-level parse never types a node's config — enforcement is + * layered on top: * + * - **Unknown keys are rejected at `registerFlow()`** (#4277, tightening + * the #4059 warning): a config key this schema does not declare fails + * registration with the path, the declared key set, a did-you-mean, and + * a per-key tombstone where one exists. The walk stops at keyValue maps + * (`additionalProperties: true` — those keys are author data) and + * exempts `assignment` wholesale (its top-level keys ARE the author's + * variable names). * - **Expression slots are validated.** Every property marked * `xExpression: 'expression'` (bare CEL) is parse-checked at `registerFlow()` * and by `objectstack validate`, via the `FLOW_NODE_EXPRESSION_PATHS` ledger @@ -256,17 +262,20 @@ export const ActionDescriptorSchema = lazySchema(() => z.object({ * unvalidated the way `screen.fields[].visibleWhen` did for four months * (#3528). Slots marked `xExpression: 'template'` are recorded but not * checked: no validator implements the single-brace `{var}` dialect they use. - * - **Everything else is designer-facing only.** Types, `required`, `enum` and - * unknown keys are not enforced anywhere. Do not rely on this schema as a - * runtime guard; an executor must still validate its own config. + * - **Types and `required` are enforced at execute time for the + * contract-carrying builtins** (#4277): those executors `parse()` their + * config against the Zod contracts in `io-node-config.zod.ts` / + * `builtin-node-config.zod.ts` / `control-flow.zod.ts`, refusing the node + * (guard, not routable) on violation. For a plugin-contributed executor + * this schema remains designer-facing — the plugin must still validate + * its own config, and SHOULD follow the same parse pattern. * - * The schema is also hand-written per executor rather than derived from the - * code that reads `config`, so a property can be declared without being read - * (or read without being declared) with nothing to reconcile the two. Only the - * expression subset has a ratchet today. + * The schema is hand-written per executor rather than derived from the code + * that reads `config`; the form↔Zod ledger tests in `service-automation` + * reconcile the two for the builtins. */ configSchema: z.unknown().optional() - .describe('JSON Schema for the node config (drives the designer form; only declared expression slots are validated)'), + .describe('JSON Schema for the node config (drives the designer form; undeclared keys are rejected at registration)'), // ── capabilities ────────────────────────────────────────────────── /** Supports async pause/resume (e.g. wait, human_task). */