diff --git a/.changeset/script-node-converged-to-function.md b/.changeset/script-node-converged-to-function.md new file mode 100644 index 000000000..1c953d40a --- /dev/null +++ b/.changeset/script-node-converged-to-function.md @@ -0,0 +1,39 @@ +--- +"@object-ui/app-shell": minor +--- + +Flow designer: the `script` node authors a function call, and nothing else (framework#4343). + +**Breaking for authoring**, not for stored metadata: the `script` panel no longer +offers `Action type`, `Template`, `Recipients`, `Template variables` or the inline +`Code` body. What it offers is the function path — `Function` (required), +`Inputs`, `Output variable` — shown unconditionally, since there is no action type +left to gate them behind. + +framework#4343 retired those five keys because none of them ran. `actionType: +'email' | 'slack'` were logger-backed stubs: they wrote a log line, reported +success, and delivered nothing under any configuration, with `template` / +`recipients` / `variables` addressing a message no channel sent. Inline +`config.script` was recognized and never executed — the built-in runtime has no +server-side JS sandbox. Any other `actionType` value was a second spelling of +`function`. Real delivery is a **`notify`** node (the messaging service: in-app +inbox by default, email once `@objectstack/plugin-email` is installed); Slack is a +**`connector_action`** with the Slack connector, or an `http` node posting to a +webhook. + +**Stored nodes are never hidden.** All five keys keep a legacy render-only field +(`__legacy__` gating — the rule this group already followed for the `code` / `sms` +/ `notification` action types objectui#3099 dropped), each labelled `(retired)` +with its replacement in the help text. `os migrate meta --from 16` rewrites the +metadata; a shorthand `actionType` moves into `function`, which is what it named. + +The flow canvas subtitle now leads with the function name (falling back to the +retired keys so an unmigrated node is never blank), and the simulator says what a +retired branch actually did rather than pretending it mocked a notification. + +The cross-repo reconciliation ledger spans the spec bump: on a spec that still +publishes the retired branches it asserts only that the form offers nothing the +executor ignores; on the spec that retires them (`SCRIPT_BUILTIN_ACTION_TYPES` +disappearing is the discriminator) the full bidirectional comparison arms itself. +Verified against a locally built framework spec: the converged panel reconciles +clean in both directions. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.spec-reconciliation.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.spec-reconciliation.test.ts index 58b39f83c..1a9dea7de 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.spec-reconciliation.test.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.spec-reconciliation.test.ts @@ -41,8 +41,9 @@ const ScriptConfigSchema = spec.ScriptConfigSchema; const SubflowConfigSchema = spec.SubflowConfigSchema; const DecisionConfigSchema = spec.DecisionConfigSchema; const DecisionConditionSchema = spec.DecisionConditionSchema; +// Also the #4343 discriminator: the spec that converges `script` removes this +// constant along with the dispatch branches it described. const SCRIPT_BUILTIN_ACTION_TYPES = spec.SCRIPT_BUILTIN_ACTION_TYPES as readonly string[] | undefined; -const SCRIPT_INVOKE_FUNCTION_ACTION_TYPE = spec.SCRIPT_INVOKE_FUNCTION_ACTION_TYPE as string | undefined; /** * Keys a Zod object schema accepts, read straight off `.shape`. @@ -132,20 +133,55 @@ function reconcile(type: string, zod: unknown, renderOnly: Record { - it('offers exactly the executor-read keys; inline `script` stays render-only (a recognized no-op)', () => { - reconcile('script', ScriptConfigSchema, { - script: 'recognized but NOT executed by the built-in runtime (no server-side JS sandbox) — renders for stored nodes, steers authors to `function`', - }); +/** + * The `script` panel spans a spec bump, so it asserts what is true on EITHER + * side of it (framework#4343). + * + * The form has converged to the one thing the node does — call a registered + * function — and the five dispatch keys it used to offer are legacy render-only + * here. On the spec that retires them those keys leave the contract too + * (`zodKeys` drops `[REMOVED]` tombstones), so the full bidirectional ledger + * applies. On the spec still installed today they are live contract keys the + * form no longer offers, and only the "offers nothing the executor ignores" + * direction is meaningful — asserting the other one would demand the form keep + * authoring branches that never delivered anything. + * + * `SCRIPT_BUILTIN_ACTION_TYPES` is the discriminator: framework#4343 removes it + * along with the branches it described, so this arms itself on the bump. + */ +const SPEC_PREDATES_SCRIPT_CONVERGENCE = SCRIPT_BUILTIN_ACTION_TYPES !== undefined; + +describe.skipIf(!ScriptConfigSchema)('script form ↔ ScriptConfigSchema (framework#4278, #4343)', () => { + it.skipIf(SPEC_PREDATES_SCRIPT_CONVERGENCE)('offers exactly the executor-read keys', () => { + reconcile('script', ScriptConfigSchema); + }); + + it.skipIf(!SPEC_PREDATES_SCRIPT_CONVERGENCE)( + 'offers nothing the executor ignores (pre-#4343 spec: the retired branches are still contract keys)', + () => { + const contract = zodKeys(ScriptConfigSchema); + expect( + offeredConfigKeys('script').filter((k) => !contract.includes(k)), + 'script: offered by the designer form but never read by the executor', + ).toEqual([]); + }, + ); + + it('offers the function path and nothing else', () => { + // The whole authorable surface, on either spec. `timeoutMs` is node-level, + // so `offeredConfigKeys` (config-rooted only) does not carry it. + expect(offeredConfigKeys('script')).toEqual(['function', 'inputs', 'outputVariable']); }); - it('offers exactly the published action types: invoke_function + the built-in set', () => { - const actionType = fieldsForNodeType('script').find((f) => f.id === 'actionType')!; - expect(actionType.options!.map((o) => o.value).sort()).toEqual( - [SCRIPT_INVOKE_FUNCTION_ACTION_TYPE!, ...SCRIPT_BUILTIN_ACTION_TYPES!].sort(), - ); - // The default is the path that runs real logic, not the no-op. - expect(actionType.defaultValue).toBe(SCRIPT_INVOKE_FUNCTION_ACTION_TYPE); + it('keeps every retired key rendering for stored nodes, without offering it', () => { + // Stored metadata is never hidden — the same rule that kept the inline + // `script` body visible after #3099 dropped it from new authoring. + for (const key of ['actionType', 'template', 'recipients', 'variables', 'script']) { + const field = fieldsForNodeType('script').find((f) => f.path[0] === 'config' && f.path[1] === key); + expect(field, `script: retired key '${key}' must keep a field so stored values render`).toBeDefined(); + expect(isLegacyGated(field!), `script: '${key}' must be legacy-gated, not offered`).toBe(true); + expect(field!.help, `script: '${key}' must name its replacement`).toMatch(/[Rr]etired in spec 17/); + } }); }); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts index c6c157bc4..11c7bdec7 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.test.ts @@ -211,54 +211,55 @@ describe('loop / map collection is a template, not a CEL predicate', () => { }); }); -describe('script node — the form authors what the executor runs (framework#4278)', () => { +describe('script node — the form authors what the executor runs (framework#4278, #4343)', () => { const fields = fieldsForNodeType('script'); - const actionType = fields.find((f) => f.id === 'actionType')!; - it('offers Call function / Email / Slack — not the broken sms / notification / no-op code', () => { - // `sms` / `notification` were in no dispatch set: the executor resolved - // them as function names and failed every run. `code` was a recognized - // no-op (no server-side JS sandbox). The offered options now mirror the - // executor's SCRIPT_BUILTIN_ACTION_TYPES + the invoke_function marker. - expect(actionType.options!.map((o) => o.value)).toEqual(['invoke_function', 'email', 'slack']); - expect(actionType.defaultValue).toBe('invoke_function'); - }); - - it('authors the function path — the one that runs real logic', () => { + it('authors the function path — the only thing the node does', () => { for (const id of ['function', 'inputs', 'outputVariable']) { const f = fields.find((x) => x.id === id); expect(f, `script.${id} must be authorable`).toBeDefined(); expect(f!.path).toEqual(['config', id]); - // Shown by default (invoke_function is the default action type). + // Unconditional now: there is no action type left to gate them behind. + expect(f!.showWhen).toBeUndefined(); expect(isFieldVisible(f!, { id: 's', type: 'script' }, fields)).toBe(true); } expect(fields.find((f) => f.id === 'inputs')!.kind).toBe('keyValue'); }); - it('gates the builtin side-effect fields to email / slack', () => { - const template = fields.find((f) => f.id === 'template')!; - expect(template.showWhen).toEqual({ field: 'actionType', equals: ['email', 'slack'] }); - expect(isFieldVisible(template, { id: 's', type: 'script', config: { actionType: 'slack' } }, fields)).toBe(true); - expect(isFieldVisible(template, { id: 's', type: 'script' }, fields)).toBe(false); + it('offers nothing else under config — the retired branches are not authorable', () => { + // framework#4343: `actionType`'s built-in side effects were logger-backed + // stubs that delivered nothing, inline `script` was never executed, and any + // other action type was a second spelling of `function`. + const offered = fields + .filter((f) => f.path[0] === 'config' && f.showWhen?.field !== '__legacy__') + .map((f) => f.id); + expect(offered).toEqual(['function', 'inputs', 'outputVariable']); }); it('drops the dead plural outputVariables field (declared-but-unread — nothing ever bound it)', () => { expect(fields.find((f) => f.id === 'outputVariables')).toBeUndefined(); }); - it('keeps the inline script body render-only: hidden for new nodes, visible when stored', () => { - const script = fields.find((f) => f.id === 'script')!; - expect(script.showWhen).toEqual({ field: '__legacy__', equals: [] }); - expect(isFieldVisible(script, { id: 's', type: 'script' }, fields)).toBe(false); - expect( - isFieldVisible(script, { id: 's', type: 'script', config: { script: 'return 1;' } }, fields), - ).toBe(true); - }); - - it('a stored legacy sms node still renders its builtin fields (stored values are never hidden)', () => { - const template = fields.find((f) => f.id === 'template')!; - const node = { id: 's', type: 'script', config: { actionType: 'sms', template: 'notify_owner' } }; - expect(isFieldVisible(template, node, fields)).toBe(true); + it.each(['actionType', 'template', 'recipients', 'variables', 'script'])( + 'keeps retired `%s` render-only: hidden for new nodes, visible when stored', + (id) => { + const field = fields.find((f) => f.id === id)!; + expect(field, `script.${id} must still render stored values`).toBeDefined(); + expect(field.showWhen).toEqual({ field: '__legacy__', equals: [] }); + expect(isFieldVisible(field, { id: 's', type: 'script' }, fields)).toBe(false); + expect( + isFieldVisible(field, { id: 's', type: 'script', config: { [id]: 'stored' } }, fields), + ).toBe(true); + }, + ); + + it('a stored legacy email node still renders everything it carries', () => { + // Stored values are never hidden — the rule that already covered the + // `code` / `sms` / `notification` action types #3099 dropped. + const node = { id: 's', type: 'script', config: { actionType: 'email', template: 'notify_owner' } }; + for (const id of ['actionType', 'template']) { + expect(isFieldVisible(fields.find((f) => f.id === id)!, node, fields), id).toBe(true); + } }); }); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts index 8363c5698..25e3cfc8a 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts +++ b/packages/app-shell/src/views/metadata-admin/inspectors/flow-node-config.ts @@ -480,70 +480,55 @@ const FLOW_NODE_CONFIG: Record = { cfg('outputVariable', 'Output variable', 'text', { placeholder: 'response' }), { id: 'timeoutMs', path: ['timeoutMs'], label: 'Timeout (ms)', kind: 'number', placeholder: '30000' }, ], - // Script — a callable step (framework#1870): call a registered function - // (`function` + `inputs` + `outputVariable` — the only path that runs real - // logic), or one of the executor's built-in side effects (email / slack — - // `template` / `recipients` / `variables`). The offered options mirror the - // spec's `SCRIPT_BUILTIN_ACTION_TYPES` + the `invoke_function` marker, and - // the whole group is reconciled against the spec-published - // `ScriptConfigSchema` (framework#4278) — before that reconciliation this - // group offered `sms` / `notification` (fail every run: neither is built in, - // so they resolve as function names), defaulted to `code` (a recognized - // no-op: the built-in runtime has no server-side JS sandbox), declared an - // `outputVariables` list nothing reads (the executor binds the singular - // `outputVariable`), and could not author the function path at all. + // Script — one thing: call a registered function (framework#1870). + // `function` + `inputs` + `outputVariable` is the whole authorable surface, + // reconciled against the spec-published `ScriptConfigSchema` (framework#4278). // - // A stored legacy node still renders completely: unknown `actionType` values - // (`code` / `sms` / `notification`) show as "(deprecated)" select options, - // and the `script` body / builtin fields surface whenever they hold a value - // (stored values are never hidden). + // framework#4343 retired the other dispatch branches, and the form follows. + // None of them ran: `actionType: 'email' | 'slack'` were logger-backed stubs + // that reported success and delivered nothing (with `template` / `recipients` + // / `variables` addressing a message no channel sent), inline `script` was + // never executed (the built-in runtime has no server-side JS sandbox), and + // any other `actionType` was a second spelling of `function`. Real delivery + // is a `notify` node; Slack is a connector (or an `http` webhook). + // + // The five keys stay as legacy render-only fields (`__legacy__` never + // matches, so they are never OFFERED) because a stored node must still show + // everything it carries — the rule this group already followed for the + // `code` / `sms` / `notification` action types #3099 dropped. Each carries + // the replacement in its help text; `os migrate meta --from 16` rewrites the + // stored metadata. script: [ - cfg('actionType', 'Action type', 'select', { - options: [ - { value: 'invoke_function', label: 'Call function' }, - { value: 'email', label: 'Email' }, - { value: 'slack', label: 'Slack' }, - ], - defaultValue: 'invoke_function', - help: 'How this step runs. "Call function" invokes a registered function — the path that runs real logic.', - }), cfg('function', 'Function', 'text', { placeholder: 'score_lead', - help: 'Registered function to call — declared via defineStack({ functions }). Always wins over Action type.', - showWhen: { field: 'actionType', equals: ['invoke_function'] }, + help: 'Registered function to call — declared via defineStack({ functions }). Required: it is what this step runs.', }), cfg('inputs', 'Inputs', 'keyValue', { help: 'Values passed to the function; {var} references resolve against the live flow variables.', - showWhen: { field: 'actionType', equals: ['invoke_function'] }, }), cfg('outputVariable', 'Output variable', 'text', { placeholder: 'aiResult', help: "Flow variable the function's return value is bound to, for later steps.", - showWhen: { field: 'actionType', equals: ['invoke_function'] }, }), - cfg('template', 'Template', 'reference', { - // Polymorphic: an email step picks from the email-template catalog; slack - // has no flat catalog yet, so it degrades to free text. - ref: { kindFrom: 'actionType', map: { email: 'email-template' } }, - placeholder: 'case_escalated', - help: 'Message template id.', - showWhen: { field: 'actionType', equals: ['email', 'slack'] }, + cfg('actionType', 'Action type (retired)', 'text', { + help: 'Retired in spec 17 — "email"/"slack" never delivered anything, and any other value was just the function name. Use a notify node for messages, a Slack connector for Slack, or move the name into Function.', + showWhen: { field: '__legacy__', equals: [] }, }), - cfg('recipients', 'Recipients', 'stringList', { - help: 'One recipient per row (user id, field ref, or address).', - showWhen: { field: 'actionType', equals: ['email', 'slack'] }, + cfg('template', 'Template (retired)', 'text', { + help: 'Retired in spec 17 — it fed a side effect that never rendered or sent a message. A notify node carries its own title/message.', + showWhen: { field: '__legacy__', equals: [] }, + }), + cfg('recipients', 'Recipients (retired)', 'stringList', { + help: 'Retired in spec 17 — these addresses were logged, never messaged. Use a notify node, whose recipients reach the messaging service.', + showWhen: { field: '__legacy__', equals: [] }, }), - cfg('variables', 'Template variables', 'keyValue', { - help: 'Values injected into the template.', - showWhen: { field: 'actionType', equals: ['email', 'slack'] }, + cfg('variables', 'Template variables (retired)', 'keyValue', { + help: 'Retired in spec 17 — injected into a template nothing rendered. A notify node carries structured data in payload.', + showWhen: { field: '__legacy__', equals: [] }, }), - // Legacy render-only (`__legacy__` never matches): the built-in runtime - // does NOT execute inline script bodies (no server-side JS sandbox — the - // executor warns and completes as a no-op), so the field is not offered - // for new authoring; a stored body still renders so nothing is hidden. - cfg('script', 'Code (not executed)', 'textarea', { + cfg('script', 'Code (not executed, retired)', 'textarea', { placeholder: 'return { ok: true };', - help: 'Inline scripts are NOT executed by the built-in runtime — this node is a no-op. Move the logic into a registered function and use "Call function".', + help: 'Retired in spec 17 — inline scripts were NEVER executed by the built-in runtime (no server-side sandbox). Move the logic into a registered function and name it in Function.', refMode: 'expression', showWhen: { field: '__legacy__', equals: [] }, }), diff --git a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx index 96d9d100c..3e37c4e3d 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx +++ b/packages/app-shell/src/views/metadata-admin/previews/FlowCanvas.tsx @@ -950,7 +950,10 @@ function nodeSummary(node: FlowNode): string | undefined { return pick('condition'); } if (node.type === 'script') { - return pick('actionType') || pick('template') || (c && c.script ? 'code' : undefined); + // The function IS the step (framework#4343). The rest are retired keys a + // stored node may still carry — kept as fallbacks so its subtitle is never + // blank before someone migrates it. + return pick('function') || pick('actionType') || pick('template') || (c && c.script ? 'code' : undefined); } if (node.type === 'approval') { const approvers = c?.approvers; diff --git a/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts b/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts index 4e8b6a41e..b5e434e53 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/simulator/flow-simulator.ts @@ -407,12 +407,16 @@ export class FlowSimulator { private mockNote(node: SimNode): string { if (node.type === 'script') { + // A script node calls a registered function and nothing else + // (framework#4343) — the branches below describe a stored node that has + // not been migrated yet, and say plainly that they never ran. + const fn = str(node.config?.function); + if (fn) return `Mocked call to '${fn}' (no function executed).`; const action = str(node.config?.actionType); if (action && action !== 'code') { - const recips = Array.isArray(node.config?.recipients) ? (node.config!.recipients as unknown[]).length : 0; - return `Mocked ${action} notification${recips ? ` to ${recips} recipient(s)` : ''}.`; + return `Retired '${action}' action — it never delivered anything; use a notify node (or a connector for Slack).`; } - return 'Mocked code script (no real code executed).'; + return 'Retired inline script — the runtime never executed it; move the logic into a registered function.'; } return `Mocked ${node.type.replace(/_/g, ' ')} (no backend call).`; }