diff --git a/.changeset/script-branch-keys-retired.md b/.changeset/script-branch-keys-retired.md new file mode 100644 index 0000000000..d1830f5d1f --- /dev/null +++ b/.changeset/script-branch-keys-retired.md @@ -0,0 +1,86 @@ +--- +'@objectstack/spec': major +'@objectstack/service-automation': minor +'@objectstack/lint': patch +--- + +feat(spec,automation)!: converge `script` to a function call — retire the `actionType` branches — and parse `script` / `subflow` config at execute time (#4343) + +A `script` node had four ways to name what it ran and only one of them ran anything. +Protocol 17 keeps that one and retires the rest. + +- **`config.actionType: 'email' | 'slack'`** were **logger-backed stubs**. They wrote a + line, reported success, and delivered nothing — under any configuration, installed + messaging service or not. Every bundled example used one; none of them ever sent + anything. +- **`config.template` / `.recipients` / `.variables`** fed those stubs, so they addressed + a message no channel sent. (The examples did not even reach them: they passed the + payload in `inputs`, which the built-in branch never read.) +- **inline `config.script`** was recognized and **never executed** — the built-in runtime + has no server-side JS sandbox, so the node warned and completed as a no-op. +- **any other `actionType`** was shorthand for a registered-function name — a second + spelling of `config.function` — and `'invoke_function'` was a marker that named nothing + on its own. + +What remains is what worked: `config.function` (now **required**) names a registered +function, `config.inputs` feeds it, `config.outputVariable` binds its return value. + +**The replacements are three different mechanisms, not one rename.** + +| Retired | Use instead | +| --- | --- | +| `actionType: 'email'` (+ `template` / `recipients` / `variables`) | a `notify` node — it delivers through the messaging service: the in-app inbox by default, real email once `@objectstack/plugin-email` is installed | +| `actionType: 'slack'` | a `connector_action` node with the Slack connector, or an `http` node posting to an incoming webhook — `notify` has no Slack channel | +| `actionType: 'my_fn'` (shorthand) | `function: 'my_fn'` — the conversion moves it for you | +| `script: '…'` (inline JS) | move the logic into a registered function and call it via `config.function` | + +**Execute-time parse.** `script` and `subflow` now run their config through the contract +before executing, the seam #4277 gave the flat builtins — a violation refuses the node as +a **guard** (wrong metadata; no `fault` edge may route it, #3863). `script` could not join +that seam while its legal key set depended on `actionType`: a flat parse would either +reject valid shapes or wave everything through. Converging the node is what made the +contract fit. `subflow`'s hand-written `flowName` check became the same parse, so its +message is now `subflow 'n1': config does not satisfy the subflow contract — +config.flowName: …`. `decision` deliberately stays export-only: its one key is optional, +so a parse would check nothing. + +**Migration.** `os migrate meta --from 16` rewrites stored sources; authoring one of these +keys in TypeScript is a compile error carrying the same prescription. A shorthand +`actionType` **converts into `function`** — that is what it named — unless `function` is +already set, in which case it was dead metadata the executor never reached. The other four +keys are dropped outright: nothing read them, so there is no value to preserve, and +rebuilding the intent is an authoring decision (the table above) rather than something a +mechanical rewrite can guess. + +The keys leave the **load path** (`retiredFromLoadPath`) with the rest of the keys retired +for *misdescribing themselves* rather than for being renamed: absorbing +`actionType: 'email'` silently would let an author keep believing the flow sends mail. The +one seam that still replays it is `registerFlow`, which rehydrates data at rest (#3903) — +a row in `sys_metadata` has no author for a tombstone to teach. So a stored email-stub node +arrives stripped of the keys nothing read and then **refuses for naming no callable**, +where it used to log a line and report success. That flip is the behavior change to expect. + +**A build gap this surfaced, fixed here.** `FlowFunctionEntrySchema` now also accepts a +**lowered handler ref** (a non-empty string), the form `objectstack build` produces: the +CLI lowers every inline callable to a serialisable ref *before* the stack is parsed (it +must — `z.function()` wraps callables and would break the ref mapping), so a built +manifest holds `{ myFn: 'myFn' }`, which neither previous member accepted. The result was +that `defineStack({ functions })` — a documented, first-class mechanism — could not +survive a build at all. Nothing had noticed because no bundled example used it; #4343 +turns that from latent into blocking, since `config.function` becomes the only thing a +`script` node can run. `Hook.handler` already declared exactly this pair (`z.union([ +z.string(), ])`, "string, post-build / inline function, pre-build"), so this +brings `functions` onto the platform's established shape rather than inventing one. A +string carries no callable and `normalizeFlowFunctionEntry` still drops it by design — the +real functions ride in the sibling ESM module the build emits, merged by name — so +hand-authoring one registers nothing and fails loudly at execute ("no function named '…' +is registered"), never silently. + +Also in this change: the retired constants `SCRIPT_BUILTIN_ACTION_TYPES`, +`SCRIPT_INVOKE_FUNCTION_ACTION_TYPE` and the `ScriptBuiltinActionType` type are removed +(they described the dispatch set that no longer exists); `os validate` names a retired key +and its replacement instead of reporting a generic missing callable; and the `#3796` +alias fixture, which carried `actionType: 'invoke_function'` through both sides, no longer +describes an end state protocol 17 can reach — the rename itself is untouched. No liveness +ledger row moves: the gate walks `FlowSchema`, whose `nodes[].config` is +`z.record(z.unknown())`, so these keys were never governed by one. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index eda67e7f47..dff6453466 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -113,7 +113,7 @@ Each node performs a specific action in the flow. | `get_record` | Query records | | `http` | Make an HTTP API call | | `notify` | Send an outbound notification via the messaging service | -| `script` | Call a named callable — a registered function (`config.function`) or a built-in side-effect marker (`config.actionType`) | +| `script` | Call a registered function named by `config.function` | | `screen` | Display a user form/screen (durable pause) | | `wait` | Pause for a timer or named signal (durable pause; timers auto-resume) | | `subflow` | Invoke another flow — a pause inside the child suspends both runs as a linked chain | @@ -214,25 +214,40 @@ or missing-`required` violation (#4277). A node type that publishes no **Script:** The built-in `script` executor never evaluates an arbitrary JavaScript string — -it **names a callable**. Two forms: - -- **`config.function`** — a function registered through - `defineStack({ functions })`. `config.inputs` is `{var}`-interpolated and - handed to it; `config.outputVariable` binds the returned value as a flow - variable, so a later declarative node persists it. This is the supported way - to run server logic. -- **`config.actionType`** — one of the two built-in side-effect markers, - `'email'` or `'slack'`. These are **logger-backed**: they record the intent - and succeed, they do not deliver anything — reach for a `notify` node when you - want real delivery. Any other `actionType` value is treated as a - registered-function name — except the marker `'invoke_function'`, which means - "call the function named in `config.function`" and errors if that key is - missing. - -Inline `config.script` (a JS source body) is *recognized* but **not executed** — -the built-in runtime has no server-side JS sandbox, so such a node warns and -no-ops. A script node that names neither a built-in action nor a registered -function fails the step loudly rather than passing silently. +it **calls a registered function**, and that is the whole of what it does. + +**`config.function`** names a function registered through +`defineStack({ functions })`, and it is **required**. `config.inputs` is +`{var}`-interpolated and handed to it; `config.outputVariable` binds the +returned value as a flow variable, so a later declarative node persists it. A +node that names no function refuses before it runs; one naming a function +nothing registered fails the step loudly rather than passing silently. + + + +`config.actionType`, `config.template`, `config.recipients`, `config.variables` +and inline `config.script` were removed in `@objectstack/spec` 17 ([#4343]). +None of them ran: the `'email'` / `'slack'` action types were **logger-backed +stubs** that recorded the intent, reported success and delivered nothing under +any configuration, and an inline JS body was recognized but never executed (the +built-in runtime has no server-side sandbox). Every other `actionType` value was +shorthand for a registered-function name. + +Replace them per branch — they are different mechanisms, not one rename: + +| Retired shape | Use instead | +| --- | --- | +| `actionType: 'email'` (+ `template` / `recipients` / `variables`) | a [`notify` node](#notify) — it delivers through the messaging service: the in-app inbox by default, real email once `@objectstack/plugin-email` is installed | +| `actionType: 'slack'` | a `connector_action` node with the Slack connector, or an `http` node posting to an incoming webhook | +| `actionType: 'my_fn'` (shorthand) | `function: 'my_fn'` — the conversion moves it for you | +| inline `config.script` | move the logic into a registered function and call it via `config.function` | + +Stored flows are rewritten by `os migrate meta --from 16`; authoring one of +these keys in TypeScript is now a compile error carrying the same prescription. + +[#4343]: https://github.com/objectstack-ai/objectstack/issues/4343 + + ```typescript { diff --git a/content/docs/references/automation/schemaless-node-config.mdx b/content/docs/references/automation/schemaless-node-config.mdx index 1bf30f2936..84b5990d25 100644 --- a/content/docs/references/automation/schemaless-node-config.mdx +++ b/content/docs/references/automation/schemaless-node-config.mdx @@ -19,21 +19,27 @@ form lives ONLY in objectui's hand-written `FLOW_NODE_CONFIG` table — with each member's reason: `decision`'s virtual Target column is derived from -the out-edges, `script`'s form switches on `actionType`, `subflow` carries a +the out-edges, `subflow` carries a top-level `timeoutMs` — a published -top-level `timeoutMs` — a published partial schema would DROP those editors +partial schema would DROP those editors (the #4210 `connector_action` -(the #4210 `connector_action` incident). So the Studio form for these types +incident). So the Studio form for these types is objectui's hand-written -is objectui's hand-written group, and until #4278 **nothing reconciled that +group, and until #4278 **nothing reconciled that hand-written table against -hand-written table against the executors**: `script`'s form offered an +the executors**: `script`'s form offered an `outputVariables` key nothing -`outputVariables` key nothing reads, two `actionType` options that fail every +reads, two `actionType` options that fail every run, a no-op default — and -run, a no-op default — and could not author the `function`/`inputs`/ +could not author the `function`/`inputs`/`outputVariable` path that works. -`outputVariable` path that works. +`script`'s own reason for staying schemaless was that its form switched on + +`actionType`. #4343 retired that switch, so the node is now three flat keys + +and could graduate to a published descriptor `configSchema` the way `map` + +did — a follow-up, deliberately not folded into the retirement. These schemas are the machine-readable half of that reconciliation. They are @@ -59,37 +65,57 @@ entry here: their contracts are the spec-structured sibling blocks on same objectui test reconciles directly. -## What these schemas are (and are not) wired to +## What these schemas are wired to + +`script` and `subflow` are **parsed at execute time** since #4343, through + +the same `parseNodeConfig()` seam #4277 gave the flat builtins + +(`service-automation`'s `parse-config.ts`): a config that fails its contract + +refuses the node as a GUARD — wrong metadata, so a rerun cannot help and no + +`fault` edge may route it (#3863). + +`script` could not be parsed while its legal key set depended on + +`actionType`; #4343 removed that dependence instead of modelling it. + +Converging the node to its one real path — call a registered function — left + +a flat three-key contract a flat parse fits exactly, and the five keys the + +other branches read became `retiredKey` tombstones. -Contract exports only — no engine path `parse()`s a node config with them, +The two halves reach different audiences, which is why they shipped together: -so registering a flow behaves exactly as before. This is where they differ +- the **tombstones** teach whoever authors the key — `tsc` types it `never`, -from their `builtin-node-config.zod.ts` siblings, which #4277 wired into +and a direct parse raises the prescription. They do NOT reach a stored -execute-time parsing (`service-automation`'s `parse-config.ts`) and into the +flow: `FlowNodeSchema.config` is `z.record(z.unknown())`, so no load-path -`registerFlow()` unknown-key rejection. +parse ever descends into a node's config; -That difference is deliberate, and it is the same reason these three publish +- the **execute-time parse** is what a stored flow meets. `registerFlow` -no descriptor `configSchema`: **their key set is not the whole contract.** +canonicalizes data at rest through the retired conversion too (#3903), so -`script`'s legal keys depend on `actionType` (a built-in side effect reads +a stored `actionType: 'email'` node arrives here stripped of the keys -`template`/`recipients`/`variables`; the function path reads +nothing read — and then refuses, naming the `function` it does not have, -`function`/`inputs`/`outputVariable`), and `decision` may carry no +instead of logging a line and reporting success as it used to. -`conditions` at all when it branches purely on edge predicates. A flat parse +`decision` stays export-only, deliberately: it may carry no `conditions` at -would either reject those shapes or wave everything through — neither is the +all when it branches purely on edge predicates (a plain BPMN exclusive -contract. Wiring them in needs a discriminated form first; until then the +gateway), and `conditions` is its only key — so a parse would have nothing -enforcement they DO get is the objectui reconciliation test, which is what +left to check. Its enforcement remains the objectui reconciliation test, -#4278 was actually about (a form authoring keys nothing reads). +which is what #4278 was actually about (a form authoring keys nothing reads). Undeclared aliases are NOT part of these contracts: `subflow`'s historical @@ -144,14 +170,14 @@ const result = DecisionCondition.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **actionType** | `string` | optional | How this step runs: a built-in side effect ('email' \| 'slack'), the 'invoke_function' marker, or shorthand for a registered-function name | -| **function** | `string` | optional | Registered function to call (defineStack(`{ functions }`)); takes precedence over actionType. Contractually pure — it returns a value a later declarative node persists | +| **function** | `string` | ✅ | Registered function to call (defineStack(`{ functions }`)). Contractually pure — it returns a value a later declarative node persists | | **inputs** | `Record` | optional | Inputs passed to the function (values interpolate `{token}` templates) | | **outputVariable** | `string` | optional | Flow variable the function's return value is bound to | -| **template** | `string` | optional | Built-in side effects only: message template id | -| **recipients** | `string[]` | optional | Built-in side effects only: recipients (user ids, field refs, or addresses) | -| **variables** | `Record` | optional | Built-in side effects only: values injected into the template | -| **script** | `string` | optional | Inline JS source — recognized but not executed by the built-in runtime; use a registered function via `function` instead | +| **actionType** | `any` | optional | [REMOVED] `script.config.actionType` was removed in @objectstack/spec 17 (#4343) — none of its values did what it said. The two built-ins were logger-backed stubs that recorded the intent and delivered nothing under any configuration, and every other value was a second spelling of `config.function`. Replace it per branch: for `email` use a `notify` node (it delivers through the messaging service — the in-app inbox by default, real email once `@objectstack/plugin-email` is installed); for `slack` use a `connector_action` node with the Slack connector, or an `http` node posting to a webhook; for anything else, move the name into `config.function`. Run `os migrate meta --from 16` to rewrite it automatically. | +| **template** | `any` | optional | [REMOVED] `script.config.template` was removed in @objectstack/spec 17 (#4343) — it fed only the logger-backed `email`/`slack` stubs, which never rendered or sent a message, so no template id was ever resolved. Delete the key. A `notify` node carries its own `title`/`message`, and stored templates live in the messaging service (`sys_notification_template`), not on the node. Run `os migrate meta --from 16` to rewrite it automatically. | +| **recipients** | `any` | optional | [REMOVED] `script.config.recipients` was removed in @objectstack/spec 17 (#4343) — the addresses were logged, never messaged: the `email`/`slack` branches it fed delivered nothing. Use a `notify` node, whose `recipients` (user ids, field refs or addresses) reach the messaging service for real. Run `os migrate meta --from 16` to rewrite it automatically. | +| **variables** | `any` | optional | [REMOVED] `script.config.variables` was removed in @objectstack/spec 17 (#4343) — it injected values into a template no side effect ever rendered. Delete the key. A `notify` node carries structured data in `payload`; a registered function takes it in `config.inputs`. Run `os migrate meta --from 16` to rewrite it automatically. | +| **script** | `any` | optional | [REMOVED] `script.config.script` was removed in @objectstack/spec 17 (#4343) — the built-in runtime has no server-side JS sandbox, so an inline body was recognized and never executed: the node warned and completed as a no-op. Move the logic into a registered function (`defineStack({ functions })`) and name it in `config.function`. Run `os migrate meta --from 16` to rewrite it automatically. | --- diff --git a/docs/audits/2026-07-unknown-key-strictness-ledger.md b/docs/audits/2026-07-unknown-key-strictness-ledger.md index d8104abe38..8498737c7f 100644 --- a/docs/audits/2026-07-unknown-key-strictness-ledger.md +++ b/docs/audits/2026-07-unknown-key-strictness-ledger.md @@ -208,7 +208,7 @@ tightening (the #4001 "sharing-rule lesson": candidates, not verdicts). | `node-executor.zod.ts` | 4 | wire | executor contract | | `io-node-config.zod.ts` | 2 | authorable | `NotifyConfigSchema` / `HttpConfigSchema` (#4045) — the sibling contracts that validate the **open** `config` slot on flow `notify` / `http` nodes. Authored per-node, so the open-slot exemption above does not extend to them; candidate once the executors' own drift is verified | | `builtin-node-config.zod.ts` | 8 | authorable | Same family (#4045): the CRUD quartet, `screen`, `map`. Written from what the executors read rather than from the descriptors' `configSchema` literals, and reconciled bidirectionally by `builtin-node-form-zod-ledger.test.ts` — so unlike most rows here, this one already has a drift check of its own. Same candidacy note as `io-node-config` | -| `schemaless-node-config.zod.ts` | 4 | authorable | Same family, third panel (#4278): `script` / `subflow` / `decision` (+ the decision branch item) — the descriptor-schemaless nodes whose form lives in objectui's hand-written table. Written from the executors; the drift check is objectui's `flow-node-config.spec-reconciliation` test (cross-repo, via the published exports). Contract exports only — nothing parses node config with them yet, so strictness candidacy follows `io-node-config` | +| `schemaless-node-config.zod.ts` | 4 | authorable | Same family, third panel (#4278): `script` / `subflow` / `decision` (+ the decision branch item) — the descriptor-schemaless nodes whose form lives in objectui's hand-written table. Written from the executors; the drift check is objectui's `flow-node-config.spec-reconciliation` test (cross-repo, via the published exports). Since #4343 `script` and `subflow` ARE parsed at execute time (`parse-config.ts`) — `script` once retiring its `actionType` branches left it flat — so strictness candidacy now follows `io-node-config` on the same terms rather than being moot; `decision` stays export-only | | `webhook.zod.ts` | 1 | authorable (p) | spec-only (#3461) | | `flow-function.zod.ts` | 1 | authorable | `FlowFunctionDeclarationSchema` (#4396) — the `{ handler, effect }` form of a `defineStack({ functions })` entry. Authored, but note what an undeclared key here would be: a sibling of a **live function**, not data. `defineStack`'s union already rejects a record whose `handler` is not callable, and the boot-path reader is the hand-written `normalizeFlowFunctionEntry` rather than a `.parse()` (re-validating a live handler every boot buys nothing), so strictness would bind at authoring only. Candidate on the same verify-first rule as its `*-node-config` neighbours | diff --git a/docs/protocol-upgrade-guide.md b/docs/protocol-upgrade-guide.md index 5510962613..9dfa080f4e 100644 --- a/docs/protocol-upgrade-guide.md +++ b/docs/protocol-upgrade-guide.md @@ -158,6 +158,8 @@ The same kind of retirement covers `wait`'s timeout pair (#4158). `waitEventConf Closing the same audit on the data side, `datasource.readReplicas` is removed (#4468). It described replica connections nothing ever opened: `ConnectableDatasource` and `DatasourceConnectionSpec` carry no replicas field, the driver factory never reads the key, and no query path distinguishes a read from a write — read/write splitting does not exist in the platform, so every statement always went to the primary. A lossless delete with no target to move to; front replicas behind one endpoint (pgpool, ProxySQL, an RDS reader endpoint) and point `config` at it. Notable as the case that shows how a key gets MORE convincing as it stays dead: #4410, closing the datasource-config gap, taught the schema to validate each replica entry against the declared driver's config contract, so sources written in between carry replica blocks that were genuinely checked — precise hosts, correct port types, typos rejected. Precision applied to an inert slot reads as evidence the slot is live, which is why ADR-0049 asks for a consumer rather than for rigor. Retired from the load path with the rest of the keys that misdescribed themselves. +The `script` flow node converges on its one real path (#4343). It had four ways to name what it ran and only one of them ran anything: `config.actionType: 'email' | 'slack'` were logger-backed stubs that wrote a line, reported success and delivered nothing under any configuration — with `config.template` / `.recipients` / `.variables` feeding a message no channel ever sent; inline `config.script` was recognized and never executed (the built-in runtime has no server-side JS sandbox), so the node warned and no-op'd; and every other `actionType` value was shorthand for a registered-function name, a second spelling of `config.function`. All five keys are retired and `function` becomes required, which is also what finally made the contract PARSEABLE: while the legal key set depended on `actionType`, a flat parse would either reject valid shapes or wave everything through, so `script` (with `subflow`) now runs through the same execute-time contract parse #4277 gave the flat builtins. A shorthand `actionType` CONVERTS into `function` — that is what it meant — unless `function` is already set, in which case it was dead metadata the executor never reached. The other four are dropped outright: nothing read them, so there is no value to preserve, and rebuilding the intent is an authoring decision the tombstones prescribe per branch (a `notify` node for mail — it delivers through the messaging service, the in-app inbox by default and real email once `@objectstack/plugin-email` is installed; a `connector_action` with the Slack connector, or an `http` node posting to a webhook, for Slack; a registered function for an inline body). Retired from the load path for the same reason as the rest: absorbing `actionType: 'email'` silently would let an author keep believing the flow sends mail. + ### Mechanical (applied for you) | Conversion | Surface | Change | Load window | @@ -186,6 +188,7 @@ Closing the same audit on the data side, `datasource.readReplicas` is removed (# | `stack-api-require-auth-removed` | `stack.api.requireAuth` | stack key 'api.requireAuth' removed — anonymous access is always denied; publish public surfaces by declaration (#3963) | retired — `migrate meta` only | | `flow-node-wait-timeout-keys-removed` | `flow.node.waitEventConfig` | waitEventConfig keys 'timeoutMs' (→ 'timerDuration', stringified — its only reader used it as the duration) and 'onTimeout' (removed — zero readers, so no timeout ever fired) (#4158) | retired — `migrate meta` only | | `datasource-read-replicas-removed` | `datasource.readReplicas` | datasource key 'readReplicas' removed (#4468 — no driver opened a replica connection and no query path splits reads from writes; front replicas behind one endpoint and point `config` at it) | retired — `migrate meta` only | +| `flow-node-script-branch-keys-removed` | `flow.node.script.config.actionType / flow.node.script.config.template / flow.node.script.config.recipients / flow.node.script.config.variables / flow.node.script.config.script` | script flow-node config keys 'actionType' (→ 'function' when it was shorthand for one; otherwise removed — 'email'/'slack' were logger-backed stubs that delivered nothing), plus 'template' / 'recipients' / 'variables' (fed those stubs) and 'script' (inline JS the runtime never executed) (#4343) | retired — `migrate meta` only | ### Semantic (delegated to you, with acceptance criteria) diff --git a/examples/app-showcase/objectstack.config.ts b/examples/app-showcase/objectstack.config.ts index f5ddb1d9ec..bad1354a4e 100644 --- a/examples/app-showcase/objectstack.config.ts +++ b/examples/app-showcase/objectstack.config.ts @@ -204,6 +204,15 @@ export default defineStack({ // Logic flows: allFlows, + // Named callables a `script` flow node invokes (#1870). Since #4343 that is + // the ONLY thing a script node does, so this map is what makes one runnable. + // A flow function is PURE: it takes `inputs`, RETURNS a value, and a later + // declarative node uses or persists it — it does no data I/O of its own + // (#4396), which is why it needs no `effect` declaration here. + functions: { + summarizeCompletedTask: ({ input }: { input: Record }) => + `Completed: ${String(input.title ?? 'task')} (priority ${String(input.priority ?? 'normal')}).`, + }, jobs: allJobs, emailTemplates: allEmails, // Declarative REST endpoints (object_operation + flow) — the metadata diff --git a/examples/app-showcase/src/automation/flows/index.ts b/examples/app-showcase/src/automation/flows/index.ts index 70fcc6f4b8..3c467ca620 100644 --- a/examples/app-showcase/src/automation/flows/index.ts +++ b/examples/app-showcase/src/automation/flows/index.ts @@ -6,14 +6,28 @@ import { ApproverBindingsFlow } from './approver-bindings.flow'; /** * Task Completed → Notify — an autolaunched, record-triggered flow that fires - * when a task transitions to Done and emails the project owner. + * when a task transitions to Done, composes a one-line summary in a registered + * function, and notifies the assignee. + * + * It also carries the two node types #4343 sorted out from each other: + * + * - **`script`** calls a registered function (`defineStack({ functions })`) and + * binds its RETURN value to a flow variable. That is the whole of what the + * node does now — the `actionType` side effects it used to offer were + * logger-backed stubs that delivered nothing. + * - **`notify`** is the real delivery mechanism: it hands the messaging service + * the notification (the in-app inbox by default, email once + * `@objectstack/plugin-email` is installed). */ export const TaskCompletedFlow = defineFlow({ name: 'showcase_task_completed', label: 'Notify on Task Completed', - description: 'Emails the project owner when a task is marked Done.', + description: 'Summarizes a completed task in a registered function, then notifies its assignee.', type: 'autolaunched', status: 'active', + variables: [ + { name: 'summary', type: 'string', isInput: false, isOutput: false }, + ], nodes: [ { id: 'start', @@ -26,23 +40,38 @@ export const TaskCompletedFlow = defineFlow({ }, }, { - id: 'notify', + id: 'summarize', type: 'script', - label: 'Send Completion Email', + label: 'Compose Summary', config: { - actionType: 'email', - inputs: { - to: '{record.project.owner}', - subject: '✅ Task done: {record.title}', - template: 'showcase_task_done_email', - }, + // Registered in `defineStack({ functions })` — see objectstack.config.ts. + // A flow function is PURE: it takes `inputs`, RETURNS a value, and a + // later declarative node uses or persists it (#4396). + function: 'summarizeCompletedTask', + inputs: { title: '{record.title}', priority: '{record.priority}' }, + outputVariable: 'summary', + }, + }, + { + id: 'notify', + type: 'notify', + label: 'Notify the assignee', + config: { + // A field ON the record: the flow record carries `project` as a scalar + // id, so `{record.project.owner}` would resolve to an empty string. + recipients: '{record.assignee}', + title: '✅ Task done: {record.title}', + message: '{summary}', + sourceObject: 'showcase_task', + sourceId: '{record.id}', }, }, { id: 'end', type: 'end', label: 'End' }, ], edges: [ - { id: 'e1', source: 'start', target: 'notify' }, - { id: 'e2', source: 'notify', target: 'end' }, + { id: 'e1', source: 'start', target: 'summarize' }, + { id: 'e2', source: 'summarize', target: 'notify' }, + { id: 'e3', source: 'notify', target: 'end' }, ], }); @@ -819,15 +848,13 @@ export const BatchRemindersFlow = defineFlow({ nodes: [ { id: 'send_reminder', - type: 'script', + type: 'notify', label: 'Send Reminder', config: { - actionType: 'email', - inputs: { - to: '{task.owner.email}', - subject: 'Reminder ({taskIndex}): {task.title}', - template: 'showcase_task_reminder_email', - }, + recipients: '{task.owner}', + title: 'Reminder ({taskIndex}): {task.title}', + sourceObject: 'showcase_task', + sourceId: '{task.id}', }, }, ], @@ -876,30 +903,37 @@ export const FanOutNotifyFlow = defineFlow({ config: { branches: [ { - name: 'Email the owner', + name: 'Notify the owner', nodes: [ { - id: 'email_owner', - type: 'script', - label: 'Email Owner', + id: 'notify_owner', + type: 'notify', + label: 'Notify Owner', config: { - actionType: 'email', - inputs: { to: '{record.project.owner}', subject: '✅ Done: {record.title}' }, + recipients: '{record.assignee}', + title: '✅ Done: {record.title}', + sourceObject: 'showcase_task', + sourceId: '{record.id}', }, }, ], edges: [], }, { + // Slack is a CONNECTOR, not a notify channel (#4343): post through + // an incoming webhook, or a `connector_action` with the Slack + // connector. The retired `script` + `actionType: 'slack'` shape + // logged a line and delivered nothing. name: 'Post to Slack', nodes: [ { id: 'slack_post', - type: 'script', + type: 'http', label: 'Slack Notify', config: { - actionType: 'slack', - inputs: { channel: '#tasks', text: 'Task done: {record.title}' }, + url: 'https://hooks.slack.com/services/T000/B000/XXXX', + method: 'POST', + body: { channel: '#tasks', text: 'Task done: {record.title}' }, }, }, ], @@ -1123,12 +1157,12 @@ export const ProjectEscalationFlow = defineFlow({ branches: [ { name: 'Owner', - nodes: [{ id: 'alert_owner', type: 'script', label: 'Alert Owner', config: { actionType: 'email', inputs: { to: '{record.owner}', subject: '🔴 Critical: {record.name}' } } }], + nodes: [{ id: 'alert_owner', type: 'notify', label: 'Alert Owner', config: { recipients: '{record.owner}', title: '🔴 Critical: {record.name}', severity: 'critical', sourceObject: 'showcase_project', sourceId: '{record.id}' } }], edges: [], }, { name: 'Exec', - nodes: [{ id: 'alert_exec', type: 'script', label: 'Alert Exec', config: { actionType: 'email', inputs: { to: 'exec@example.com', subject: '🔴 Critical project: {record.name}' } } }], + nodes: [{ id: 'alert_exec', type: 'notify', label: 'Alert Exec', config: { recipients: 'exec@example.com', title: '🔴 Critical project: {record.name}', severity: 'critical', sourceObject: 'showcase_project', sourceId: '{record.id}' } }], edges: [], }, ], diff --git a/examples/app-todo/src/flows/task.flow.ts b/examples/app-todo/src/flows/task.flow.ts index ebdf1d087c..cd75dccc63 100644 --- a/examples/app-todo/src/flows/task.flow.ts +++ b/examples/app-todo/src/flows/task.flow.ts @@ -30,15 +30,17 @@ export const TaskReminderFlow: Flow = { config: { collection: '{tasksToRemind}', iteratorVariable: 'currentTask' }, }, { - id: 'send_reminder', type: 'script', label: 'Send Reminder Email', + // `notify` is what actually delivers (#4343): it hands the messaging + // service the notification — the in-app inbox by default, and email once + // `@objectstack/plugin-email` is installed. The `script` node this + // replaced only ever logged a line and reported success. + id: 'send_reminder', type: 'notify', label: 'Send Reminder', config: { - actionType: 'email', - inputs: { - to: '{currentTask.owner.email}', - subject: 'Task Due Tomorrow: {currentTask.subject}', - template: 'task_reminder_email', - data: { taskSubject: '{currentTask.subject}', dueDate: '{currentTask.due_date}', priority: '{currentTask.priority}' }, - }, + recipients: '{currentTask.owner}', + title: 'Task due tomorrow: {currentTask.subject}', + message: 'Due {currentTask.due_date} · priority {currentTask.priority}.', + sourceObject: 'todo_task', + sourceId: '{currentTask.id}', }, }, { id: 'end', type: 'end', label: 'End' }, @@ -90,15 +92,14 @@ export const OverdueEscalationFlow: Flow = { }, }, { - id: 'notify_owner', type: 'script', label: 'Notify Task Owner', + id: 'notify_owner', type: 'notify', label: 'Notify Task Owner', config: { - actionType: 'email', - inputs: { - to: '{currentTask.owner.email}', - subject: 'URGENT: Task Overdue - {currentTask.subject}', - template: 'overdue_escalation_email', - data: { taskSubject: '{currentTask.subject}', dueDate: '{currentTask.due_date}', daysOverdue: '{currentTask.days_overdue}' }, - }, + recipients: '{currentTask.owner}', + title: 'URGENT: task overdue — {currentTask.subject}', + message: 'Due {currentTask.due_date}, {currentTask.days_overdue} day(s) overdue.', + severity: 'critical', + sourceObject: 'todo_task', + sourceId: '{currentTask.id}', }, }, { id: 'end', type: 'end', label: 'End' }, diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 9e8d98456f..e0e5e3daf9 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -68,31 +68,29 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); // #1870 — a `script` node that names no callable is a silent no-op. - it('flags a script node that declares neither actionType nor function (#1870)', () => { + it('flags a script node that declares no function (#1870)', () => { const issues = validateStackExpressions({ flows: [{ name: 'helpdesk_flow', nodes: [ { id: 'start', type: 'start', config: {} }, - { id: 'triage', type: 'script', config: { actionType: undefined } }, + { id: 'triage', type: 'script', config: {} }, ], edges: [], }], }); expect(issues).toHaveLength(1); expect(issues[0].where).toContain("node 'triage' (script) callable"); - expect(issues[0].message).toMatch(/neither .*actionType.* nor .*function/); + expect(issues[0].message).toMatch(/declares no .*function/); }); - it('accepts a script node that names a built-in action or a function (#1870)', () => { + it('accepts a script node that names a function (#1870)', () => { const issues = validateStackExpressions({ flows: [{ name: 'helpdesk_flow', nodes: [ { id: 'start', type: 'start', config: {} }, - { id: 'mail', type: 'script', config: { actionType: 'email' } }, { id: 'triage', type: 'script', config: { function: 'helpdesk.aiTriageStub' } }, - { id: 'inline', type: 'script', config: { script: 'variables.x = 1;' } }, ], edges: [], }], @@ -107,7 +105,7 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { name: 'helpdesk_flow', nodes: [ { id: 'start', type: 'start', config: {} }, - { id: 'triage', type: 'script', config: { actionType: 'invoke_function', functionName: 'helpdesk.aiTriageStub' } }, + { id: 'triage', type: 'script', config: { functionName: 'helpdesk.aiTriageStub' } }, ], edges: [], }], @@ -115,19 +113,56 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues).toHaveLength(0); }); - it('flags actionType invoke_function with no function/functionName', () => { + // #4343 — the retired dispatch keys. Naming them beats the generic "no + // callable": they are what the author actually wrote, and each branch has a + // different replacement. + it('flags a retired dispatch key and prescribes the replacement mechanism', () => { + const issues = validateStackExpressions({ + flows: [{ + name: 'helpdesk_flow', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'mail', type: 'script', config: { actionType: 'email', template: 't', recipients: ['a'] } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).toMatch(/#4343/); + expect(issues[0].message).toMatch(/config\.actionType/); + expect(issues[0].message).toMatch(/config\.template/); + expect(issues[0].message).toMatch(/`notify` node/); + expect(issues[0].message).toMatch(/os migrate meta --from 16/); + }); + + it('tells a shorthand actionType exactly where its name belongs', () => { const issues = validateStackExpressions({ flows: [{ name: 'helpdesk_flow', nodes: [ { id: 'start', type: 'start', config: {} }, - { id: 'triage', type: 'script', config: { actionType: 'invoke_function', inputs: { x: 1 } } }, + { id: 'triage', type: 'script', config: { actionType: 'helpdesk.aiTriageStub' } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].message).toMatch(/function: 'helpdesk\.aiTriageStub'/); + }); + + it('flags an inline script body — the runtime never executed it', () => { + const issues = validateStackExpressions({ + flows: [{ + name: 'helpdesk_flow', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { id: 'inline', type: 'script', config: { script: 'variables.x = 1;' } }, ], edges: [], }], }); expect(issues).toHaveLength(1); - expect(issues[0].message).toMatch(/invoke_function.*no .*function/i); + expect(issues[0].message).toMatch(/config\.script/); }); // #1928 — bare field references are silently null in `record`-scoped sites diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 82758483f3..37d4b928bc 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -168,11 +168,12 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { found.value, ); } - // #1870 — a `script` node must declare a callable target (`actionType` or - // `function`). A node with neither is a silent no-op that otherwise passes - // build. (Function *existence* isn't checkable here — functions are code, - // not serialized into the artifact — so this is a structural check; the - // runtime verifies the named function is actually registered.) + // #1870 — a `script` node must name a callable, and since #4343 that is + // the whole of what the node does: `config.function`. A node without one + // is a silent no-op that otherwise passes build. (Function *existence* + // isn't checkable here — functions are code, not serialized into the + // artifact — so this is a structural check; the runtime verifies the + // named function is actually registered.) if (node.type === 'script') { // `function` is canonical; a pre-parse source may still carry the // `functionName` alias during the protocol-17 window, until the @@ -180,28 +181,34 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const fn = (typeof cfg.function === 'string' ? cfg.function.trim() : '') || (typeof cfg.functionName === 'string' ? cfg.functionName.trim() : ''); + // A source that predates #4343 may still carry a retired dispatch key. + // Naming it beats the generic "no callable": these ARE what the author + // wrote, and each has a different replacement. The schema tombstones + // carry the full prescription; this is the one-line version at lint. const action = typeof cfg.actionType === 'string' ? cfg.actionType.trim() : ''; - // Inline `config.script` (a JS body) is also a declared form — the - // built-in runtime doesn't execute it (warned at run time), but the node - // is not the empty no-op this check targets, so don't flag it. - const inline = typeof cfg.script === 'string' ? cfg.script.trim() : ''; - if (!fn && !action && !inline) { + const retired = ['actionType', 'template', 'recipients', 'variables', 'script'] + .filter((k) => cfg[k] != null); + if (retired.length > 0) { issues.push({ where: `${at} · node '${node.id}' (script) callable`, message: - `script node declares neither \`actionType\` nor \`function\` — it would do nothing at runtime. ` + - `Name a built-in action (e.g. \`actionType: 'email'\`) or a registered function ` + - `(\`function: 'my_fn'\`, registered via \`defineStack({ functions })\`).`, + `script node carries \`${retired.map((k) => `config.${k}`).join('`, `')}\` — retired in ` + + `@objectstack/spec 17 (#4343). The built-in 'email'/'slack' actions were logger-backed ` + + `stubs that delivered nothing, and inline \`config.script\` was never executed. ` + + (action && action !== 'invoke_function' && !['email', 'slack'].includes(action) + ? `\`actionType: '${action}'\` named a registered function — move it to \`function: '${action}'\`. ` + : `Use a \`notify\` node for mail, a \`connector_action\` (Slack connector) or \`http\` node ` + + `for Slack, and a registered function for logic. `) + + `Run \`os migrate meta --from 16\` to rewrite it automatically.`, source: JSON.stringify({ id: node.id, type: node.type, config: cfg }), }); - } else if (action === 'invoke_function' && !fn) { - // `actionType: 'invoke_function'` is a marker that names no callable on - // its own — the function name must be in `function`/`functionName`. + } else if (!fn) { issues.push({ where: `${at} · node '${node.id}' (script) callable`, message: - `script node uses \`actionType: 'invoke_function'\` but no \`function\` (or \`functionName\`) — ` + - `it names no callable. Set \`function: 'my_fn'\` and register it via \`defineStack({ functions })\`.`, + `script node declares no \`function\` — it would do nothing at runtime. ` + + `Name a registered function (\`function: 'my_fn'\`, registered via ` + + `\`defineStack({ functions })\`).`, source: JSON.stringify({ id: node.id, type: node.type, config: cfg }), }); } diff --git a/packages/services/service-automation/src/builtin/config-parse.test.ts b/packages/services/service-automation/src/builtin/config-parse.test.ts index 0b7bc74316..bacac967f0 100644 --- a/packages/services/service-automation/src/builtin/config-parse.test.ts +++ b/packages/services/service-automation/src/builtin/config-parse.test.ts @@ -19,6 +19,11 @@ * 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. + * + * #4343 added the two schemaless nodes whose contracts could carry the same + * seam: `subflow` (flat all along — it just had a hand-written guard) and + * `script`, once retiring its non-functional dispatch branches left it flat. + * `decision` stays out: its one key is optional, so a parse would check nothing. */ import { describe, it, expect } from 'vitest'; @@ -204,4 +209,87 @@ describe('execute-time config parse (#4277)', () => { expect(result.error).toContain('map'); expect(result.error).toContain('config.collection'); }); + + // ── the two schemaless nodes that joined the seam in #4343 ────────────── + // + // `script` could not be parsed while its legal key set depended on + // `actionType`; converging it to a function call (retiring the branches that + // never delivered anything) is what made a flat parse fit. `subflow` was + // always flat — it just carried a hand-written guard instead of the contract. + + it('script refuses a node that names no callable', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('script', {})); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('does not satisfy the script contract'); + expect(result.error).toContain('config.function'); + }); + + it('script refuses a retired email stub — it used to log a line and report success', async () => { + const engine = engineWith(); + // `registerFlow` strips the retired keys on rehydration (#3903), so what + // reaches the parse is a node with nothing to run. Before #4343 this was a + // green step that delivered no mail. + engine.registerFlow('f', flowWith('script', { + actionType: 'email', template: 'task_done', recipients: ['{record.owner}'], + })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('does not satisfy the script contract'); + }); + + it('a script parse refusal is a guard — a fault edge does NOT route it', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith( + 'script', + { actionType: 'slack', template: 't' }, + { + 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'); + expect(result.success).toBe(false); + expect(result.error).toContain('does not satisfy the script contract'); + }); + + it('an unresolvable function name stays ROUTABLE — the registry is the host, not the metadata', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith( + 'script', + { function: 'never_registered' }, + { + nodes: [{ id: 'recover', type: 'assignment', label: 'R', config: { recovered: true } }], + edges: [{ id: 'e3', source: 'n1', target: 'recover', type: 'fault' }], + }, + )); + + // The negative half of the contract (#3863): the same flow succeeds on a + // host that registers the name, so the author must be able to handle it. + const result = await engine.execute('f'); + expect(result.success).toBe(true); + }); + + it('subflow refuses a missing flowName through the contract, not a hand-written check', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('subflow', {})); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('does not satisfy the subflow contract'); + expect(result.error).toContain('config.flowName'); + }); + + it('subflow refuses an empty flowName — declared is not the same as named', async () => { + const engine = engineWith(); + engine.registerFlow('f', flowWith('subflow', { flowName: '' })); + + const result = await engine.execute('f'); + expect(result.success).toBe(false); + expect(result.error).toContain('config.flowName'); + }); }); diff --git a/packages/services/service-automation/src/builtin/screen-nodes.test.ts b/packages/services/service-automation/src/builtin/screen-nodes.test.ts index 6b2652f95c..d8c5719098 100644 --- a/packages/services/service-automation/src/builtin/screen-nodes.test.ts +++ b/packages/services/service-automation/src/builtin/screen-nodes.test.ts @@ -1,11 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect, beforeEach } from 'vitest'; -import { - SCRIPT_BUILTIN_ACTION_TYPES, - SCRIPT_INVOKE_FUNCTION_ACTION_TYPE, - ScriptConfigSchema, -} from '@objectstack/spec/automation'; +import { ScriptConfigSchema } from '@objectstack/spec/automation'; import { AutomationEngine, type FlowFunctionHandler } from '../engine.js'; import { registerScreenNodes } from './screen-nodes.js'; @@ -49,12 +45,6 @@ describe('script node (#1870 — callable resolution)', () => { registerScreenNodes(engine, createCtx()); }); - it('runs the built-in email side-effect', async () => { - engine.registerFlow('script_flow', scriptFlow({ actionType: 'email', template: 't', recipients: ['a'] })); - const result = await engine.execute('script_flow', {} as any); - expect(result.success).toBe(true); - }); - it('invokes a registered function and captures its return value as output', async () => { const calls: Array> = []; const fn: FlowFunctionHandler = (c) => { @@ -73,36 +63,16 @@ describe('script node (#1870 — callable resolution)', () => { expect(calls).toEqual([{ ticket: 't_1' }]); }); - it('resolves a bare actionType that matches no built-in as a function name', async () => { - let called = false; - engine.setFunctionResolver((name) => (name === 'pm.aiRiskAssessmentStub' ? (() => { called = true; return 1; }) : undefined)); - engine.registerFlow('script_flow', scriptFlow({ actionType: 'pm.aiRiskAssessmentStub' })); - const result = await engine.execute('script_flow', {} as any); - expect(result.success).toBe(true); - expect(called).toBe(true); - }); - it('FAILS LOUDLY for an unregistered function instead of silently no-op (#1870)', async () => { // No resolver wired → nothing resolves. engine.registerFlow('script_flow', scriptFlow({ function: 'helpdesk.aiTriageStub' })); const result = await engine.execute('script_flow', {} as any); expect(result.success).toBe(false); expect(result.error).toMatch(/aiTriageStub/); - expect(result.error).toMatch(/no function named|not a built-in/i); - }); - - it('recognizes inline config.script as a no-op (not a loud failure) — built-in runtime has no JS sandbox', async () => { - engine.registerFlow('script_flow', scriptFlow({ script: 'variables.x = 1;', outputVariables: ['x'] })); - const result = await engine.execute('script_flow', {} as any); - // Recognized form: succeeds (doesn't fail loud), but is documented as not executed. - expect(result.success).toBe(true); - }); - - it('FAILS LOUDLY when the script node declares no target at all (actionType: undefined repro)', async () => { - engine.registerFlow('script_flow', scriptFlow({ actionType: undefined })); - const result = await engine.execute('script_flow', {} as any); - expect(result.success).toBe(false); - expect(result.error).toMatch(/neither .*actionType.* nor .*function|nothing to run/i); + expect(result.error).toMatch(/no function named/i); + // It stays a ROUTABLE failure, not a guard: the function registry is the + // host's, so the same metadata succeeds where the name is registered + // (#3863). config-parse.test.ts pins that with a fault edge. }); it('surfaces a thrown function as a loud step failure', async () => { @@ -119,20 +89,11 @@ it('canonicalizes a stored `functionName` key to `function` at load (#1870 DX, # let calledWith: any; engine.setFunctionResolver((name) => name === 'helpdesk.aiTriageStub' ? ((c: any) => { calledWith = c.input; return { triaged: true }; }) : undefined); - engine.registerFlow('script_flow', scriptFlow({ actionType: 'invoke_function', functionName: 'helpdesk.aiTriageStub', inputs: { ticketId: 't1' } })); + engine.registerFlow('script_flow', scriptFlow({ functionName: 'helpdesk.aiTriageStub', inputs: { ticketId: 't1' } })); const r = await engine.execute('script_flow', {} as any); expect(r.success).toBe(true); expect(calledWith).toEqual({ ticketId: 't1' }); }); - - it('treats actionType invoke_function as a marker, not a function name', async () => { - // invoke_function alone (no `function`) must NOT try to resolve a - // function literally named 'invoke_function'; it fails with a clear message. - engine.registerFlow('script_flow', scriptFlow({ actionType: 'invoke_function' })); - const r = await engine.execute('script_flow', {} as any); - expect(r.success).toBe(false); - expect(r.error).toMatch(/invoke_function.*requires.*function/i); - }); it('exposes the function result via outputVariable for downstream nodes (pure-function pattern)', async () => { const seen: Array> = []; engine.setFunctionResolver((name) => { @@ -161,61 +122,72 @@ it('canonicalizes a stored `functionName` key to `function` at load (#1870 DX, # }); /** - * #4278 — the script node's contract is the spec-published one. The designer - * form for `script` is objectui's hand-written group (this node deliberately - * publishes no descriptor configSchema — config-schemas.test.ts), so the only - * machine-readable statement of what it accepts is - * `SCRIPT_BUILTIN_ACTION_TYPES` / `ScriptConfigSchema` in - * `@objectstack/spec/automation`. These pins are the objectstack half of the - * cross-repo reconciliation: the executor dispatches exactly the published - * built-in set (it now builds its dispatch set FROM the constant), and its - * failure message names that same set — objectui's side reconciles its form - * options and key set against the same exports. + * #4343 — what a STORED flow carrying a retired dispatch branch does now. + * + * The retirement has two channels and they reach different people. The + * `retiredKey()` tombstones teach whoever *authors* the key: `tsc` types it + * `never`, and a direct `ScriptConfigSchema` parse raises the prescription. + * They never reach a stored flow — `FlowNodeSchema.config` is + * `z.record(z.unknown())`, so no load-path parse descends into a node's config. + * + * A stored flow meets the other channel. `registerFlow` canonicalizes data at + * rest through the RETIRED conversion too (#3903 — a row in `sys_metadata` has + * no author for a tombstone to teach), so the doomed keys are stripped on + * rehydration with a logged notice, and the execute-time parse then judges + * what is left. For the branches that never delivered anything, what is left + * names no callable — so the node refuses, loudly, where it used to log a line + * and report success. That flip is the whole point of the retirement, and it is + * what these cases pin. */ -describe('script contract ↔ spec-published constants (#4278)', () => { +describe('script retired branches, as a stored flow meets them (#4343)', () => { let engine: AutomationEngine; beforeEach(() => { engine = new AutomationEngine(createTestLogger()); registerScreenNodes(engine, createCtx()); + engine.setFunctionResolver((name) => (name === 'score_lead' ? (() => 1) : undefined)); }); - it.each([...SCRIPT_BUILTIN_ACTION_TYPES])( - "every published built-in actionType runs the built-in branch: '%s'", - async (actionType) => { - engine.registerFlow('script_flow', scriptFlow({ actionType, template: 't', recipients: ['a'] })); - const result = await engine.execute('script_flow', {} as any); - expect(result.success).toBe(true); - }, - ); - - it('an actionType outside the published set fails naming exactly that set (the #4278 sms repro)', async () => { - // The old objectui form offered 'sms' / 'notification'; neither is in - // the published set, so they resolve as function names and fail. The - // error must name the published members — it is the message the #4278 - // report quoted, and the form's options now come from the same constant. - engine.registerFlow('script_flow', scriptFlow({ actionType: 'sms' })); + it.each([ + ['the email stub', { actionType: 'email', template: 't', recipients: ['a'], variables: { x: 1 } }], + ['the slack stub', { actionType: 'slack', template: 't', recipients: ['#tasks'] }], + ['the example shape, payload in `inputs`', { actionType: 'email', inputs: { to: 'a@b.c' } }], + ['an inline body', { script: 'return { ok: true };' }], + ['the bare marker', { actionType: 'invoke_function' }], + ] as const)('%s no longer succeeds silently — it refuses, naming the callable it lacks', async (_name, config) => { + engine.registerFlow('script_flow', scriptFlow({ ...config })); const result = await engine.execute('script_flow', {} as any); expect(result.success).toBe(false); - for (const builtin of SCRIPT_BUILTIN_ACTION_TYPES) { - expect(result.error).toContain(builtin); - } - expect(result.error).toMatch(/'sms' is not a built-in action/); + expect(result.error).toContain('does not satisfy the script contract'); + expect(result.error).toContain('config.function'); + }); + + it('converts a shorthand `actionType` into the function it always named, and runs it', async () => { + // The one retired value carrying real intent: `actionType` that matched + // no built-in was a function name (#1870), so the conversion moves it + // rather than dropping it — and the node keeps working. + engine.registerFlow('script_flow', scriptFlow({ actionType: 'score_lead' })); + const result = await engine.execute('script_flow', {} as any); + expect(result.success).toBe(true); }); - it('the published Zod accepts the canonical authoring shapes (contract sanity)', () => { - // Function path — the only shape that does real work. + it('accepts the converged shape', async () => { + engine.registerFlow('script_flow', scriptFlow({ function: 'score_lead' })); + const result = await engine.execute('script_flow', {} as any); + expect(result.success).toBe(true); + }); + + it('the tombstones still refuse an AUTHORED key outright, prescription and all', () => { + // The other channel: no conversion runs in front of a direct parse, so + // this is what `tsc` and `os validate` put in front of an author. + const result = ScriptConfigSchema.safeParse({ function: 'score_lead', actionType: 'email' }); + expect(result.success).toBe(false); + expect(result.error!.issues[0]!.message).toMatch(/#4343/); expect(ScriptConfigSchema.parse({ - actionType: SCRIPT_INVOKE_FUNCTION_ACTION_TYPE, function: 'score_lead', inputs: { leadId: '{record.id}' }, outputVariable: 'score', })).toMatchObject({ function: 'score_lead' }); - // Built-in side effect. - expect(ScriptConfigSchema.parse({ actionType: 'email', template: 't', recipients: ['a'], variables: { x: 1 } })) - .toMatchObject({ actionType: 'email' }); - // Inline script — recognized (and documented as not executed). - expect(ScriptConfigSchema.parse({ script: 'return 1;' })).toMatchObject({ script: 'return 1;' }); }); }); diff --git a/packages/services/service-automation/src/builtin/screen-nodes.ts b/packages/services/service-automation/src/builtin/screen-nodes.ts index 25b1003f26..9f2ca5accb 100644 --- a/packages/services/service-automation/src/builtin/screen-nodes.ts +++ b/packages/services/service-automation/src/builtin/screen-nodes.ts @@ -1,8 +1,8 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { PluginContext } from '@objectstack/core'; -import { defineActionDescriptor, ScreenConfigSchema, SCRIPT_BUILTIN_ACTION_TYPES } from '@objectstack/spec/automation'; -import type { ScreenConfigParsed } from '@objectstack/spec/automation'; +import { defineActionDescriptor, ScreenConfigSchema, ScriptConfigSchema } from '@objectstack/spec/automation'; +import type { ScreenConfigParsed, ScriptConfigParsed } from '@objectstack/spec/automation'; import type { AutomationEngine } from '../engine.js'; import { interpolate } from './template.js'; import { parseNodeConfig } from './parse-config.js'; @@ -19,28 +19,24 @@ import { parseNodeConfig } from './parse-config.js'; * as bare flow variables). A field-less screen — or one with * `waitForInput === false` — stays a server pass-through (input vars, if any, * are already injected from `context.params`). - * - 'script' nodes name a callable to run (#1870): - * - `config.actionType` selecting a built-in side-effect ('email', 'slack', - * logger-backed), or - * - `config.function` (or a bare `actionType` that matches no built-in) - * naming a registered function — resolved via `engine.resolveFunction()`, - * which the host bridges to `bundle.functions` / `defineStack({ functions })`. - * A target that resolves to neither fails the step LOUDLY rather than the old - * silent "no-op handler" success, so an unwired callable can't quietly skip. - * The named function is contractually PURE — it returns a value and the flow - * graph persists it — which the descriptor publishes as - * `handlerContract: 'pure'` and a writing function opts out of by declaring - * `effect: 'writes'` where it is registered (#4396). - */ - -/** - * Built-in `script` side-effect action types with a (logger-backed) handler. - * Anything else is treated as a registered-function name (#1870). The member - * list is the spec-published `SCRIPT_BUILTIN_ACTION_TYPES` — the same constant - * the designer's `actionType` options reconcile against (#4278), so the form, - * this dispatch set, and the failure message below cannot disagree. + * - 'script' nodes call a registered function (#1870): `config.function` names + * it, `engine.resolveFunction()` resolves it, and the host bridges that to + * `bundle.functions` / `defineStack({ functions })`. A name that resolves to + * nothing fails the step LOUDLY rather than the old silent "no-op handler" + * success, so an unwired callable can't quietly skip. The named function is + * contractually PURE — it returns a value and the flow graph persists it — + * which the descriptor publishes as `handlerContract: 'pure'` and a writing + * function opts out of by declaring `effect: 'writes'` where it is registered + * (#4396). + * + * #4343 converged this node to that single path. `config.actionType`'s + * built-in side effects ('email' / 'slack') were logger-backed stubs that + * delivered nothing, inline `config.script` was never executed (no + * server-side JS sandbox), and any other `actionType` was a second spelling + * of `config.function`. All five keys are retired in the spec contract, which + * this executor now parses before it runs — see the note in `execute` for why + * that parse is what makes the retirement audible to stored metadata. */ -const SCRIPT_BUILTINS = new Set(SCRIPT_BUILTIN_ACTION_TYPES); export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext): void { // screen — server-side pass-through (input vars already injected by engine). @@ -201,7 +197,7 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext }, }); - // script — dispatch by actionType. + // script — call the registered function named by `config.function`. engine.registerNodeExecutor({ type: 'script', descriptor: defineActionDescriptor({ @@ -215,66 +211,41 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext handlerContract: 'pure', }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; - // The historical aliases (`functionName`/`input`) are canonicalized at - // load by the ADR-0087 D2 conversion 'flow-node-script-config-aliases' - // (#3796), so only the canonical keys are read here. - const fnRaw = cfg.function; - const fnName = typeof fnRaw === 'string' && fnRaw.trim() ? fnRaw.trim() : undefined; - const actionType = typeof cfg.actionType === 'string' && cfg.actionType.trim() ? cfg.actionType.trim() : undefined; - - // Built-in side-effect actions keep their logger-backed behavior — but - // only when an explicit `function` isn't set (that always wins). - if (!fnName && actionType && SCRIPT_BUILTINS.has(actionType)) { - ctx.logger.info( - `[Script:${actionType}] template=${String(cfg.template)} ` + - `recipients=${JSON.stringify(cfg.recipients)} ` + - `vars=${JSON.stringify(cfg.variables)}`, - ); - return { - success: true, - output: { actionType, template: cfg.template, recipients: cfg.recipients }, - }; - } - - // Inline `config.script` (a JS source body) is a distinct, recognized - // form — but the built-in runtime has no server-side JS sandbox, so it - // does not execute it. Warn loudly (not a silent success) and steer the - // author to the supported path — a registered function — rather than - // failing the flow. Executing inline scripts is a separate capability, - // out of #1870's callable-resolution scope. - const inlineScript = typeof cfg.script === 'string' && cfg.script.trim() ? cfg.script : undefined; - if (!fnName && inlineScript) { - ctx.logger.warn( - `[Script] node '${node.id}': inline \`config.script\` is not executed by the built-in runtime ` + - `(no server-side JS sandbox) — this node is a no-op. To run server logic, move it into a ` + - `registered function and call it via \`config.function\` + \`defineStack({ functions })\`.`, - ); - return { success: true, output: { script: 'not-executed' } }; - } - - // `actionType: 'invoke_function'` is a MARKER meaning "call the named - // function" — the name lives in `function`, not in actionType itself. A - // bare actionType that matched no built-in is still accepted as a - // function name (shorthand). - const target = fnName ?? (actionType === 'invoke_function' ? undefined : actionType); - if (!target) { - return { - success: false, - error: - actionType === 'invoke_function' - ? `script node '${node.id}': actionType 'invoke_function' requires \`config.function\` naming the function to call.` - : `script node '${node.id}': declares neither \`actionType\` nor \`function\` — nothing to run.`, - }; - } + // #4343 — the contract is parsed before anything runs. A `script` node + // calls a registered function and nothing else, so `config.function` is + // required and a retired dispatch key refuses here with its tombstone + // prescription (`notify` for mail, a Slack connector for slack, a + // registered function for an inline body). + // + // A refusal is a GUARD, not a routable failure: a config that fails its + // contract is wrong METADATA — re-running it unchanged can never + // succeed — so no `fault` edge may silence it (#3863). + // + // What a STORED flow meets here is the required `function`, not the + // tombstones: `registerFlow` canonicalizes data at rest through the + // retired conversion as well (#3903), so an old `actionType: 'email'` + // node arrives stripped of the keys nothing read, and refuses for + // naming no callable. That is the behavioral change the retirement + // bought — the same node used to log a line and report success. + // + // The historical aliases (`functionName`/`input`) are canonicalized on + // the same seam by 'flow-node-script-config-aliases' (#3796), so only + // the canonical keys reach the parse. + const parsed = parseNodeConfig('script', node.id, ScriptConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const target = cfg.function.trim(); + // Unresolvable is a RUNTIME failure, deliberately: the function + // registry is the host's (`defineStack({ functions })`), so the same + // metadata succeeds on a host that registers the name. Only the + // metadata itself earns a guard. const registration = engine.resolveFunction(target); if (!registration) { return { success: false, error: - `script node '${node.id}': '${target}' is not a built-in action ` + - `(${[...SCRIPT_BUILTINS].join(', ')}) and no function named '${target}' is registered. ` + + `script node '${node.id}': no function named '${target}' is registered. ` + `Register it via \`defineStack({ functions: { '${target}': fn } })\`, or fix the name (#1870).`, }; } @@ -283,8 +254,7 @@ export function registerScreenNodes(engine: AutomationEngine, ctx: PluginContext // `{var}` references against the live flow variables (so a function can // consume a prior node's output, e.g. `{aiResult.id}`). const input = interpolate(cfg.inputs ?? {}, variables, context) as Record; - const outputVariable = - typeof cfg.outputVariable === 'string' && cfg.outputVariable.trim() ? cfg.outputVariable.trim() : undefined; + const outputVariable = cfg.outputVariable?.trim() || undefined; // Pure-function pattern: the function RETURNS its result; `outputVariable` // exposes it as a flow variable so a later declarative node persists it // (e.g. `update_record fields: { ai_category: '{aiResult.ai_category}' }`). diff --git a/packages/services/service-automation/src/builtin/subflow-node.test.ts b/packages/services/service-automation/src/builtin/subflow-node.test.ts index b29e62bc80..e4516ae33a 100644 --- a/packages/services/service-automation/src/builtin/subflow-node.test.ts +++ b/packages/services/service-automation/src/builtin/subflow-node.test.ts @@ -128,7 +128,10 @@ describe('subflow node executor', () => { engine.registerFlow('parent_flow', parentFlow({ input: {} })); const result = await engine.execute('parent_flow'); expect(result.success).toBe(false); - expect(result.error).toMatch(/flowName is required/i); + // #4343 — the hand-written guard became the contract parse; same guard + // classification, message now derived from `SubflowConfigSchema`. + expect(result.error).toMatch(/does not satisfy the subflow contract/i); + expect(result.error).toMatch(/config\.flowName/); }); it('fails with a clear error when the child flow is not registered', async () => { diff --git a/packages/services/service-automation/src/builtin/subflow-node.ts b/packages/services/service-automation/src/builtin/subflow-node.ts index 4c881e0bc8..8ae1dca015 100644 --- a/packages/services/service-automation/src/builtin/subflow-node.ts +++ b/packages/services/service-automation/src/builtin/subflow-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, SubflowConfigSchema } from '@objectstack/spec/automation'; +import type { SubflowConfigParsed } from '@objectstack/spec/automation'; import type { AutomationContext } from '@objectstack/spec/contracts'; import type { AutomationEngine } from '../engine.js'; import { interpolate } from './template.js'; import { refuseNode } from '../guard-refusal.js'; +import { parseNodeConfig } from './parse-config.js'; /** Hard cap on subflow nesting — turns an accidental cycle into a clean error. */ const MAX_SUBFLOW_DEPTH = 16; @@ -52,14 +54,18 @@ export function registerSubflowNode(engine: AutomationEngine, ctx: PluginContext supportsPause: true, }), async execute(node, variables, context) { - const cfg = (node.config ?? {}) as Record; + // #4343 — the contract is parsed before anything runs, the same seam the + // flat builtins got in #4277: a missing or empty `flowName` refuses this + // node as a GUARD (wrong metadata; a rerun cannot supply it), with the + // path named, instead of the hand-written check this replaces. + // // The historical `flow` alias is canonicalized at load by the ADR-0087 D2 // conversion 'flow-node-subflow-flow-alias' (#4278), so only the - // canonical key is read here (contract: SubflowConfigSchema). - const flowName = typeof cfg.flowName === 'string' ? cfg.flowName : undefined; - if (!flowName) { - return refuseNode(`subflow '${node.id}': config.flowName is required`); - } + // canonical key reaches the parse. + const parsed = parseNodeConfig('subflow', node.id, SubflowConfigSchema, node.config); + if (!parsed.ok) return parsed.refusal; + const cfg = parsed.config; + const flowName = cfg.flowName; // Cycle guard: depth rides on the context so it accumulates across nesting. const depth = Number((context as { $subflowDepth?: number } | undefined)?.$subflowDepth ?? 0); @@ -72,10 +78,9 @@ export function registerSubflowNode(engine: AutomationEngine, ctx: PluginContext } // Map inputs (resolve `{var}` against the parent's variables/context). - const rawInput = (cfg.input && typeof cfg.input === 'object' ? cfg.input : {}) as Record; - const params = interpolate(rawInput, variables, context ?? ({} as AutomationContext)) as Record; + const params = interpolate(cfg.input ?? {}, variables, context ?? ({} as AutomationContext)) as Record; - const outVar = typeof cfg.outputVariable === 'string' && cfg.outputVariable ? cfg.outputVariable : undefined; + const outVar = cfg.outputVariable || undefined; // Parent linkage for nested durable pause: should the child suspend, the // engine persists these with the child run and uses them to bubble the 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 fb8a21607c..4d4ef1e680 100644 --- a/packages/services/service-automation/src/guard-refusal-inventory.test.ts +++ b/packages/services/service-automation/src/guard-refusal-inventory.test.ts @@ -139,7 +139,10 @@ const GUARDS: Array<{ name: string; why: string; node: Record; name: 'subflow without flowName', why: 'a required config key', node: { type: 'subflow', config: {} }, - expect: 'flowName is required', + // #4343 moved this from a hand-written `refuseNode` to the contract + // parse, like the CRUD entries above. Same classification, same node — + // only the message is now derived from `SubflowConfigSchema`. + expect: 'does not satisfy the subflow contract', }, { name: 'map without flowName', diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 7fa19d88c2..1a879fcfc0 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2244,8 +2244,6 @@ "RetryPolicy (type)", "RetryPolicySchema (const)", "SCHEMALESS_NODE_CONFIG_SCHEMAS (const)", - "SCRIPT_BUILTIN_ACTION_TYPES (const)", - "SCRIPT_INVOKE_FUNCTION_ACTION_TYPE (const)", "ScheduleState (type)", "ScheduleStateParsed (type)", "ScheduleStateSchema (const)", @@ -2255,7 +2253,6 @@ "ScreenConfigSchema (const)", "ScreenFieldConfig (type)", "ScreenFieldConfigSchema (const)", - "ScriptBuiltinActionType (type)", "ScriptConfig (type)", "ScriptConfigParsed (type)", "ScriptConfigSchema (const)", diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 9ce4ea628e..aaae66f476 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -1,5 +1,5 @@ { - "description": "Ratchet of every AUTHORABLE key in the spec \u2014 what a metadata author may write, which for this platform IS the third-party API. Auto-updated on additions (commit the change). A key that disappears without a tombstone fails gen:schema, because these schemas are not .strict() and Zod would silently strip it. \"[RETIRED]\" marks a tombstoned key that still rejects with an upgrade prescription. See #3855, ADR-0059 \u00a75.", + "description": "Ratchet of every AUTHORABLE key in the spec — what a metadata author may write, which for this platform IS the third-party API. Auto-updated on additions (commit the change). A key that disappears without a tombstone fails gen:schema, because these schemas are not .strict() and Zod would silently strip it. \"[RETIRED]\" marks a tombstoned key that still rejects with an upgrade prescription. See #3855, ADR-0059 §5.", "keys": [ "ai/AIModelConfig:maxTokens", "ai/AIModelConfig:model", @@ -2475,14 +2475,14 @@ "automation/ScreenFieldConfig:required", "automation/ScreenFieldConfig:type", "automation/ScreenFieldConfig:visibleWhen", - "automation/ScriptConfig:actionType", + "automation/ScriptConfig:actionType [RETIRED]", "automation/ScriptConfig:function", "automation/ScriptConfig:inputs", "automation/ScriptConfig:outputVariable", - "automation/ScriptConfig:recipients", - "automation/ScriptConfig:script", - "automation/ScriptConfig:template", - "automation/ScriptConfig:variables", + "automation/ScriptConfig:recipients [RETIRED]", + "automation/ScriptConfig:script [RETIRED]", + "automation/ScriptConfig:template [RETIRED]", + "automation/ScriptConfig:variables [RETIRED]", "automation/StateMachine:contextSchema", "automation/StateMachine:description", "automation/StateMachine:id", diff --git a/packages/spec/spec-changes.json b/packages/spec/spec-changes.json index fb5df1b4ac..e94bfffc0a 100644 --- a/packages/spec/spec-changes.json +++ b/packages/spec/spec-changes.json @@ -211,6 +211,12 @@ "to": "datasource key 'readReplicas' removed (#4468 — no driver opened a replica connection and no query path splits reads from writes; front replicas behind one endpoint and point `config` at it)", "conversionId": "datasource-read-replicas-removed", "toMajor": 17 + }, + { + "surface": "flow.node.script.config.actionType / flow.node.script.config.template / flow.node.script.config.recipients / flow.node.script.config.variables / flow.node.script.config.script", + "to": "script flow-node config keys 'actionType' (→ 'function' when it was shorthand for one; otherwise removed — 'email'/'slack' were logger-backed stubs that delivered nothing), plus 'template' / 'recipients' / 'variables' (fed those stubs) and 'script' (inline JS the runtime never executed) (#4343)", + "conversionId": "flow-node-script-branch-keys-removed", + "toMajor": 17 } ], "migrated": [ @@ -700,6 +706,12 @@ "to": "datasource key 'readReplicas' removed (#4468 — no driver opened a replica connection and no query path splits reads from writes; front replicas behind one endpoint and point `config` at it)", "conversionId": "datasource-read-replicas-removed", "toMajor": 17 + }, + { + "surface": "flow.node.script.config.actionType / flow.node.script.config.template / flow.node.script.config.recipients / flow.node.script.config.variables / flow.node.script.config.script", + "to": "script flow-node config keys 'actionType' (→ 'function' when it was shorthand for one; otherwise removed — 'email'/'slack' were logger-backed stubs that delivered nothing), plus 'template' / 'recipients' / 'variables' (fed those stubs) and 'script' (inline JS the runtime never executed) (#4343)", + "conversionId": "flow-node-script-branch-keys-removed", + "toMajor": 17 } ], "migrated": [ diff --git a/packages/spec/src/automation/flow-function.test.ts b/packages/spec/src/automation/flow-function.test.ts index 613b9accdd..a0a0ddbab5 100644 --- a/packages/spec/src/automation/flow-function.test.ts +++ b/packages/spec/src/automation/flow-function.test.ts @@ -69,6 +69,24 @@ describe('FlowFunctionEntrySchema', () => { it('rejects a declaration whose handler is not callable', () => { expect(FlowFunctionEntrySchema.safeParse({ handler: 'scoreLead' }).success).toBe(false); }); + + // #4343 — what `objectstack build` produces. The CLI lowers every inline + // callable to a serialisable ref BEFORE the stack is parsed, so a built + // manifest holds `{ scoreLead: 'scoreLead' }`. Rejecting that made + // `defineStack({ functions })` — a documented, first-class mechanism — + // unbuildable, which #4343 turned from latent into blocking by making + // `config.function` the only thing a `script` node can run. + it('accepts a lowered handler ref, the form a built artifact carries', () => { + expect(FlowFunctionEntrySchema.safeParse('scoreLead').success).toBe(true); + // Empty is not a name. + expect(FlowFunctionEntrySchema.safeParse('').success).toBe(false); + }); + + it('drops a lowered ref when normalizing — it names a function without carrying one', () => { + // The callable for that name comes from the sidecar ESM module the build + // emits; binding the string would register a name pointing at nothing. + expect(normalizeFlowFunctionEntry('scoreLead')).toBeUndefined(); + }); }); describe('defineStack({ functions }) — the authoring surface (#4396)', () => { diff --git a/packages/spec/src/automation/flow-function.zod.ts b/packages/spec/src/automation/flow-function.zod.ts index a5ce3ab843..59fc7a7fb8 100644 --- a/packages/spec/src/automation/flow-function.zod.ts +++ b/packages/spec/src/automation/flow-function.zod.ts @@ -112,13 +112,33 @@ export type FlowFunctionDeclaration = z.infer; /** - * One entry of the `functions` map as authors may write it: the handler alone - * (pure), or a {@link FlowFunctionDeclarationSchema} that states its effect. + * One entry of the `functions` map: the handler alone (pure), a + * {@link FlowFunctionDeclarationSchema} that states its effect, or the + * **lowered handler ref** a built artifact carries. + * + * The first two are what an author writes. The third is what `objectstack + * build` produces and was, until #4343, the reason `defineStack({ functions })` + * could not survive a build at all: the CLI lowers every inline callable to a + * serialisable string ref BEFORE the stack is parsed (it must — `z.function()` + * wraps callables and would break the ref mapping), so the manifest reaching + * this schema holds `{ myFn: 'myFn' }`, which neither of the other two members + * accepts. The build failed on a mechanism its own docs call first-class. + * + * A string entry carries no callable, and that is correct rather than lossy: + * the real functions ride in the sibling ESM module esbuild emits, and + * {@link collectBundleFunctionEntries} merges both sources by name. The string + * is the artifact's record that the NAME exists — which is why + * {@link normalizeFlowFunctionEntry} deliberately drops it (see there). + * + * Authoring a string by hand therefore registers nothing. It fails loudly, not + * silently: a `script` node naming it refuses at execute with "no function + * named '…' is registered" (#1870). */ export const FlowFunctionEntrySchema = lazySchema(() => z.union([ z.function(), FlowFunctionDeclarationSchema, -]).describe('A named handler function, or a declaration record stating its effect')); + z.string().min(1).describe('A lowered handler ref (built artifacts) — the callable rides in the sibling ESM module'), +]).describe('A named handler function, a declaration record stating its effect, or a lowered handler ref')); export type FlowFunctionEntry = z.infer; @@ -153,6 +173,12 @@ export function isFlowFunctionEffect(value: unknown): value is FlowFunctionEffec * Deliberately hand-written rather than a `FlowFunctionEntrySchema.parse()`: * the entry holds a live function, and the collectors that call this run on the * boot path where re-parsing every handler buys nothing. + * + * A lowered string ref (the third member of that schema) returns `undefined` + * here BY DESIGN — it names a function without carrying one. The callable for + * that name comes from the built sidecar module, which the same collector + * merges in; treating the string as an entry would register a name bound to + * nothing. */ export function normalizeFlowFunctionEntry(entry: unknown): NormalizedFlowFunction | undefined { if (typeof entry === 'function') { diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index e06867e9c6..ff3b35aad5 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -33,12 +33,12 @@ export const FlowNodeAction = z.enum([ 'get_record', // CRUD: Get/Query 'http', // Outbound HTTP callout (ADR-0018 M3) — canonical; outbox-backed when durable 'notify', // Outbound notification (ADR-0012) — dispatched via the messaging service - 'script', // Custom action: a built-in side-effect (`config.actionType: 'email'|'slack'`) - // or a registered function (`config.function: 'name'` + `config.inputs`), - // resolved from `defineStack({ functions })`. (Inline `config.script` JS is - // recognized but NOT executed by the built-in runtime — no server-side - // sandbox.) A script node naming none of these is flagged at build and - // fails loudly at run time (#1870). + 'script', // Custom action: call the registered function named by `config.function` + // (+ `config.inputs`), resolved from `defineStack({ functions })`. The key + // is REQUIRED — a node naming no callable is flagged at build and refused + // at execute (#1870). The `actionType` dispatch branches (logger-backed + // 'email'/'slack', inline `config.script`) were retired in 17 (#4343): + // use `notify` / a connector / a registered function instead. 'screen', // Screen / User-Input Element 'wait', // Delay/Sleep 'subflow', // Call another flow diff --git a/packages/spec/src/automation/schemaless-node-config.test.ts b/packages/spec/src/automation/schemaless-node-config.test.ts new file mode 100644 index 0000000000..84eec4198b --- /dev/null +++ b/packages/spec/src/automation/schemaless-node-config.test.ts @@ -0,0 +1,147 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The two schemaless node contracts that are PARSED at execute time (#4343). + * + * `script` and `subflow` run through `service-automation`'s `parseNodeConfig()` + * before their executors do anything, so what this file pins is not decoration: + * a shape accepted here runs, and a shape rejected here refuses the node as a + * guard. `decision` is deliberately absent — it stays export-only (its one key + * is optional, so a parse would have nothing to check). + * + * The structural assertions at the bottom guard the downstream walkers that a + * union-shaped contract would have broken, which is why #4343 converged the + * node instead of modelling its branches: the authorable-surface ratchet, the + * expression ledger and objectui's reconciliation all read a FLAT + * `properties` / `.shape`. + */ + +import { describe, expect, it } from 'vitest'; +import { z } from 'zod'; + +import { + ScriptConfigSchema, + SubflowConfigSchema, + getSchemalessNodeConfigJsonSchemas, +} from './schemaless-node-config.zod.js'; + +/** Every key the contract still declares, tombstones included. */ +const SCRIPT_SHAPE_KEYS = [ + 'actionType', 'function', 'inputs', 'outputVariable', + 'recipients', 'script', 'template', 'variables', +]; +/** The keys #4343 retired — each must reject with its own prescription. */ +const SCRIPT_RETIRED: ReadonlyArray<[string, unknown]> = [ + ['actionType', 'email'], + ['template', 'task_done'], + ['recipients', ['{record.owner}']], + ['variables', { taskName: '{record.name}' }], + ['script', 'return { ok: true };'], +]; + +describe('ScriptConfigSchema (#4343 — converged to a function call)', () => { + it('accepts the one shape the executor runs', () => { + expect(ScriptConfigSchema.parse({ + function: 'score_lead', + inputs: { leadId: '{record.id}' }, + outputVariable: 'score', + })).toEqual({ + function: 'score_lead', + inputs: { leadId: '{record.id}' }, + outputVariable: 'score', + }); + }); + + it('accepts a bare `function` — inputs and outputVariable stay optional', () => { + expect(ScriptConfigSchema.parse({ function: 'score_lead' })).toEqual({ function: 'score_lead' }); + }); + + it('requires `function`: a script node that names no callable has nothing to run', () => { + const empty = ScriptConfigSchema.safeParse({}); + expect(empty.success).toBe(false); + expect(empty.error!.issues[0]!.path).toEqual(['function']); + + // Same for a present-but-empty name — `.min(1)`, not just "declared". + expect(ScriptConfigSchema.safeParse({ function: '' }).success).toBe(false); + }); + + it.each(SCRIPT_RETIRED)('rejects the retired `%s` with its own prescription', (key, value) => { + const result = ScriptConfigSchema.safeParse({ function: 'score_lead', [key]: value }); + expect(result.success).toBe(false); + const message = result.error!.issues.map((i) => i.message).join('\n'); + // The tombstone's payload is the prescription, not "unrecognized key" — + // this string IS the upgrade doc for whoever hits it (retired-key.ts). + expect(message).toContain(`\`script.config.${key}\``); + expect(message).toMatch(/#4343/); + expect(message).toMatch(/os migrate meta --from 16/); + expect(result.error!.issues[0]!.path).toEqual([key]); + }); + + it('names every violated key at once, so one refusal lists the whole job', () => { + const result = ScriptConfigSchema.safeParse({ + function: 'score_lead', actionType: 'email', template: 't', recipients: ['a'], + }); + expect(result.success).toBe(false); + expect(result.error!.issues.map((i) => i.path[0]).sort()) + .toEqual(['actionType', 'recipients', 'template']); + }); + + it('prescribes a different mechanism per branch — the retirement is not one rename', () => { + const messageFor = (key: string, value: unknown) => + ScriptConfigSchema.safeParse({ function: 'f', [key]: value }).error!.issues[0]!.message; + // Mail has a real delivery path; Slack does not go through it (no slack + // channel exists — that is a connector), and an inline body is a function. + expect(messageFor('actionType', 'email')).toMatch(/`notify` node/); + expect(messageFor('actionType', 'email')).toMatch(/connector_action/); + expect(messageFor('script', 'return 1;')).toMatch(/defineStack\(\{ functions \}\)/); + }); +}); + +describe('SubflowConfigSchema (#4343 — parsed at execute time)', () => { + it('accepts the executor-read shape', () => { + expect(SubflowConfigSchema.parse({ + flowName: 'escalation_flow', + input: { caseId: '{record.id}' }, + outputVariable: 'subResult', + })).toEqual({ + flowName: 'escalation_flow', + input: { caseId: '{record.id}' }, + outputVariable: 'subResult', + }); + }); + + it('refuses a missing or empty `flowName` — the step cannot pick a flow', () => { + for (const bad of [{}, { flowName: '' }]) { + const result = SubflowConfigSchema.safeParse(bad); + expect(result.success, JSON.stringify(bad)).toBe(false); + expect(result.error!.issues[0]!.path).toEqual(['flowName']); + } + }); +}); + +describe('structural contract — what the downstream walkers require', () => { + it('keeps the tombstoned keys IN the shape, so the ratchet can see them retired', () => { + // A `retiredKey()` is still a property. Deleting it outright would read as + // "the key vanished" to the authorable-surface gate, which is the hard + // failure the tombstone route exists to avoid. + expect(Object.keys(ScriptConfigSchema.shape).sort()).toEqual(SCRIPT_SHAPE_KEYS); + for (const [key] of SCRIPT_RETIRED) { + expect(ScriptConfigSchema.shape[key as keyof typeof ScriptConfigSchema.shape].description) + .toMatch(/^\[REMOVED\]/); + } + }); + + it('stays a FLAT JSON Schema — no anyOf/oneOf for a union-blind walker to miss', () => { + const json = getSchemalessNodeConfigJsonSchemas().script as Record; + expect(Object.keys(json.properties as object).sort()).toEqual(SCRIPT_SHAPE_KEYS); + for (const combinator of ['anyOf', 'oneOf', 'allOf']) { + expect(json[combinator], `top-level ${combinator} would blind the authorable-surface walk`) + .toBeUndefined(); + } + expect((json.required as string[])).toEqual(['function']); + }); + + it('still converts without throwing, tombstones and all', () => { + expect(() => z.toJSONSchema(SubflowConfigSchema, { unrepresentable: 'any' })).not.toThrow(); + }); +}); diff --git a/packages/spec/src/automation/schemaless-node-config.zod.ts b/packages/spec/src/automation/schemaless-node-config.zod.ts index 997e5a3213..a91a2e69eb 100644 --- a/packages/spec/src/automation/schemaless-node-config.zod.ts +++ b/packages/spec/src/automation/schemaless-node-config.zod.ts @@ -11,14 +11,18 @@ * * `config-schemas.test.ts` in `service-automation` pins the schemaless class * with each member's reason: `decision`'s virtual Target column is derived from - * the out-edges, `script`'s form switches on `actionType`, `subflow` carries a - * top-level `timeoutMs` — a published partial schema would DROP those editors - * (the #4210 `connector_action` incident). So the Studio form for these types - * is objectui's hand-written group, and until #4278 **nothing reconciled that - * hand-written table against the executors**: `script`'s form offered an - * `outputVariables` key nothing reads, two `actionType` options that fail every - * run, a no-op default — and could not author the `function`/`inputs`/ - * `outputVariable` path that works. + * the out-edges, `subflow` carries a top-level `timeoutMs` — a published + * partial schema would DROP those editors (the #4210 `connector_action` + * incident). So the Studio form for these types is objectui's hand-written + * group, and until #4278 **nothing reconciled that hand-written table against + * the executors**: `script`'s form offered an `outputVariables` key nothing + * reads, two `actionType` options that fail every run, a no-op default — and + * could not author the `function`/`inputs`/`outputVariable` path that works. + * + * `script`'s own reason for staying schemaless was that its form switched on + * `actionType`. #4343 retired that switch, so the node is now three flat keys + * and could graduate to a published descriptor `configSchema` the way `map` + * did — a follow-up, deliberately not folded into the retirement. * * These schemas are the machine-readable half of that reconciliation. They are * **written from the executors** (`service-automation/builtin/screen-nodes.ts` @@ -34,24 +38,37 @@ * {@link FlowNodeSchema} (`waitEventConfig` / `connectorConfig`), which the * same objectui test reconciles directly. * - * ## What these schemas are (and are not) wired to + * ## What these schemas are wired to + * + * `script` and `subflow` are **parsed at execute time** since #4343, through + * the same `parseNodeConfig()` seam #4277 gave the flat builtins + * (`service-automation`'s `parse-config.ts`): a config that fails its contract + * refuses the node as a GUARD — wrong metadata, so a rerun cannot help and no + * `fault` edge may route it (#3863). + * + * `script` could not be parsed while its legal key set depended on + * `actionType`; #4343 removed that dependence instead of modelling it. + * Converging the node to its one real path — call a registered function — left + * a flat three-key contract a flat parse fits exactly, and the five keys the + * other branches read became {@link retiredKey} tombstones. * - * Contract exports only — no engine path `parse()`s a node config with them, - * so registering a flow behaves exactly as before. This is where they differ - * from their `builtin-node-config.zod.ts` siblings, which #4277 wired into - * execute-time parsing (`service-automation`'s `parse-config.ts`) and into the - * `registerFlow()` unknown-key rejection. + * The two halves reach different audiences, which is why they shipped together: * - * That difference is deliberate, and it is the same reason these three publish - * no descriptor `configSchema`: **their key set is not the whole contract.** - * `script`'s legal keys depend on `actionType` (a built-in side effect reads - * `template`/`recipients`/`variables`; the function path reads - * `function`/`inputs`/`outputVariable`), and `decision` may carry no - * `conditions` at all when it branches purely on edge predicates. A flat parse - * would either reject those shapes or wave everything through — neither is the - * contract. Wiring them in needs a discriminated form first; until then the - * enforcement they DO get is the objectui reconciliation test, which is what - * #4278 was actually about (a form authoring keys nothing reads). + * - the **tombstones** teach whoever authors the key — `tsc` types it `never`, + * and a direct parse raises the prescription. They do NOT reach a stored + * flow: `FlowNodeSchema.config` is `z.record(z.unknown())`, so no load-path + * parse ever descends into a node's config; + * - the **execute-time parse** is what a stored flow meets. `registerFlow` + * canonicalizes data at rest through the retired conversion too (#3903), so + * a stored `actionType: 'email'` node arrives here stripped of the keys + * nothing read — and then refuses, naming the `function` it does not have, + * instead of logging a line and reporting success as it used to. + * + * `decision` stays export-only, deliberately: it may carry no `conditions` at + * all when it branches purely on edge predicates (a plain BPMN exclusive + * gateway), and `conditions` is its only key — so a parse would have nothing + * left to check. Its enforcement remains the objectui reconciliation test, + * which is what #4278 was actually about (a form authoring keys nothing reads). * * Undeclared aliases are NOT part of these contracts: `subflow`'s historical * `flow` spelling graduated into the ADR-0087 D2 conversion @@ -61,87 +78,106 @@ import { z } from 'zod'; import { lazySchema } from '../shared/lazy-schema'; +import { retiredKey } from '../shared/retired-key'; // ─── script ────────────────────────────────────────────────────────── -/** - * `script` action types with a built-in (logger-backed) side-effect handler. - * Any other non-marker `actionType` is treated as a registered-function name - * (#1870). The executor builds its dispatch set from THIS constant, and the - * designer's `actionType` options must stay within - * `[SCRIPT_INVOKE_FUNCTION_ACTION_TYPE, ...SCRIPT_BUILTIN_ACTION_TYPES]` — - * the #4278 drift was the form offering `sms` / `notification`, which are in - * neither set and so failed every run as unresolvable function names. - */ -export const SCRIPT_BUILTIN_ACTION_TYPES = ['email', 'slack'] as const; -export type ScriptBuiltinActionType = (typeof SCRIPT_BUILTIN_ACTION_TYPES)[number]; - -/** - * The `actionType` MARKER meaning "call the registered function named by - * `config.function`" — it is not itself a function name. The executor fails - * the step with a clear message when this marker is set and `function` is not. - */ -export const SCRIPT_INVOKE_FUNCTION_ACTION_TYPE = 'invoke_function'; - /** * `script` node config — what the executor reads (screen-nodes.ts). * - * Dispatch precedence, verbatim from the executor: - * - * 1. `function` set → resolve and call that registered function (always wins). - * 2. `actionType` ∈ {@link SCRIPT_BUILTIN_ACTION_TYPES} → the built-in - * logger-backed side effect, fed by `template` / `recipients` / `variables`. - * 3. `script` set (no `function`) → **recognized but NOT executed**: the - * built-in runtime has no server-side JS sandbox, so the node warns loudly - * and completes as a no-op. Authoring is steered to a registered function. - * 4. Anything left in `actionType` (except the - * {@link SCRIPT_INVOKE_FUNCTION_ACTION_TYPE} marker) is shorthand for a - * function name; a name that resolves to nothing fails the step LOUDLY. + * **One shape, one path (#4343):** a `script` node names a registered function + * (`defineStack({ functions })`), passes it `inputs`, and binds its return + * value to `outputVariable`. `function` is required — a script node that names + * no callable has nothing to run, and the execute-time parse refuses it rather + * than letting the run discover that halfway through. * * The invoked function is contractually PURE — it returns its result and the * flow graph persists it (`FlowFunctionEffectSchema`, #4396). The descriptor * publishes that as `handlerContract: 'pure'`, and it is what lets the node - * report no record metrics without guessing. + * report no record metrics without guessing. A function that legitimately + * writes declares `effect: 'writes'` where it is registered, so the run reports + * an effect it cannot count instead of reporting none. + * + * ## What the four other shapes were, and why they are gone + * + * Until #4343 the legal key set depended on `actionType`, which is why this + * contract could not be parsed at all (see the module header). Of the four + * dispatch branches only the function path ran real logic: + * + * - `actionType: 'email' | 'slack'` were **logger-backed stubs**. They wrote a + * line to the log, reported success, and delivered nothing under any + * configuration — `template` / `recipients` / `variables` fed a message no + * channel ever sent. `notify` (real delivery, via the messaging service) and + * `connector_action` were already the live mechanisms. + * - `script` (inline JS) was **recognized but never executed**: the built-in + * runtime has no server-side JS sandbox, so the node warned and no-op'd. + * - any other `actionType` was **shorthand for a function name** — a second + * spelling of `function`, and the `invoke_function` marker named nothing on + * its own. + * + * All five keys are tombstoned below; the ADR-0087 D2 conversion + * `flow-node-script-branch-keys-removed` rewrites stored sources (moving a + * shorthand `actionType` into `function`, where that is what it meant). */ export const ScriptConfigSchema = lazySchema(() => z.object({ - /** Built-in side-effect id, the `invoke_function` marker, or (shorthand) a registered-function name. */ - actionType: z.string().optional() - .describe("How this step runs: a built-in side effect ('email' | 'slack'), the 'invoke_function' marker, or shorthand for a registered-function name"), /** - * Registered function to call (`defineStack({ functions })`) — always wins - * over `actionType`. + * Registered function to call (`defineStack({ functions })`) — required: it + * is the whole of what a `script` node does. * * Contractually pure: it takes `inputs`, RETURNS a value, and does no data * I/O of its own. A function that legitimately writes declares * `effect: 'writes'` where it is registered, so the run reports an effect it * cannot count instead of reporting none (#4396). */ - function: z.string().optional() - .describe('Registered function to call (defineStack({ functions })); takes precedence over actionType. Contractually pure — it returns a value a later declarative node persists'), + function: z.string().min(1) + .describe('Registered function to call (defineStack({ functions })). Contractually pure — it returns a value a later declarative node persists'), /** Inputs passed to the function; values interpolate `{token}` templates against the live flow variables. */ inputs: z.record(z.string(), z.unknown()).optional() .describe('Inputs passed to the function (values interpolate {token} templates)'), /** Flow variable the function's RETURN value is bound to (pure-function pattern — data I/O stays on the graph). */ outputVariable: z.string().optional() .describe("Flow variable the function's return value is bound to"), - /** Built-in side effects only: message template id. */ - template: z.string().optional() - .describe('Built-in side effects only: message template id'), - /** Built-in side effects only: recipient list (user ids, field refs, addresses). */ - recipients: z.array(z.string()).optional() - .describe('Built-in side effects only: recipients (user ids, field refs, or addresses)'), - /** Built-in side effects only: values injected into the template. */ - variables: z.record(z.string(), z.unknown()).optional() - .describe('Built-in side effects only: values injected into the template'), - /** - * Inline JS source — recognized but NOT executed by the built-in runtime (no - * server-side JS sandbox): the node warns and completes as a no-op. Kept in - * the contract because the executor reads it; deliberately NOT offered for - * new authoring (the designer renders a stored value read-only-style and - * steers authors to `function`). - */ - script: z.string().optional() - .describe('Inline JS source — recognized but not executed by the built-in runtime; use a registered function via `function` instead'), + + // The four retired dispatch branches (#4343). Each tombstone carries its own + // prescription because the three replacements are different mechanisms, not + // one rename: real messaging is `notify`, Slack is a connector, and inline + // logic belongs in a registered function. + actionType: retiredKey( + '`script.config.actionType` was removed in @objectstack/spec 17 (#4343) — none of its values ' + + 'did what it said. The two built-ins were logger-backed stubs that recorded the intent and ' + + 'delivered nothing under any configuration, and every other value was a second spelling of ' + + '`config.function`. Replace it per branch: for `email` use a `notify` node (it delivers ' + + 'through the messaging service — the in-app inbox by default, real email once ' + + '`@objectstack/plugin-email` is installed); for `slack` use a `connector_action` node with ' + + 'the Slack connector, or an `http` node posting to a webhook; for anything else, move the ' + + 'name into `config.function`. Run `os migrate meta --from 16` to rewrite it automatically.', + ), + template: retiredKey( + '`script.config.template` was removed in @objectstack/spec 17 (#4343) — it fed only the ' + + 'logger-backed `email`/`slack` stubs, which never rendered or sent a message, so no template ' + + 'id was ever resolved. Delete the key. A `notify` node carries its own `title`/`message`, and ' + + 'stored templates live in the messaging service (`sys_notification_template`), not on the ' + + 'node. Run `os migrate meta --from 16` to rewrite it automatically.', + ), + recipients: retiredKey( + '`script.config.recipients` was removed in @objectstack/spec 17 (#4343) — the addresses were ' + + 'logged, never messaged: the `email`/`slack` branches it fed delivered nothing. Use a ' + + '`notify` node, whose `recipients` (user ids, field refs or addresses) reach the messaging ' + + 'service for real. Run `os migrate meta --from 16` to rewrite it automatically.', + ), + variables: retiredKey( + '`script.config.variables` was removed in @objectstack/spec 17 (#4343) — it injected values ' + + 'into a template no side effect ever rendered. Delete the key. A `notify` node carries ' + + 'structured data in `payload`; a registered function takes it in `config.inputs`. ' + + 'Run `os migrate meta --from 16` to rewrite it automatically.', + ), + script: retiredKey( + '`script.config.script` was removed in @objectstack/spec 17 (#4343) — the built-in runtime has ' + + 'no server-side JS sandbox, so an inline body was recognized and never executed: the node ' + + 'warned and completed as a no-op. Move the logic into a registered function ' + + '(`defineStack({ functions })`) and name it in `config.function`. ' + + 'Run `os migrate meta --from 16` to rewrite it automatically.', + ), })); export type ScriptConfig = z.input; @@ -152,8 +188,11 @@ export type ScriptConfigParsed = z.infer; /** * `subflow` node config — what the executor reads (subflow-node.ts). * - * `flowName` is execute-time required (the step is refused without it). The - * historical undeclared `flow` alias is NOT part of this contract: the + * `flowName` is execute-time required: since #4343 the executor parses this + * contract before it runs, so a missing or empty name refuses the node as a + * guard (wrong metadata — a rerun cannot supply it) instead of failing through + * a hand-written check. The historical undeclared `flow` alias is NOT part of + * this contract: the * ADR-0087 D2 conversion `flow-node-subflow-flow-alias` rewrites it at load * (#4278 — the `map.flow` graduation path), so the executor only ever sees * `flowName`. The node-level `timeoutMs` lives on {@link FlowNodeSchema}, not @@ -161,7 +200,7 @@ export type ScriptConfigParsed = z.infer; */ export const SubflowConfigSchema = lazySchema(() => z.object({ /** The flow to invoke (execute-time required). */ - flowName: z.string().describe('Flow invoked as this step (it may pause — approval / screen / wait)'), + flowName: z.string().min(1).describe('Flow invoked as this step (it may pause — approval / screen / wait)'), /** Values passed to the child's input variables; `{token}` templates resolve against the parent's variables. */ input: z.record(z.string(), z.unknown()).optional() .describe("Values passed to the subflow's input variables (interpolate {token} templates)"), diff --git a/packages/spec/src/conversions/conversions.test.ts b/packages/spec/src/conversions/conversions.test.ts index a8541c30c5..3d67017a6c 100644 --- a/packages/spec/src/conversions/conversions.test.ts +++ b/packages/spec/src/conversions/conversions.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest'; import { FlowSchema } from '../automation/flow.zod.js'; +import { ScriptConfigSchema } from '../automation/schemaless-node-config.zod.js'; import { normalizeStackInput } from '../shared/metadata-collection.zod.js'; import { applyConversions, collectConversionNotices } from './apply.js'; import { ALL_CONVERSIONS, CONVERSIONS_BY_MAJOR } from './registry.js'; @@ -289,6 +290,114 @@ describe('conversion layer (ADR-0087 D2)', () => { }); }); + describe('flow-node-script-branch-keys-removed (#4343)', () => { + /** One `script` node in a flow shaped the way the conversion walks it. */ + const scriptFlow = (config: Record) => ({ + flows: [ + { + name: 'task_lifecycle', + label: 'Task lifecycle', + type: 'autolaunched', + edges: [], + nodes: [ + { id: 'n1', type: 'start', label: 'Start' }, + { id: 's', type: 'script', label: 'Script', config }, + ], + }, + ], + }); + const cfgOf = (stack: Record) => (stack.flows as any[])[0].nodes[1].config; + // Retired from the load path (the keys misdescribed themselves), so the + // default `applyConversions` skips it — only `os migrate meta` replays it. + const convert = (stack: Record) => collectConversionNotices(stack, { includeRetired: true }); + + it('moves a shorthand `actionType` into `function` — that is what it named', () => { + const { stack, notices } = convert(scriptFlow({ actionType: 'score_lead', outputVariable: 'score' })); + expect(cfgOf(stack)).toEqual({ function: 'score_lead', outputVariable: 'score' }); + expect(notices).toHaveLength(1); + expect(notices[0]!.to).toBe('config.function'); + }); + + it('drops a shorthand `actionType` instead of moving it when `function` already won', () => { + const { stack, notices } = convert(scriptFlow({ actionType: 'stale_name', function: 'score_lead' })); + expect(cfgOf(stack)).toEqual({ function: 'score_lead' }); + expect(notices).toHaveLength(1); + }); + + it('drops the built-in ids and the bare marker — neither was ever a function name', () => { + for (const actionType of ['email', 'slack', 'invoke_function']) { + const { stack, notices } = convert(scriptFlow({ actionType, function: 'score_lead' })); + expect(cfgOf(stack), actionType).toEqual({ function: 'score_lead' }); + expect(notices, actionType).toHaveLength(1); + expect(notices[0]!.to, actionType).toMatch(/removed/); + } + }); + + it('drops the stub payload keys — nothing ever read them, so there is nothing to preserve', () => { + const { stack, notices } = convert(scriptFlow({ + actionType: 'email', + template: 'task_done', + recipients: ['{record.owner}'], + variables: { taskName: '{record.name}' }, + })); + expect(cfgOf(stack)).toEqual({}); + expect(notices).toHaveLength(4); + }); + + it('drops an inline `script` body the runtime never executed', () => { + const { stack, notices } = convert(scriptFlow({ script: 'return { ok: true };' })); + expect(cfgOf(stack)).toEqual({}); + expect(notices).toHaveLength(1); + }); + + it('leaves an already-converged node untouched', () => { + const { stack, notices } = convert(scriptFlow({ function: 'score_lead', inputs: { id: '{record.id}' } })); + expect(cfgOf(stack)).toEqual({ function: 'score_lead', inputs: { id: '{record.id}' } }); + expect(notices).toHaveLength(0); + }); + + it('leaves a non-script node carrying the same key names alone', () => { + // Unlike the wait retirement, these tombstones live on the script config + // contract — no other node type is parsed against it, so a `template` key + // elsewhere is that node's own business. + const stack0 = { + flows: [{ + name: 'f', + nodes: [{ id: 'n', type: 'notify', config: { template: 'x', recipients: ['a'] } }], + }], + }; + const { stack, notices } = convert(stack0); + expect(stack).toEqual(stack0); + expect(notices).toHaveLength(0); + }); + + it('tombstones every retired key so a source that skipped conversion is rejected, not stripped', () => { + // NOTE the channel: unlike `waitEventConfig`, a node's `config` is + // `z.record(z.unknown())` on `FlowNodeSchema`, so `FlowSchema.parse` does + // NOT reach these tombstones — they answer whoever AUTHORS the key (`tsc` + // types it `never`; this parse raises the prescription). A stored flow is + // reached by the other half: `registerFlow` replays this conversion even + // though it is retired (#3903), and the execute-time parse then refuses + // what is left over for naming no callable. + for (const bad of [ + { actionType: 'email' }, + { template: 't' }, + { recipients: ['a'] }, + { variables: { x: 1 } }, + { script: 'return 1;' }, + ]) { + const key = Object.keys(bad)[0]!; + expect( + () => ScriptConfigSchema.parse({ function: 'score_lead', ...bad }), + `${key} must be rejected`, + ).toThrow(/4343/); + } + // The flow-level parse is deliberately blind here — pinned so the note + // above stays true if `FlowNodeSchema.config` is ever tightened. + expect(() => FlowSchema.parse((scriptFlow({ actionType: 'email' }).flows as any[])[0])).not.toThrow(); + }); + }); + describe('flow-node-wait-event-config-lift (PD #12 retirement, #4045)', () => { /** * One `wait` node in a flow that `FlowSchema` can actually parse — `label` is diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index fb84fb359c..80b3091129 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -1510,6 +1510,10 @@ const flowNodeConnectorConfigLift: MetadataConversion = { * `connectorConfig.input` (singular) is a *different, canonical* surface and is * deliberately not touched here. Both are pure key renames with unchanged * values. **Live window**; retires at 18. + * + * The fixture below carried `actionType: 'invoke_function'` through both sides + * until #4343 retired that key — an end state protocol 17 no longer reaches, so + * it is gone from both. The rename itself is untouched. */ const flowNodeScriptConfigAliases: MetadataConversion = { id: 'flow-node-script-config-aliases', @@ -1538,7 +1542,6 @@ const flowNodeScriptConfigAliases: MetadataConversion = { id: 'n2', type: 'script', config: { - actionType: 'invoke_function', functionName: 'score_lead', input: { leadId: '{record.id}' }, outputVariable: 'score', @@ -1558,7 +1561,6 @@ const flowNodeScriptConfigAliases: MetadataConversion = { id: 'n2', type: 'script', config: { - actionType: 'invoke_function', function: 'score_lead', inputs: { leadId: '{record.id}' }, outputVariable: 'score', @@ -2285,6 +2287,165 @@ const datasourceReadReplicasRemoved: MetadataConversion = { }, }; +/** + * `script` node config — the four retired dispatch branches (protocol 17, #4343). + * + * A `script` node had four ways to name what it ran and only one of them ran + * anything. `actionType: 'email' | 'slack'` were logger-backed stubs: they wrote + * a line and reported success, and `template` / `recipients` / `variables` fed a + * message no channel ever sent — under any configuration, with or without the + * messaging service installed. Inline `config.script` was recognized and never + * executed (the built-in runtime has no server-side JS sandbox), so the node + * warned and no-op'd. Every remaining `actionType` value was shorthand for a + * registered-function name — a second spelling of `config.function` — and the + * `invoke_function` marker named nothing on its own. + * + * So the node converges on its one real path (call the function named by + * `config.function`), and the five keys leave the surface. This is what let the + * contract be parsed at execute time at all: while the legal key set depended on + * `actionType`, a flat parse would either reject valid shapes or wave everything + * through — see the module header of `automation/schemaless-node-config.zod.ts`. + * + * **Retired from the load path**, like every other key retired for lying rather + * than for being renamed (see `flow-node-wait-timeout-keys-removed` for the + * distinction the registry draws): silently absorbing `actionType: 'email'` + * would let an author keep believing the flow sends mail. + * + * A **shorthand `actionType` moves into `function`** rather than being dropped, + * because that is what it meant (#1870) — the same reasoning that moves + * `timeoutMs` into `timerDuration`. It moves only when `function` is not already + * set: with both present the executor always took `function`, so the shorthand + * was already dead metadata. The built-in ids and the `invoke_function` marker + * are never function names, so they are dropped, not moved. + * + * The other four keys are dropped outright: no reader ever consumed them, so + * there is no value to preserve. Rebuilding the intent is an authoring decision + * the tombstones prescribe per branch (`notify` for mail, a `connector_action` + * with the Slack connector — or `http` to a webhook — for Slack, a registered + * function for an inline body), not something a mechanical rewrite can guess. + * + * Ordering note: this runs AFTER `flow-node-script-config-aliases`, so the + * `functionName` → `function` rename has already happened when the shorthand + * rule asks whether `function` is set. + */ +const SCRIPT_RETIRED_BUILTIN_ACTION_TYPES = new Set(['email', 'slack']); +const SCRIPT_RETIRED_INVOKE_FUNCTION_MARKER = 'invoke_function'; + +function removeScriptBranchKeys(stack: Dict, emit: Emit): Dict { + return mapFlowNodes(stack, (node, path) => { + // Filtered to `script`, unlike the wait retirement: these tombstones live on + // the script config contract, which no other node type is parsed against. + if (node.type !== 'script') return node; + const cfg = node.config; + if (!isDict(cfg)) return node; + + const next: Dict = { ...cfg }; + let changed = false; + + if (next.actionType != null) { + const actionType = typeof next.actionType === 'string' ? next.actionType.trim() : ''; + const hasFunction = typeof next.function === 'string' && next.function.trim() !== ''; + const isShorthand = + actionType !== '' + && actionType !== SCRIPT_RETIRED_INVOKE_FUNCTION_MARKER + && !SCRIPT_RETIRED_BUILTIN_ACTION_TYPES.has(actionType); + + if (isShorthand && !hasFunction) { + next.function = actionType; + emit({ from: 'config.actionType', to: 'config.function', path: `${path}.config.function` }); + } else if (isShorthand) { + emit({ from: 'config.actionType', to: '(removed — `config.function` already named the callable)', path: `${path}.config` }); + } else { + emit({ from: 'config.actionType', to: '(removed — logger-backed stub or bare marker; nothing was delivered)', path: `${path}.config` }); + } + delete next.actionType; + changed = true; + } + + for (const key of ['template', 'recipients', 'variables'] as const) { + if (next[key] == null) continue; + emit({ from: `config.${key}`, to: '(removed — fed a side effect that never delivered)', path: `${path}.config` }); + delete next[key]; + changed = true; + } + + if (next.script != null) { + emit({ from: 'config.script', to: '(removed — inline JS was never executed)', path: `${path}.config` }); + delete next.script; + changed = true; + } + + return changed ? { ...node, config: next } : node; + }); +} + +const flowNodeScriptBranchKeysRemoved: MetadataConversion = { + id: 'flow-node-script-branch-keys-removed', + toMajor: 17, + retiredFromLoadPath: true, + surface: + 'flow.node.script.config.actionType / flow.node.script.config.template / ' + + 'flow.node.script.config.recipients / flow.node.script.config.variables / ' + + 'flow.node.script.config.script', + summary: + "script flow-node config keys 'actionType' (→ 'function' when it was shorthand for one; otherwise removed — " + + "'email'/'slack' were logger-backed stubs that delivered nothing), plus 'template' / 'recipients' / " + + "'variables' (fed those stubs) and 'script' (inline JS the runtime never executed) (#4343)", + apply(stack, emit) { + return removeScriptBranchKeys(stack, emit); + }, + fixture: { + before: { + flows: [ + { + name: 'task_lifecycle', + nodes: [ + { id: 'n1', type: 'start' }, + // The logger-backed stub in full: nothing here was ever delivered. + { + id: 'n2', + type: 'script', + config: { + actionType: 'email', + template: 'task_done', + recipients: ['{record.owner}'], + variables: { taskName: '{record.name}' }, + }, + }, + // Shorthand for a registered function — the one value that MOVES. + { id: 'n3', type: 'script', config: { actionType: 'score_lead', outputVariable: 'score' } }, + // Inline body: recognized, never executed. Dropped; the node is left + // naming no callable, which the execute-time parse now says out loud. + { id: 'n4', type: 'script', config: { script: 'return { ok: true };' } }, + // The marker alongside the canonical key: marker dropped, key kept. + { id: 'n5', type: 'script', config: { actionType: 'invoke_function', function: 'score_lead' } }, + // Already converged — left byte-identical. + { id: 'n6', type: 'script', config: { function: 'notify_owner', inputs: { id: '{record.id}' } } }, + ], + }, + ], + }, + after: { + flows: [ + { + name: 'task_lifecycle', + nodes: [ + { id: 'n1', type: 'start' }, + { id: 'n2', type: 'script', config: {} }, + { id: 'n3', type: 'script', config: { outputVariable: 'score', function: 'score_lead' } }, + { id: 'n4', type: 'script', config: {} }, + { id: 'n5', type: 'script', config: { function: 'score_lead' } }, + { id: 'n6', type: 'script', config: { function: 'notify_owner', inputs: { id: '{record.id}' } } }, + ], + }, + ], + }, + // n2: actionType + template + recipients + variables. n3: the move. + // n4: script. n5: the marker. n6: nothing. + expectedNotices: 7, + }, +}; + export const CONVERSIONS_BY_MAJOR: Readonly> = { 11: [flowNodeHttpRename, pageKindJsxToHtml, flowNodeFilterAlias, objectCompactLayoutRename], 13: [stackRolesToPositions, owdLegacyReadAliases, sharingRecipientRoleToPosition], @@ -2315,6 +2476,9 @@ export const CONVERSIONS_BY_MAJOR: Readonly … } })`. An empty - `script` node — or one pointing at an unregistered function — fails loudly. - Inline `config.script` JS is **not executed** by the built-in runtime (no - server-side sandbox) — move logic into a registered `function`. +9. **`script` nodes call a registered function — that is all they do.** Set + `config.function` to a function registered via + `defineStack({ functions: { my_fn: (ctx) => … } })`. It is **required**: an + empty `script` node refuses at execute, and one pointing at an unregistered + function fails loudly. + + The other dispatch forms were retired in spec 17 (#4343) because none of them + ran: `config.actionType: 'email' | 'slack'` were logger-backed stubs that + delivered nothing (with `config.template` / `.recipients` / `.variables` + feeding a message no channel sent), and inline `config.script` JS was never + executed (no server-side sandbox). Use a **`notify`** node for real + notification delivery, a **`connector_action`** (Slack connector) or `http` + webhook for Slack, and a registered function for logic. Stored flows convert + with `os migrate meta --from 16`. **A flow `function` is a PURE compute step — it does NOT read/write the database.** It receives `ctx.input` and **returns** a value; `config.outputVariable`