diff --git a/.changeset/flow-node-expression-ledger.md b/.changeset/flow-node-expression-ledger.md new file mode 100644 index 0000000000..3b3d5b94b9 --- /dev/null +++ b/.changeset/flow-node-expression-ledger.md @@ -0,0 +1,48 @@ +--- +'@objectstack/spec': minor +'@objectstack/service-automation': minor +'@objectstack/lint': minor +--- + +Validate the expression slots a flow node's `configSchema` declares (#4027). + +A node type's designer `configSchema` and the keys its validators traverse were +two unreconciled lists. Both the engine's `registerFlow` pass and the author-time +`objectstack validate` pass hardcoded `config.condition` / `edge.condition` and +assumed every other node string was a `{var}` template — so a declared expression +property outside that hardcoded set was validated by nobody. + +That is how #3528 shipped. `screen.fields[].visibleWhen` has been on the `screen` +descriptor since #3304, typed `xExpression: 'expression'` (bare CEL) and offered +to authors in Studio, but no validator traversed it. An app authored the +predicate in the *other* dialect — `'{createOpportunity} == true'` — and it passed +`tsc`, `objectstack validate` and registration in silence. Because `required` *is* +enforced, a field the author had made conditional rendered unconditionally and +blocked Submit on an input the user was never shown: the run paused forever and no +resume was ever issued. + +Now: + +- **`FLOW_NODE_EXPRESSION_PATHS`** (`@objectstack/spec`) is the declared ledger of + expression-bearing node config paths, each recording the dialect it takes. +- **Both validators read it.** A malformed `visibleWhen` is a located, quoted + error at `registerFlow` *and* at `objectstack validate` — `node 'screen_1' + (screen) screen field visibleWhen at config.fields[1].visibleWhen`. +- **A reconciliation ratchet** derives the expression properties from the live + descriptors and fails CI in both directions: a new `xExpression` property with + no ledger entry, or a stale entry no descriptor declares. It walks every + registered builtin, not just `screen`. + +Dialects are recorded rather than assumed because there are three, and two of them +disagree about braces: bare CEL (`{…}` is the #1491 brace-trap), single-brace +`{var}` flow interpolation (`{…}` is correct), and the ADR-0032 §3 double-brace +text template. Only bare-CEL slots are checked — `loop.collection` and +`map.collection` are recorded as `flow-template` and deliberately left alone, +since no validator implements their dialect and checking them under either of the +other two would reject every currently-valid flow. + +`ActionDescriptor.configSchema`'s TSDoc no longer claims `registerFlow()` +validates `config` against it. It never did: `FlowNodeSchema.config` is +`z.record(z.unknown())`, so types, `required`, `enum` and unknown keys are still +unenforced. The doc now states exactly what is checked and what is designer-facing +only, so nothing relies on a guard that does not exist. diff --git a/content/docs/references/automation/node-executor.mdx b/content/docs/references/automation/node-executor.mdx index 682a955249..e87cc1b918 100644 --- a/content/docs/references/automation/node-executor.mdx +++ b/content/docs/references/automation/node-executor.mdx @@ -70,7 +70,7 @@ Canonical cross-paradigm action/node descriptor (ADR-0018) | **icon** | `string` | optional | Icon id resolved by the designer | | **category** | `Enum<'logic' \| 'data' \| 'io' \| 'human' \| 'control' \| 'custom'>` | ✅ | Palette category | | **paradigms** | `Enum<'flow' \| 'approval'>[]` | ✅ | Authoring surfaces that may offer this action | -| **configSchema** | `any` | optional | JSON Schema for the node config (drives form + parse validation) | +| **configSchema** | `any` | optional | JSON Schema for the node config (drives the designer form; only declared expression slots are validated) | | **supportsPause** | `boolean` | ✅ | Supports async pause/resume | | **supportsCancellation** | `boolean` | ✅ | Supports cancellation | | **supportsRetry** | `boolean` | ✅ | Supports retry on failure | diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index d727b5e265..9f69b79cb0 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -521,4 +521,92 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { // The ADR-0062 D7 `field.columnName`-on-external-objects lint was removed with // `field.columnName` itself (#2377): external column mapping is `external.columnMap`. + + /** + * Descriptor-declared expression slots (#4027). Before this, the flow walk + * hardcoded `config.condition` and assumed every other node string was a + * template, so `screen.fields[].visibleWhen` — declared bare CEL since #3304 — + * was validated by nobody and #3528 shipped the wrong dialect through + * `objectstack validate` in silence. + */ + describe('descriptor-declared expression slots (#4027)', () => { + const screenFlow = (fields: unknown[]) => ({ + objects, + flows: [{ + name: 'lead_conversion', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead' } }, + { id: 'screen_1', type: 'screen', config: { fields } }, + ], + edges: [], + }], + }); + + it('flags a `{var}` template dialect in a screen field visibleWhen', () => { + // The exact predicate HotCRM shipped (#3528). + const issues = validateStackExpressions(screenFlow([ + { name: 'createOpportunity', type: 'boolean', required: true }, + { name: 'opportunityName', type: 'text', required: true, visibleWhen: '{createOpportunity} == true' }, + ])); + const found = issues.filter(i => i.where.includes('visibleWhen')); + expect(found).toHaveLength(1); + expect(found[0].severity).toBe('error'); + expect(found[0].where).toContain("flow 'lead_conversion'"); + expect(found[0].where).toContain("node 'screen_1'"); + // Indexed into the repeater, so the author knows WHICH field. + expect(found[0].where).toContain('config.fields[1].visibleWhen'); + expect(found[0].source).toBe('{createOpportunity} == true'); + }); + + it('passes the corrected bare-CEL predicate', () => { + const issues = validateStackExpressions(screenFlow([ + { name: 'createOpportunity', type: 'boolean', required: true, defaultValue: false }, + { name: 'opportunityName', type: 'text', required: true, visibleWhen: 'createOpportunity == true' }, + ])); + expect(issues.filter(i => i.where.includes('visibleWhen'))).toHaveLength(0); + }); + + it('does not report the screen field names as unknown object fields', () => { + // A screen's `visibleWhen` binds the screen's OWN collected values, not the + // trigger record's fields — so no schema hint is passed. Were one passed, + // `createOpportunity` would be flagged as an unknown `crm_lead` field and + // every correct predicate would carry a spurious finding. + const issues = validateStackExpressions(screenFlow([ + { name: 'createOpportunity', type: 'boolean' }, + { name: 'opportunityName', visibleWhen: 'createOpportunity == true' }, + ])); + expect(issues).toHaveLength(0); + }); + + it('leaves a correct single-brace loop collection alone', () => { + // `loop.collection` is the single-brace `{var}` flow-interpolation dialect, + // where braces are CORRECT. It is recorded in the ledger as `flow-template` + // and deliberately not validated — checking it as a CEL predicate (or as an + // ADR-0032 §3 double-brace template) would fail every valid flow. + const issues = validateStackExpressions({ + objects, + flows: [{ + name: 'loop_flow', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_lead' } }, + { id: 'loop_1', type: 'loop', config: { collection: '{tasks}', iteratorVariable: 'task' } }, + ], + edges: [], + }], + }); + expect(issues).toHaveLength(0); + }); + + it('tolerates a screen with no fields, a non-array fields, and no config', () => { + expect(validateStackExpressions(screenFlow([]))).toHaveLength(0); + expect(validateStackExpressions({ + objects, + flows: [{ + name: 'f', + nodes: [{ id: 's', type: 'screen', config: { fields: 'nope' } }, { id: 's2', type: 'screen' }], + edges: [], + }], + })).toHaveLength(0); + }); + }); }); diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 67b1f39afa..5b9d3c0cef 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -9,13 +9,16 @@ * from `@objectstack/formula`, so the verdict matches `registerFlow` and the * agent `validate_expression` tool exactly. * - * Scope (v1): flow predicates (start/decision `config.condition` + edge - * `condition`), object validation-rule / formula predicates, and UI action - * `visible` / `disabled` predicates. Each error is located (flow/object/action - * + node/edge/field) with a corrective message. + * Scope: flow predicates (start/decision `config.condition` + edge `condition`), + * every **descriptor-declared** expression slot named by + * `FLOW_NODE_EXPRESSION_PATHS` (#4027 — e.g. a screen field's `visibleWhen`), + * object validation-rule / formula predicates, and UI action `visible` / + * `disabled` predicates. Each error is located (flow/object/action + + * node/edge/field) with a corrective message. */ import { validateExpression } from '@objectstack/formula'; +import { resolveFlowNodeExpressions } from '@objectstack/spec/automation'; export interface ExprIssue { where: string; @@ -111,6 +114,18 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); }; + /** + * A declared bare-CEL slot (#4027). No object schema is passed: these slots + * bind the *screen's own* collected values, not the trigger record's fields, so + * a field-existence pass would report every field name as unknown. + */ + const checkDeclaredPredicate = (where: string, raw: unknown): void => { + if (raw == null) return; + const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }); + for (const e of res.errors) issues.push({ where, message: e.message, source: e.source, severity: 'error' }); + for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); + }; + // ── Flows ────────────────────────────────────────────────────────── for (const flow of asArray(stack.flows)) { const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)'; @@ -124,6 +139,26 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const node of nodes) { const cfg = (node.config ?? {}) as AnyRec; check(`flow '${flowName}' · node '${node.id}' (${node.type}) condition`, cfg.condition, objectName); + + // Descriptor-declared expression slots (#4027). Before this, the traversal + // hardcoded `condition` and assumed every other node string was a `{var}` + // template — so `screen.fields[].visibleWhen`, declared bare CEL since + // #3304, was validated by nobody and #3528 shipped a template-dialect + // predicate through compile, validate and run time in silence. + // Only `predicate` slots are checkable: `flow-template` slots take the + // single-brace `{var}` dialect `interpolate()` implements, which no + // validator covers (the `template` role enforces ADR-0032 §3's + // double-brace text template and would reject every correct + // `loop.collection`). The ledger records them regardless, so the + // reconciliation ratchet still sees the marker. + const nodeType = typeof node.type === 'string' ? node.type : ''; + for (const found of resolveFlowNodeExpressions(nodeType, cfg)) { + if (found.entry.role !== 'predicate') continue; + checkDeclaredPredicate( + `flow '${flowName}' · node '${node.id}' (${nodeType}) ${found.entry.label} at config.${found.path}`, + 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, diff --git a/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts b/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts new file mode 100644 index 0000000000..67a42ef97b --- /dev/null +++ b/packages/services/service-automation/src/builtin/config-expression-ledger.test.ts @@ -0,0 +1,191 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * **configSchema ↔ validation reconciliation** (#4027) — the ratchet that keeps + * a designer-declared expression property from going unvalidated. + * + * Two hand-written lists describe a builtin node's config: the designer + * `configSchema` its executor publishes, and whatever the validators happen to + * traverse. Nothing reconciled them, so a property could live in the first and + * be absent from the second indefinitely — which is exactly how #3528 shipped. + * `screen.fields[].visibleWhen` was on the designer form from #3304, typed + * `xExpression: 'expression'` (bare CEL) and offered to authors in Studio, while + * both validators hardcoded `config.condition` and assumed every other node + * string was a `{var}` template. The key had zero coverage on either side. + * + * This test derives the expression properties from the LIVE descriptors and + * asserts the ledger in `@objectstack/spec` matches exactly, in both directions: + * + * - a new `xExpression` property with no ledger entry FAILS — the #3528 shape, + * caught at CI instead of by an app whose screen cannot be submitted; + * - a stale ledger entry for a property no descriptor declares also FAILS, so + * the ledger cannot rot into a list of paths that no longer exist. + * + * It walks **every** registered builtin — not just `screen` — which is the audit + * the original triage never did. + */ + +import { describe, it, expect } from 'vitest'; +import { + FLOW_NODE_EXPRESSION_PATHS, + resolveFlowNodeExpressions, + type FlowNodeExpressionRole, +} from '@objectstack/spec/automation'; +import { AutomationEngine } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} +function ctx() { + return { logger: silentLogger(), getService() { throw new Error('none'); } } as any; +} + +interface SchemaNode { + type?: string; + properties?: Record; + items?: SchemaNode; + xExpression?: string; +} + +/** + * `xExpression` marker → the dialect the ledger records for that slot. + * + * `'template'` maps to `flow-template` (single-brace `{var}`, resolved by + * `interpolate()`), NOT to `validateExpression`'s `'template'` role — that role + * enforces the ADR-0032 §3 double-brace text template, a different dialect that + * rejects a single `{x}`. Conflating the two would fail every valid + * `loop.collection`. + */ +const ROLE_BY_MARKER: Record = { + expression: 'predicate', + template: 'flow-template', +}; + +/** + * Collect every `xExpression`-marked property in a configSchema, as the same + * `key[].nested` path syntax the ledger uses. + */ +function collectExpressionProps( + schema: SchemaNode | undefined, + prefix = '', +): { path: string; marker: string }[] { + if (!schema || typeof schema !== 'object') return []; + const out: { path: string; marker: string }[] = []; + + if (schema.properties) { + for (const [key, prop] of Object.entries(schema.properties)) { + if (!prop || typeof prop !== 'object') continue; + // An array of objects contributes `key[]`, matching the ledger's syntax + // for "every element of this array" (`fields[].visibleWhen`). + const isObjectArray = prop.type === 'array' && !!prop.items; + const here = prefix + ? `${prefix}.${key}${isObjectArray ? '[]' : ''}` + : `${key}${isObjectArray ? '[]' : ''}`; + if (typeof prop.xExpression === 'string') out.push({ path: here, marker: prop.xExpression }); + if (isObjectArray) out.push(...collectExpressionProps(prop.items, here)); + else out.push(...collectExpressionProps(prop, here)); + } + } + return out; +} + +const engine = new AutomationEngine(silentLogger()); +installBuiltinNodes(engine, ctx()); + +/** Every declared expression slot, derived from the live descriptors. */ +function declaredFromDescriptors(): { nodeType: string; path: string; role: FlowNodeExpressionRole }[] { + const found: { nodeType: string; path: string; role: FlowNodeExpressionRole }[] = []; + for (const descriptor of engine.getActionDescriptors()) { + const schema = descriptor.configSchema as SchemaNode | undefined; + for (const { path, marker } of collectExpressionProps(schema)) { + const role = ROLE_BY_MARKER[marker]; + expect( + role, + `${descriptor.type}.${path} declares an unknown xExpression marker '${marker}' — ` + + `add it to ROLE_BY_MARKER and teach the validators which dialect it takes`, + ).toBeDefined(); + found.push({ nodeType: descriptor.type, path, role: role! }); + } + } + return found; +} + +const key = (e: { nodeType: string; path: string; role: string }) => `${e.nodeType}.${e.path} (${e.role})`; + +describe('configSchema ↔ expression-ledger reconciliation (#4027)', () => { + it('every xExpression property a builtin declares is in the ledger', () => { + const declared = declaredFromDescriptors(); + // Sanity: if this ever empties, the derivation broke and the whole ratchet + // would pass vacuously — the failure mode a ledger test must not have. + expect(declared.length, 'no xExpression properties found — derivation is broken').toBeGreaterThan(0); + + const ledger = new Set(FLOW_NODE_EXPRESSION_PATHS.map(key)); + const missing = declared.filter((d) => !ledger.has(key(d))).map(key); + expect( + missing, + `these declared expression properties are validated by NOBODY — add them to ` + + `FLOW_NODE_EXPRESSION_PATHS so registerFlow and objectstack validate check them (#3528 shape)`, + ).toEqual([]); + }); + + it('the ledger carries no path a builtin no longer declares', () => { + const declared = new Set(declaredFromDescriptors().map(key)); + // Structural predicate surfaces (`config.condition`, `edge.condition`) are + // not descriptor properties and are deliberately absent from the ledger, so + // every ledger entry must correspond to a real declared property. + const stale = FLOW_NODE_EXPRESSION_PATHS.map(key).filter((k) => !declared.has(k)); + expect(stale, 'stale ledger entries — the descriptor no longer declares these').toEqual([]); + }); + + it('screen.fields[].visibleWhen is covered — the #3528 regression', () => { + const screen = FLOW_NODE_EXPRESSION_PATHS.find( + (e) => e.nodeType === 'screen' && e.path === 'fields[].visibleWhen', + ); + expect(screen, 'the key whose absence made every HotCRM lead conversion un-submittable').toBeDefined(); + // Bare CEL, not a template: `{createOpportunity} == true` must be an error + // and `createOpportunity == true` must not. + expect(screen!.role).toBe('predicate'); + }); +}); + +describe('resolveFlowNodeExpressions — path resolution (#4027)', () => { + it('resolves each element of a field repeater, with its index', () => { + const found = resolveFlowNodeExpressions('screen', { + fields: [ + { name: 'createOpportunity', type: 'boolean' }, + { name: 'opportunityName', visibleWhen: 'createOpportunity == true' }, + { name: 'opportunityAmount', visibleWhen: 'createOpportunity == true' }, + ], + }); + expect(found.map((f) => f.path)).toEqual([ + 'fields[1].visibleWhen', + 'fields[2].visibleWhen', + ]); + expect(found.every((f) => f.entry.role === 'predicate')).toBe(true); + }); + + it('resolves a top-level flow-template slot, marked as not-validated', () => { + const found = resolveFlowNodeExpressions('loop', { collection: '{tasks}' }); + expect(found).toHaveLength(1); + expect(found[0].path).toBe('collection'); + // `flow-template`, not `predicate` — the consumers skip it, which is what + // keeps a correct single-brace `{tasks}` from being rejected as a CEL brace. + expect(found[0].entry.role).toBe('flow-template'); + }); + + it('skips absent, empty and non-string values rather than inventing findings', () => { + expect(resolveFlowNodeExpressions('screen', {})).toEqual([]); + expect(resolveFlowNodeExpressions('screen', { fields: [] })).toEqual([]); + expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: ' ' }] })).toEqual([]); + // A non-string in an expression slot is a type violation for the schema pass + // to report — not something to hand to a parser. + expect(resolveFlowNodeExpressions('screen', { fields: [{ visibleWhen: true }] })).toEqual([]); + // A repeater authored as a non-array must not throw. + expect(resolveFlowNodeExpressions('screen', { fields: 'nope' })).toEqual([]); + }); + + it('returns nothing for a node type with no declared slots', () => { + expect(resolveFlowNodeExpressions('decision', { condition: 'a == b' })).toEqual([]); + }); +}); diff --git a/packages/services/service-automation/src/builtin/screen-visible-when-validation.test.ts b/packages/services/service-automation/src/builtin/screen-visible-when-validation.test.ts new file mode 100644 index 0000000000..77f12b6c0c --- /dev/null +++ b/packages/services/service-automation/src/builtin/screen-visible-when-validation.test.ts @@ -0,0 +1,141 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `screen.fields[].visibleWhen` is validated at registration (#4027). + * + * The reproduction is #3528's, verbatim. HotCRM authored the predicate in the + * `{var}` template dialect its sibling config keys use correctly: + * + * ```ts + * { name: 'opportunityName', required: true, visibleWhen: '{createOpportunity} == true' } + * ``` + * + * A screen field's `visibleWhen` is **bare CEL**, so that never resolved. The + * consequence was not a cosmetic one: `required` *is* enforced, so a field the + * author had made conditional rendered unconditionally and blocked Submit on an + * input the user was never meant to see. The flow paused forever, no resume was + * ever issued, and no layer complained — `tsc`, `objectstack validate` and + * `registerFlow` all passed it, because no validator traversed the key. + * + * It is now a loud registration error, located and quoted. + */ + +import { describe, it, expect } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { installBuiltinNodes } from './index.js'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} +function ctx() { + return { logger: silentLogger(), getService() { throw new Error('none'); } } as any; +} + +function engineWithBuiltins() { + const engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, ctx()); + return engine; +} + +/** A single-screen flow whose conditional field carries `visibleWhen`. */ +function flowWithVisibleWhen(visibleWhen: string) { + return { + name: 'lead_conversion', + label: 'Lead Conversion', + type: 'screen', + status: 'active', + version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { objectName: 'crm_lead' } }, + { + id: 'screen_1', type: 'screen', label: 'Conversion Details', + config: { + fields: [ + { name: 'createOpportunity', label: 'Create Opportunity?', type: 'boolean', required: true, defaultValue: false }, + { name: 'opportunityName', label: 'Opportunity Name', type: 'text', required: true, visibleWhen }, + ], + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'screen_1', type: 'default' }, + { id: 'e2', source: 'screen_1', target: 'end', type: 'default' }, + ], + }; +} + +describe('screen field visibleWhen validation (#4027, reproducing #3528)', () => { + it('rejects the `{var}` template dialect in a visibleWhen predicate', () => { + const engine = engineWithBuiltins(); + // The exact predicate HotCRM shipped. + expect(() => engine.registerFlow('lead_conversion', flowWithVisibleWhen('{createOpportunity} == true'))) + .toThrow(/template braces|bare CEL/); + }); + + it('locates the offending field, so an author knows which one to fix', () => { + const engine = engineWithBuiltins(); + let message = ''; + try { + engine.registerFlow('lead_conversion', flowWithVisibleWhen('{createOpportunity} == true')); + } catch (e) { + message = (e as Error).message; + } + // Node id, the human label, and the indexed path into the repeater. + expect(message).toContain("node 'screen_1'"); + expect(message).toContain('screen field visibleWhen'); + expect(message).toContain('config.fields[1].visibleWhen'); + // And the source itself, quoted back. + expect(message).toContain('{createOpportunity} == true'); + }); + + it('accepts the corrected bare-CEL predicate', () => { + const engine = engineWithBuiltins(); + expect(() => engine.registerFlow('lead_conversion', flowWithVisibleWhen('createOpportunity == true'))) + .not.toThrow(); + }); + + it('accepts a screen with no visibleWhen at all', () => { + const engine = engineWithBuiltins(); + const flow = flowWithVisibleWhen('createOpportunity == true') as any; + delete flow.nodes[1].config.fields[1].visibleWhen; + expect(() => engine.registerFlow('lead_conversion', flow)).not.toThrow(); + }); + + it('reports every malformed field, not just the first', () => { + const engine = engineWithBuiltins(); + const flow = flowWithVisibleWhen('{createOpportunity} == true') as any; + flow.nodes[1].config.fields.push({ + name: 'opportunityAmount', label: 'Amount', type: 'currency', + visibleWhen: '{createOpportunity} == true', + }); + let message = ''; + try { + engine.registerFlow('lead_conversion', flow); + } catch (e) { + message = (e as Error).message; + } + expect(message).toContain('config.fields[1].visibleWhen'); + expect(message).toContain('config.fields[2].visibleWhen'); + expect(message).toMatch(/2 invalid expressions/); + }); + + it('still accepts a `{var}` template where a template is what is declared', () => { + // The dialects are opposite, and the ledger records which is which: braces + // are the brace-trap in `visibleWhen` and the CORRECT spelling in + // `loop.collection`. A dialect-blind check would break this flow. + const engine = engineWithBuiltins(); + expect(() => engine.registerFlow('loop_flow', { + name: 'loop_flow', label: 'Loop', type: 'screen', status: 'active', version: 1, + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: {} }, + { id: 'loop_1', type: 'loop', label: 'Loop', config: { collection: '{tasks}', iteratorVariable: 'task' } }, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'loop_1', type: 'default' }, + { id: 'e2', source: 'loop_1', target: 'end', type: 'default' }, + ], + })).not.toThrow(); + }); +}); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index ca1740b5b0..2977ba9ab9 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -6,6 +6,7 @@ import type { AutomationContext, AutomationResult, ResumeSignal, IAutomationServ import { RESUME_AUTHORITY_SERVICE } from '@objectstack/spec/contracts'; import type { Logger } from '@objectstack/spec/contracts'; import { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES, validateControlFlow, findRegionEntry, defineActionDescriptor } from '@objectstack/spec/automation'; +import { resolveFlowNodeExpressions } from '@objectstack/spec/automation'; import { applyConversionsToFlow } from '@objectstack/spec'; import type { FlowRegionParsed } from '@objectstack/spec/automation'; import type { Connector, ConnectorProviderFactory } from '@objectstack/spec/integration'; @@ -2723,9 +2724,28 @@ export class AutomationEngine implements IAutomationService { * other malformed predicate LOUDLY, with the offending location + source + * a corrective hint, instead of letting it fail silently at run time. * - * Only the *predicate* surfaces are checked here (start/node `config.condition` - * and `edge.condition`) — node string fields are templates (a different - * dialect) and are validated by the template engine, not as CEL. + * Two families of surface are checked: + * + * 1. The *structural* predicates every flow has — start/node + * `config.condition` and `edge.condition`. Always bare CEL. + * 2. Every **descriptor-declared** bare-CEL slot named by + * `FLOW_NODE_EXPRESSION_PATHS` (#4027) — the ledger records the dialect + * each declared slot takes, and the `predicate` ones are checked here. + * + * (2) exists because assuming "node string fields are templates" is what let + * #3528 ship: `screen.fields[].visibleWhen` is declared *bare CEL*, this pass + * never traversed it, and an app authored it in the `{var}` template dialect + * its sibling config keys use. Nothing complained at any layer. The ledger is + * the declared list of such slots and is reconciled against the live + * descriptors by a test, so a newly declared expression key cannot go + * unvalidated again. + * + * Recording the dialect is what makes (2) safe rather than a regression: + * `{…}` is the #1491 brace-trap in a bare-CEL slot and the *correct* spelling + * in a single-brace `{var}` flow-interpolation slot (`loop.collection`). A + * dialect-blind pass would reject every valid `collection` while still + * passing every wrong `visibleWhen`, so the `flow-template` slots are + * recorded and skipped — no validator implements their dialect. * * #1928 — when an object-schema resolver is wired ({@link setObjectSchemaResolver}), * a second, schema-aware pass surfaces the checks `objectstack build` runs @@ -2751,7 +2771,7 @@ export class AutomationEngine implements IAutomationService { })() : undefined; - const check = (where: string, raw: unknown): void => { + const check = (where: string, raw: unknown, useSchemaHint = true): void => { if (raw == null) return; // Fatal pass — syntax, brace-in-CEL, unknown-function (ADR-0032 §5). // Unchanged: matches the CLI build's fatal set exactly. @@ -2763,7 +2783,7 @@ export class AutomationEngine implements IAutomationService { // valid (else it would just re-report the fatal error). Everything it // finds (field-existence, tier-3 typo, tier-4 type mismatch) is LOGGED, // never thrown, so registration behaviour is strictly additive (#1928). - if (result.errors.length === 0 && schemaHint) { + if (useSchemaHint && result.errors.length === 0 && schemaHint) { const schemaPass = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, schemaHint); for (const issue of [...schemaPass.errors, ...schemaPass.warnings]) { this.logger.warn(`[flow '${flowName}'] ${where}: ${issue.message}\n source: \`${issue.source}\``); @@ -2775,6 +2795,29 @@ export class AutomationEngine implements IAutomationService { const cfg = (node.config ?? {}) as Record; // start-node trigger gate + decision/branch predicates live in config.condition check(`node '${node.id}' (${node.type}) condition`, cfg.condition); + + // Descriptor-declared expression slots (#4027). The ledger names them + // per node type and carries the dialect each one takes, so a declared + // key like `screen.fields[].visibleWhen` is checked as the bare CEL it + // is — the traversal gap that let #3528 ship a `{var}` predicate. + // + // Only `predicate` slots are checked: `flow-template` slots take the + // single-brace `{var}` dialect `interpolate()` implements, and no + // validator implements it (validateExpression's `template` role is the + // ADR-0032 §3 double-brace text template and would reject every + // correct `loop.collection`). They are declared in the ledger so the + // reconciliation ratchet still covers the marker. + for (const found of resolveFlowNodeExpressions(node.type, node.config)) { + if (found.entry.role !== 'predicate') continue; + // No schema hint: a screen's `visibleWhen` binds the screen's OWN + // collected values, not the trigger record's fields, so the + // field-existence pass would report every field name as unknown. + check( + `node '${node.id}' (${node.type}) ${found.entry.label} at config.${found.path}`, + found.value, + false, + ); + } } for (const edge of flow.edges) { check(`edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition as unknown); @@ -2782,8 +2825,10 @@ export class AutomationEngine implements IAutomationService { if (failures.length > 0) { throw new Error( - `Flow '${flowName}' has ${failures.length} invalid condition${failures.length > 1 ? 's' : ''} (ADR-0032 §1a). ` + - `Conditions are bare CEL — do not wrap field references in \`{…}\` template braces:\n${failures.join('\n')}`, + `Flow '${flowName}' has ${failures.length} invalid expression${failures.length > 1 ? 's' : ''} (ADR-0032 §1a). ` + + `Predicates — conditions and declared bare-CEL slots such as a screen field's \`visibleWhen\` — ` + + `must not wrap references in \`{…}\` template braces; template slots (e.g. \`loop.collection\`) require them:\n` + + `${failures.join('\n')}`, ); } } diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 10b9a430cf..73168b7fd7 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2090,6 +2090,7 @@ "ExecutionStepLogParsed (type)", "ExecutionStepLogSchema (const)", "FLOW_BUILTIN_NODE_TYPES (const)", + "FLOW_NODE_EXPRESSION_PATHS (const)", "FLOW_STRUCTURAL_NODE_TYPES (const)", "Flow (type)", "FlowEdge (type)", @@ -2097,6 +2098,8 @@ "FlowEdgeSchema (const)", "FlowNode (type)", "FlowNodeAction (const)", + "FlowNodeExpressionPath (interface)", + "FlowNodeExpressionRole (type)", "FlowNodeParsed (type)", "FlowNodeSchema (const)", "FlowParsed (type)", @@ -2133,6 +2136,7 @@ "ParallelConfigParsed (type)", "ParallelConfigSchema (const)", "RegionAnalysis (interface)", + "ResolvedFlowNodeExpression (interface)", "RetryPolicy (type)", "RetryPolicySchema (const)", "ScheduleState (type)", @@ -2187,6 +2191,7 @@ "getApprovalNodeConfigJsonSchema (function)", "importBpmnToConstructs (function)", "normalizeDecisionOutputs (function)", + "resolveFlowNodeExpressions (function)", "validateControlFlow (function)" ], "./api": [ diff --git a/packages/spec/src/automation/flow-node-expression-paths.ts b/packages/spec/src/automation/flow-node-expression-paths.ts new file mode 100644 index 0000000000..36cd232e22 --- /dev/null +++ b/packages/spec/src/automation/flow-node-expression-paths.ts @@ -0,0 +1,193 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * @module automation/flow-node-expression-paths + * + * **Ledger of expression-bearing flow-node config paths** (#4027). + * + * A flow node's `config` is `z.record(z.unknown())` — deliberately open, because + * node types are extensible and each owns its own config shape. The consequence + * is that the *only* machine-readable description of a node's config is the + * designer `configSchema` its executor publishes, and nothing connected that to + * validation. Two separate validators — the engine's `registerFlow` pass and the + * author-time `@objectstack/lint` pass — each hardcoded the list of config keys + * that hold expressions (`config.condition`, `edge.condition`) rather than + * asking the descriptor. A declared expression key outside that hardcoded list + * was therefore never validated by anyone. + * + * That is exactly how #3528 shipped. `screen.fields[].visibleWhen` is declared + * bare CEL (`xExpression: 'expression'` on the `screen` descriptor since #3304), + * but neither validator traversed it, so an app authored the predicate in the + * *other* dialect — `'{createOpportunity} == true'`, the `{var}` template form + * used correctly by sibling config keys — and it passed `tsc`, passed + * `objectstack validate`, passed registration, and dead-ended the flow's screen + * at run time with no diagnostic anywhere. + * + * This ledger is the declared source both validators now read, mirroring the + * dispatcher↔client route ledger of #3569: one list, two consumers, plus a + * reconciliation test that fails when a descriptor's `configSchema` declares an + * `xExpression` property this ledger does not carry + * (`config-expression-ledger.test.ts` in `service-automation`). A new expression + * key can no longer be added to a designer form and silently go unvalidated. + * + * ## Why the dialect must be recorded, not assumed + * + * `xExpression` takes two values that mean **opposite** things about braces, and + * there are in fact *three* dialects in play across the platform: + * + * - `'expression'` → **bare CEL**. `{…}` is the #1491 brace-trap and a hard error. + * - `'template'` → **single-brace `{var}` flow interpolation**, the dialect + * `interpolate()` implements for flow node config. `{…}` is the *correct* + * spelling here. + * - The **double-brace `{{ }}` text template** of ADR-0032 §3, which is what + * `validateExpression`'s `'template'` role enforces — a different dialect from + * the one above, and not the one flow config uses. + * + * A dialect-blind check would be worse than none: it would reject every correct + * `loop.collection` while still passing every wrong `visibleWhen`. Each entry + * therefore records the dialect, and only those with a validator that actually + * implements that dialect are checked. + */ + +/** + * The dialect a declared expression slot takes — and therefore what, if + * anything, can validate it today. + */ +export type FlowNodeExpressionRole = + /** + * Bare CEL — a boolean predicate. `{…}` braces are the #1491 brace-trap. + * Validated by `validateExpression('predicate', …)` at both registration and + * `objectstack validate`. + */ + | 'predicate' + /** + * Single-brace `{var}` flow interpolation, resolved by `interpolate()` against + * the flow's live variables. + * + * **Declared but not validated.** No validator implements this dialect: the + * `'template'` role of `validateExpression` enforces the ADR-0032 §3 + * *double-brace* text template and rejects a single `{x}` as a legacy hole, so + * routing these slots through it would fail every correct `loop.collection` in + * every existing flow. Recorded here so the reconciliation ratchet still sees + * the marker and a future validator has one place to hook into. + */ + | 'flow-template'; + +/** One expression-bearing config slot on a builtin flow node type. */ +export interface FlowNodeExpressionPath { + /** Registry node type the path belongs to (`node.type`). */ + readonly nodeType: string; + /** + * Dot path into `node.config`, using `[]` for "every element of this array". + * `'fields[].visibleWhen'` reads `config.fields[i].visibleWhen` for each `i`. + */ + readonly path: string; + /** Which dialect this slot takes — decides what counts as malformed. */ + readonly role: FlowNodeExpressionRole; + /** Author-facing label for diagnostics, e.g. `screen field visibleWhen`. */ + readonly label: string; +} + +/** + * Every expression slot declared by a builtin node's `configSchema`. + * + * Kept in sync with the live descriptors by the reconciliation test — add an + * `xExpression` property to a builtin `configSchema` without adding it here and + * CI fails, which is the regression guard #3528 lacked. + * + * Not listed here (deliberately): `config.condition` and `edge.condition`. Those + * are *structural* predicate surfaces on every node and edge rather than + * descriptor-declared config properties, and both validators already walk them. + */ +export const FLOW_NODE_EXPRESSION_PATHS: readonly FlowNodeExpressionPath[] = [ + { + nodeType: 'screen', + path: 'fields[].visibleWhen', + role: 'predicate', + label: 'screen field visibleWhen', + }, + { + nodeType: 'loop', + path: 'collection', + role: 'flow-template', + label: 'loop collection', + }, + { + nodeType: 'map', + path: 'collection', + role: 'flow-template', + label: 'map collection', + }, +]; + +/** One resolved expression value found in a node's config. */ +export interface ResolvedFlowNodeExpression { + /** The ledger entry that matched. */ + readonly entry: FlowNodeExpressionPath; + /** Concrete location, with array indices filled in (`fields[1].visibleWhen`). */ + readonly path: string; + /** The authored value sitting at that location. */ + readonly value: unknown; +} + +/** + * Resolve every expression slot the ledger declares for `nodeType` against a + * concrete `config`, filling in array indices. + * + * Pure path resolution — no validation, no I/O. Both the engine and the lint + * pass call this and then apply their own severity policy (the engine throws on + * a malformed predicate at `registerFlow`; the lint pass reports it as a located + * `objectstack validate` finding), so neither has to know the path shapes. + * + * Absent, `null` and non-string values are skipped: "not authored" is not a + * malformed expression, and a non-string in an expression slot is a *type* + * violation for the schema pass to report, not something to hand to a parser. + */ +export function resolveFlowNodeExpressions( + nodeType: string, + config: unknown, +): ResolvedFlowNodeExpression[] { + if (config == null || typeof config !== 'object') return []; + const out: ResolvedFlowNodeExpression[] = []; + for (const entry of FLOW_NODE_EXPRESSION_PATHS) { + if (entry.nodeType !== nodeType) continue; + walk(config as Record, entry.path.split('.'), '', (path, value) => { + if (typeof value === 'string' && value.trim()) out.push({ entry, path, value }); + }); + } + return out; +} + +/** + * Descend `segments` through `node`, expanding a `key[]` segment over every + * element of that array, and hand each terminal value to `emit` with its + * concrete path. + */ +function walk( + node: unknown, + segments: readonly string[], + prefix: string, + emit: (path: string, value: unknown) => void, +): void { + if (node == null || typeof node !== 'object') return; + const [head, ...rest] = segments; + if (head === undefined) return; + + const isArraySegment = head.endsWith('[]'); + const key = isArraySegment ? head.slice(0, -2) : head; + const value = (node as Record)[key]; + const here = prefix ? `${prefix}.${key}` : key; + + if (isArraySegment) { + if (!Array.isArray(value)) return; + value.forEach((element, index) => { + const indexed = `${here}[${index}]`; + if (rest.length === 0) emit(indexed, element); + else walk(element, rest, indexed, emit); + }); + return; + } + + if (rest.length === 0) emit(here, value); + else walk(value, rest, here, emit); +} diff --git a/packages/spec/src/automation/index.ts b/packages/spec/src/automation/index.ts index 8ff2a3321a..518eae4844 100644 --- a/packages/spec/src/automation/index.ts +++ b/packages/spec/src/automation/index.ts @@ -13,5 +13,6 @@ export * from './time-relative-trigger.zod'; export * from './sync.zod'; export * from './state-machine.zod'; export * from './node-executor.zod'; +export * from './flow-node-expression-paths'; export * from './bpmn-interop.zod'; export * from './bpmn-mapping'; diff --git a/packages/spec/src/automation/node-executor.zod.ts b/packages/spec/src/automation/node-executor.zod.ts index dbb3f7e9c8..94c301cc58 100644 --- a/packages/spec/src/automation/node-executor.zod.ts +++ b/packages/spec/src/automation/node-executor.zod.ts @@ -208,8 +208,9 @@ export type ActionParadigm = z.infer; * * The runtime registry (`AutomationEngine.getActionDescriptors()`) aggregates * these and backs both: - * - **flow validation** (`registerFlow()` checks `node.type` is a registered - * action, and that `config` satisfies `configSchema`), and + * - **flow validation** — `registerFlow()` checks `node.type` is a registered + * action, and validates the expression slots the descriptor declares (see + * {@link configSchema} for exactly how far that goes), and * - the **designer palette** (label / icon / category / config form). * * Keyed by `type` (not `id` + `nodeTypes` like the legacy @@ -239,11 +240,33 @@ export const ActionDescriptorSchema = lazySchema(() => z.object({ // ── config contract ─────────────────────────────────────────────── /** * JSON Schema (compiled from the executor's Zod) describing the node - * `config`. Drives Studio form generation and `registerFlow()` config - * validation. Optional — actions with no config omit it. + * `config`. Drives Studio form generation. Optional — actions with no config + * omit it. + * + * **What actually validates against this, and what does not** (#4027). An + * earlier revision of this doc claimed `registerFlow()` checks that `config` + * satisfies `configSchema`. It does not, and never did — `FlowNodeSchema.config` + * is `z.record(z.unknown())`, so no layer rejects an undeclared or misspelled + * config key at author time. What *is* enforced: + * + * - **Expression slots are validated.** Every property marked + * `xExpression: 'expression'` (bare CEL) is parse-checked at `registerFlow()` + * and by `objectstack validate`, via the `FLOW_NODE_EXPRESSION_PATHS` ledger + * and its reconciliation ratchet — so a declared predicate cannot go + * unvalidated the way `screen.fields[].visibleWhen` did for four months + * (#3528). Slots marked `xExpression: 'template'` are recorded but not + * checked: no validator implements the single-brace `{var}` dialect they use. + * - **Everything else is designer-facing only.** Types, `required`, `enum` and + * unknown keys are not enforced anywhere. Do not rely on this schema as a + * runtime guard; an executor must still validate its own config. + * + * The schema is also hand-written per executor rather than derived from the + * code that reads `config`, so a property can be declared without being read + * (or read without being declared) with nothing to reconcile the two. Only the + * expression subset has a ratchet today. */ configSchema: z.unknown().optional() - .describe('JSON Schema for the node config (drives form + parse validation)'), + .describe('JSON Schema for the node config (drives the designer form; only declared expression slots are validated)'), // ── capabilities ────────────────────────────────────────────────── /** Supports async pause/resume (e.g. wait, human_task). */