diff --git a/.changeset/region-metadata-parity.md b/.changeset/region-metadata-parity.md new file mode 100644 index 0000000000..98d5a11606 --- /dev/null +++ b/.changeset/region-metadata-parity.md @@ -0,0 +1,59 @@ +--- +'@objectstack/spec': patch +'@objectstack/service-automation': patch +'@objectstack/lint': patch +--- + +Flow metadata is canonicalized inside structured regions, not just at the top level (#4347). + +`registerFlow` canonicalizes a stored flow through three passes — the ADR-0087 conversion +table, `FlowSchema.parse`, and the ADR-0032 predicate validation — and every one of them +walked `flow.nodes` / `flow.edges` only. An ADR-0031 container keeps a whole sub-graph in +its open `config` (`loop.config.body`, `parallel.config.branches[]`, +`try_catch.config.try`/`.catch`), so all three stopped at the container and metadata came +out **position-dependent**: the same node converted at the top level and did not one level +in, and the same predicate was stored as a `{ dialect: 'cel', source }` envelope on a +top-level edge and left a bare string on a loop-body edge. + +The reporting app shipped three sweeps whose gates never opened. Each run reported +`success: true`, queried correctly, selected exactly the right records, and then did +nothing — which is indistinguishable from "this sweep had no work to do" unless you assert +on records written. + +- **`mapFlowNodes` recurses into regions**, to any depth. Every conversion in the table now + reaches a nested node, which matters most for the two that change behaviour rather than + spelling: a `webhook` / `http_request` callout inside a loop body kept a type no executor + owns (the run failed), and a `delete_record` kept `config.filters`, leaving the canonical + `filter` the executor reads absent — the erased-condition hazard + `flow-node-crud-filter-alias` exists to prevent. Notice paths carry the region + (`flows[0].nodes[3].config.body.nodes[1].config.filter`), so the warning points at the + node to edit. +- **New `normalizeControlFlowRegions`**, called at the load seam after + `validateControlFlow`: each region is parsed through its own schema (recursively — regions + nest), so nested edges and nodes carry the same canonical shapes as top-level ones. A + region that does not parse is left untouched; rejecting one stays `validateControlFlow`'s + job, so which flows register is unchanged. +- **New `collectFlowGraphs`** yields a flow's own graph plus every nested region, each with + a scope label. Both predicate validators iterate it instead of `flow.nodes` — the engine's + `validateFlowExpressions` and `@objectstack/lint`'s author-time + `validateStackExpressions` — so the `{record.x}` brace-trap they exist to catch is now + caught inside a loop body too, naming the region (`loop 'sweep' body · edge 'b1' …`). It + used to pass `objectstack validate`, pass registration, and fail at run time with the + diagnostic suppressed. + +The container executors already parse their own config at run time (`parseNodeConfig`, +#4277), so a nested predicate did evaluate correctly on current `main` — what was still +wrong is everything that reads a region *without* re-parsing it (the Studio designer, +`getFlow`, the version history), and every conversion, none of which the executors replay. + +Also hardened, per the issue's secondary finding: `evaluateCondition`'s legacy `{var}` +template path **refuses an unresolved dotted reference** instead of comparing it as a +string. `'oppRecord.amount > 500000'` was compared `'oppRecord.amount' > '500000'` — `'o'` +against `'5'` — so it was constantly true regardless of the amount: silently wrong in the +*true* direction, a gate that reports success while never gating. It now throws with the +source and the fix (a CEL envelope, or brace the reference if the `{var}` dialect was +meant), the same "never swallow a broken predicate" rule ADR-0032 §1c set for the CEL path. +The `try { … } catch { return false }` around that block went with it: nothing in it throws, +so it guarded nothing and would have swallowed the new refusal straight back into the silent +wrong answer. Bare-word comparisons (`'{status} == active'`) and `{var}` templates are +unchanged — only dotted references, which substitution can never leave behind, are refused. diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 9f69b79cb0..59b78f6a68 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -597,6 +597,82 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues).toHaveLength(0); }); + /** + * #4347 — the walk used to stop at a container, so a predicate written in + * the wrong dialect inside a `loop` body passed `objectstack validate` and + * shipped, while the identical predicate one level out was a build error. + */ + describe('structured regions', () => { + const flowWith = (container: Record) => ({ + objects, + flows: [{ + name: 'sweep', + nodes: [{ id: 'start', type: 'start', config: { objectName: 'crm_lead' } }, container], + edges: [], + }], + }); + const badRegion = () => ({ + nodes: [ + { id: 'gate', type: 'decision', config: { condition: '{record.rating} >= 4' } }, + { id: 'act', type: 'update_record' }, + ], + edges: [{ id: 'b1', source: 'gate', target: 'act', condition: '{record.status} == "open"' }], + }); + + it('flags a brace-in-CEL predicate inside a loop body, naming the region', () => { + const issues = validateStackExpressions(flowWith({ + id: 'loop_1', type: 'loop', config: { collection: '{rows}', body: badRegion() }, + })); + // Both the body node's `config.condition` and the body edge's condition. + expect(issues).toHaveLength(2); + for (const issue of issues) expect(issue.where).toContain("loop 'loop_1' body"); + expect(issues.map(i => i.source)).toEqual(['{record.rating} >= 4', '{record.status} == "open"']); + }); + + it('flags them in parallel branches and try_catch regions too', () => { + expect(validateStackExpressions(flowWith({ + id: 'par', type: 'parallel', config: { branches: [badRegion(), badRegion()] }, + }))).toHaveLength(4); + + const tc = validateStackExpressions(flowWith({ + id: 'tc', type: 'try_catch', config: { try: badRegion(), catch: badRegion() }, + })); + expect(tc).toHaveLength(4); + expect(tc.filter(i => i.where.includes("try_catch 'tc' catch"))).toHaveLength(2); + }); + + it('reaches a container nested inside another region', () => { + const issues = validateStackExpressions(flowWith({ + id: 'outer', type: 'loop', + config: { + collection: '{rows}', + body: { + nodes: [{ id: 'inner', type: 'loop', config: { collection: '{cols}', body: badRegion() } }], + edges: [], + }, + }, + })); + expect(issues).toHaveLength(2); + for (const issue of issues) expect(issue.where).toContain("loop 'outer' body → loop 'inner' body"); + }); + + it('leaves a correct region alone', () => { + expect(validateStackExpressions(flowWith({ + id: 'loop_1', type: 'loop', + config: { + collection: '{rows}', + body: { + nodes: [ + { id: 'gate', type: 'decision', config: { condition: 'record.rating >= 4' } }, + { id: 'act', type: 'update_record' }, + ], + edges: [{ id: 'b1', source: 'gate', target: 'act', condition: 'record.status == "open"' }], + }, + }, + }))).toHaveLength(0); + }); + }); + it('tolerates a screen with no fields, a non-array fields, and no config', () => { expect(validateStackExpressions(screenFlow([]))).toHaveLength(0); expect(validateStackExpressions({ diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 5b9d3c0cef..82758483f3 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -18,7 +18,8 @@ */ import { validateExpression } from '@objectstack/formula'; -import { resolveFlowNodeExpressions } from '@objectstack/spec/automation'; +import { collectFlowGraphs, resolveFlowNodeExpressions } from '@objectstack/spec/automation'; +import type { FlowNodeParsed } from '@objectstack/spec/automation'; export interface ExprIssue { where: string; @@ -130,76 +131,85 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { for (const flow of asArray(stack.flows)) { const flowName = typeof flow.name === 'string' ? flow.name : '(unnamed flow)'; const nodes = Array.isArray(flow.nodes) ? (flow.nodes as AnyRec[]) : []; - const edges = Array.isArray(flow.edges) ? (flow.edges as AnyRec[]) : []; // The record-change target object — `record.*` refs resolve against it. const startNode = nodes.find(n => n.type === 'start'); const startCfg = (startNode?.config ?? {}) as AnyRec; const objectName = typeof startCfg.objectName === 'string' ? startCfg.objectName : undefined; - for (const node of nodes) { - const cfg = (node.config ?? {}) as AnyRec; - check(`flow '${flowName}' · node '${node.id}' (${node.type}) condition`, cfg.condition, objectName); + // #4347 — every graph in the flow, not just `flow.nodes`/`flow.edges`. An + // ADR-0031 container keeps a whole sub-graph in its `config`, so the + // top-level walk validated PART of the flow while reporting on all of it: a + // predicate written in the wrong dialect inside a `loop` body passed + // `objectstack validate` and shipped. This is the author-time half of the + // same traversal the engine's registration pass now does; `scope` names the + // region so the located message still points at one edge. + for (const graph of collectFlowGraphs(flow as { nodes?: FlowNodeParsed[] })) { + const at = graph.scope ? `flow '${flowName}' · ${graph.scope}` : `flow '${flowName}'`; + for (const node of graph.nodes as unknown as AnyRec[]) { + const cfg = (node.config ?? {}) as AnyRec; + check(`${at} · 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, - // 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 - // 'flow-node-script-config-aliases' conversion (#3796) canonicalizes it. - const fn = - (typeof cfg.function === 'string' ? cfg.function.trim() : '') || - (typeof cfg.functionName === 'string' ? cfg.functionName.trim() : ''); - 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) { - issues.push({ - where: `flow '${flowName}' · 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 })\`).`, - 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`. - issues.push({ - where: `flow '${flowName}' · 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 })\`.`, - source: JSON.stringify({ id: node.id, type: node.type, config: cfg }), - }); + // 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( + `${at} · 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, + // 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 + // 'flow-node-script-config-aliases' conversion (#3796) canonicalizes it. + const fn = + (typeof cfg.function === 'string' ? cfg.function.trim() : '') || + (typeof cfg.functionName === 'string' ? cfg.functionName.trim() : ''); + 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) { + 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 })\`).`, + 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`. + 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 })\`.`, + source: JSON.stringify({ id: node.id, type: node.type, config: cfg }), + }); + } } } - } - for (const edge of edges) { - check(`flow '${flowName}' · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition, objectName); + for (const edge of graph.edges as unknown as AnyRec[]) { + check(`${at} · edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition, objectName); + } } } diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 9ab2adc742..7d06274724 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -5,7 +5,7 @@ import type { ExecutionLog, ActionDescriptor } from '@objectstack/spec/automatio import type { AutomationContext, AutomationResult, ResumeSignal, IAutomationService, ScreenSpec } from '@objectstack/spec/contracts'; 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 { FlowSchema, FLOW_STRUCTURAL_NODE_TYPES, validateControlFlow, normalizeControlFlowRegions, collectFlowGraphs, findRegionEntry, defineActionDescriptor } from '@objectstack/spec/automation'; import { resolveFlowNodeExpressions } from '@objectstack/spec/automation'; import { applyConversionsToFlow } from '@objectstack/spec'; import type { FlowRegionParsed } from '@objectstack/spec/automation'; @@ -25,6 +25,15 @@ import { ConnectorSchema } from '@objectstack/spec/integration'; // engine at module load in both ESM and CJS builds. import { ExpressionEngine, validateExpression, nearestName } from '@objectstack/formula'; +/** + * A bare **dotted reference** (`record.amount`, `row.shouldRun`, + * `previous.status`) — a CEL path, and a shape `{var}` template substitution can + * never leave behind, since it replaces the whole `{…}` token with a value. + * Each segment must start like an identifier, so numeric literals (`1.5`, + * `500.00`) are not references. See {@link AutomationEngine.refuseUnresolvedCelOperand}. + */ +const UNRESOLVED_CEL_REFERENCE = /^[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)+$/; + /** * The slice of a descriptor's JSON-Schema `configSchema` that the undeclared-key * walk reads (#4045). Structural only — no validation semantics. @@ -1459,15 +1468,25 @@ export class AutomationEngine implements IAutomationService { onNotice: (n) => this.logger.warn(`[flow '${name}'] ${n.code}: ${n.message}`), onConflict: (c) => this.logger.warn(`[flow '${name}'] ${c.code}: ${c.message}`), }); - const parsed = FlowSchema.parse(converted); + const flowShell = FlowSchema.parse(converted); // DAG cycle detection - this.detectCycles(parsed); + this.detectCycles(flowShell); // ADR-0031 — validate structured control-flow constructs (loop bodies, // parallel branches, try/catch regions) are well-formed (single-entry/ // single-exit, acyclic). Reject the malformed before it can run. - validateControlFlow(parsed); + validateControlFlow(flowShell); + + // #4347 — then canonicalize what lives INSIDE those regions. A region + // sits in `FlowNodeSchema.config`, which is an open `z.record`, so the + // parse above stopped at the container: a bare-string `condition` on a + // top-level edge came back as the canonical `{ dialect: 'cel', source }` + // envelope while the identical predicate on a loop-body edge stayed a + // bare string. Same flow, same call, different stored shape by nesting + // depth. Runs after `validateControlFlow` so a malformed region is + // still reported by the validator that owns that message. + const parsed = normalizeControlFlowRegions(flowShell); // ADR-0018 §M1 — validate node types against the live action registry. // The protocol no longer gates `type` with a closed enum; membership is @@ -2939,36 +2958,46 @@ export class AutomationEngine implements IAutomationService { } }; - for (const node of flow.nodes) { - 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, - ); + // #4347 — every graph in the flow, not just the top-level arrays. An + // ADR-0031 container keeps a whole sub-graph in its `config`, so + // iterating `flow.nodes`/`flow.edges` checked PART of the flow while + // reporting on all of it: the `{record.x}` brace-trap this pass exists + // to catch registered in silence one level in, and the flow then failed + // at run time with the loud diagnostic suppressed. `scope` names the + // region so a nested finding says where it is. + for (const graph of collectFlowGraphs(flow)) { + const at = graph.scope ? `${graph.scope}: ` : ''; + for (const node of graph.nodes) { + const cfg = (node.config ?? {}) as Record; + // start-node trigger gate + decision/branch predicates live in config.condition + check(`${at}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( + `${at}node '${node.id}' (${node.type}) ${found.entry.label} at config.${found.path}`, + found.value, + false, + ); + } + } + for (const edge of graph.edges) { + check(`${at}edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition as unknown); } - } - for (const edge of flow.edges) { - check(`edge '${edge.id}' (${edge.source}→${edge.target}) condition`, edge.condition as unknown); } if (failures.length > 0) { @@ -3503,30 +3532,72 @@ export class AutomationEngine implements IAutomationService { } resolved = resolved.trim(); - try { - // Boolean literals - if (resolved === 'true') return true; - if (resolved === 'false') return false; - - // Comparison operators (ordered by length to match longer operators first) - const operators = ['===', '!==', '>=', '<=', '!=', '==', '>', '<'] as const; - for (const op of operators) { - const idx = resolved.indexOf(op); - if (idx !== -1) { - const left = resolved.slice(0, idx).trim(); - const right = resolved.slice(idx + op.length).trim(); - return this.compareValues(left, op, right); - } + // No `try { … } catch { return false }` around this block (#4347). Nothing + // in it throws — `indexOf` / `slice` / `Number` / `compareValues` are all + // total — so the catch guarded nothing, and the one thing that CAN throw + // here now is the deliberate refusal below, which a swallow-to-`false` + // would turn straight back into the silent wrong answer it exists to + // prevent (ADR-0032 §1c, same rule as the CEL path above). + + // Boolean literals + if (resolved === 'true') return true; + if (resolved === 'false') return false; + + // Comparison operators (ordered by length to match longer operators first) + const operators = ['===', '!==', '>=', '<=', '!=', '==', '>', '<'] as const; + for (const op of operators) { + const idx = resolved.indexOf(op); + if (idx !== -1) { + const left = resolved.slice(0, idx).trim(); + const right = resolved.slice(idx + op.length).trim(); + this.refuseUnresolvedCelOperand(exprStr, left, right); + return this.compareValues(left, op, right); } + } - // Numeric truthy check - const numVal = Number(resolved); - if (!isNaN(numVal)) return numVal !== 0; + // Numeric truthy check + const numVal = Number(resolved); + if (!isNaN(numVal)) return numVal !== 0; - return false; - } catch { - return false; - } + return false; + } + + /** + * Refuse a comparison whose operand is an **unresolved CEL reference** + * (#4347). + * + * The legacy path substitutes `{var}` tokens and then compares whatever text + * is left. A dotted path can never be what that substitution produced — it + * replaces the whole `{…}` token with a VALUE — so a surviving + * `record.amount` means a CEL predicate reached the template path. Comparing + * it as a string is not merely unhelpful, it is silently WRONG *in the + * true direction*: + * + * 'oppRecord.amount > 500000' → 'oppRecord.amount' > '500000' + * → 'o' > '5' → TRUE, for every record + * + * — a gate that reports success while never actually gating. So this refuses + * rather than warns, the same rule ADR-0032 §1c set for the CEL path: a + * predicate that cannot be evaluated is a fault, never a quiet `false` (or, + * here, a quiet `true`). + * + * Only *dotted* references are refused. A bare word compares as a string on + * purpose — `'{status} == active'` is the documented legacy spelling, and + * after substitution both sides are plain words. + */ + private refuseUnresolvedCelOperand(source: string, ...operands: readonly string[]): void { + const offender = operands.find(o => UNRESOLVED_CEL_REFERENCE.test(o)); + if (!offender) return; + throw new Error( + `condition evaluation error: '${offender}' is an unresolved expression reference — ` + + `source: \`${source}\`. This predicate reached the legacy \`{var}\` template path, which ` + + `compares leftover text as STRINGS: a dotted reference is then compared character by ` + + `character (\`record.amount > 500000\` compares 'r' against '5' and is true for every ` + + `record), so the branch would be silently wrong rather than merely unevaluated. ` + + `Write it as CEL — a \`{ dialect: 'cel', source }\` envelope or the \`P\` tagged template ` + + `— which resolves references properly; or, if the \`{var}\` template dialect was meant, ` + + `brace the reference (\`{${offender}}\`).`, + ); } /** diff --git a/packages/services/service-automation/src/nested-region-parity.test.ts b/packages/services/service-automation/src/nested-region-parity.test.ts new file mode 100644 index 0000000000..d46cf27d04 --- /dev/null +++ b/packages/services/service-automation/src/nested-region-parity.test.ts @@ -0,0 +1,252 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A node behaves the same inside a structured region as it does outside one + * (#4347). + * + * `registerFlow` canonicalizes a stored flow through three passes — the ADR-0087 + * conversion table, `FlowSchema.parse`, and the ADR-0032 predicate validation — + * and all three walked `flow.nodes` / `flow.edges` only. An ADR-0031 container + * keeps a whole sub-graph in its open `config`, so every one of them stopped at + * the container and metadata came out **position-dependent**: the reporting app + * shipped three sweeps whose gates never opened, with `success: true` on every + * run and nothing in the log. + * + * Each test here is a parity assertion: identical metadata, once at the top + * level and once inside a `loop` body, must come out the same. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from './engine.js'; +import type { NodeExecutor } from './engine.js'; +import { registerLoopNode } from './builtin/loop-node.js'; +import { registerLogicNodes } from './builtin/logic-nodes.js'; + +function silentLogger() { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; +} +function ctx() { + return { logger: silentLogger(), getService() { return undefined; } } as any; +} + +const CONDITION = 'row.shouldRun == true'; +const ENVELOPE = { dialect: 'cel', source: CONDITION }; + +describe('#4347 — a loop-body predicate is canonicalized like a top-level one', () => { + let engine: AutomationEngine; + let opened: string[]; + + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + opened = []; + registerLoopNode(engine, ctx()); + registerLogicNodes(engine, ctx()); + // The node behind the gate: it records that the gate let execution through, + // which is the only way to tell "nothing to do" from "the gate is broken". + engine.registerNodeExecutor({ + type: 'mark', + async execute(node) { opened.push(String(node.id)); return { success: true }; }, + } as NodeExecutor); + }); + + /** The issue's repro: the same predicate on a top-level edge and a body edge. */ + const reproFlow = (condition: unknown) => ({ + name: 'repro', label: 'Repro', type: 'schedule' as const, status: 'active' as const, runAs: 'system' as const, + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { schedule: '0 0 * * *' } }, + { + id: 'loop', type: 'loop', label: 'Loop', + config: { + collection: [{ shouldRun: true }], iteratorVariable: 'row', + body: { + nodes: [ + { id: 'gate', type: 'decision', label: 'Gate' }, + { id: 'inner', type: 'mark', label: 'Inner' }, + ], + edges: [{ id: 'b1', source: 'gate', target: 'inner', type: 'conditional' as const, condition }], + }, + }, + }, + { id: 'outer', type: 'mark', label: 'Outer' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'loop', type: 'default' as const }, + { id: 'e2', source: 'loop', target: 'outer', type: 'conditional' as const, condition }, + ], + }); + + it.each([ + ['a bare string', CONDITION], + ['an explicit CEL envelope', ENVELOPE], + ])('stores %s as the canonical envelope on BOTH edges', (_label, condition) => { + engine.registerFlow('repro', reproFlow(condition)); + const flow = engine.flows.get('repro')!; + + const topEdge = flow.edges.find(e => e.id === 'e2')!.condition; + const bodyEdge = (flow.nodes.find(n => n.id === 'loop')!.config as any).body.edges[0].condition; + + expect(topEdge).toEqual(ENVELOPE); + expect(bodyEdge).toEqual(ENVELOPE); + }); + + it.each([ + ['a bare string', CONDITION], + ['an explicit CEL envelope', ENVELOPE], + ])('opens the loop-body gate when written as %s', async (_label, condition) => { + engine.registerFlow('repro', reproFlow(condition)); + const result = await engine.execute('repro', { params: {}, event: 'schedule' } as never); + + expect(result.success).toBe(true); + // The assertion the reporting app had to add by hand: a run that reports + // success and writes nothing is indistinguishable from one with no work. + expect(opened).toEqual(['inner', 'outer']); + }); + + it('still evaluates a loop-body predicate that is FALSE as false', async () => { + engine.registerFlow('repro', reproFlow('row.shouldRun == false')); + await engine.execute('repro', { params: {}, event: 'schedule' } as never); + expect(opened).toEqual([]); + }); +}); + +describe('#4347 — the conversion table reaches a node inside a region', () => { + it('renames a protocol-11 callout type in a loop body, so the node still has an executor', async () => { + const engine = new AutomationEngine(silentLogger()); + registerLoopNode(engine, ctx()); + const called: string[] = []; + // Only the CANONICAL type is registered — exactly the runtime situation: the + // retired alias has no executor, so an unconverted nested node fails. + engine.registerNodeExecutor({ + type: 'http', + async execute(node) { called.push(String(node.id)); return { success: true }; }, + } as NodeExecutor); + + engine.registerFlow('callout', { + name: 'callout', label: 'Callout', type: 'schedule', status: 'active', runAs: 'system', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { schedule: '0 0 * * *' } }, + { + id: 'loop', type: 'loop', label: 'Loop', + config: { + collection: [1], iteratorVariable: 'row', + body: { nodes: [{ id: 'nested', type: 'webhook', label: 'Nested', config: { url: 'https://x' } }], edges: [] }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'loop', type: 'default' }], + }); + + expect((engine.flows.get('callout')!.nodes[1]!.config as any).body.nodes[0].type).toBe('http'); + const result = await engine.execute('callout', { params: {}, event: 'schedule' } as never); + expect(result.success).toBe(true); + expect(called).toEqual(['nested']); + }); + + it('canonicalizes a nested CRUD alias — an unconverted `filters` leaves no filter at all', () => { + const engine = new AutomationEngine(silentLogger()); + registerLoopNode(engine, ctx()); + engine.registerNodeExecutor({ type: 'delete_record', async execute() { return { success: true }; } } as NodeExecutor); + + engine.registerFlow('purge', { + name: 'purge', label: 'Purge', type: 'schedule', status: 'active', runAs: 'system', + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { schedule: '0 0 * * *' } }, + { + id: 'loop', type: 'loop', label: 'Loop', + config: { + collection: [1], iteratorVariable: 'row', + body: { + nodes: [{ id: 'del', type: 'delete_record', label: 'Del', config: { object: 'lead', filters: { status: 'stale' } } }], + edges: [], + }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'loop', type: 'default' }], + }); + + expect((engine.flows.get('purge')!.nodes[1]!.config as any).body.nodes[0].config) + .toEqual({ objectName: 'lead', filter: { status: 'stale' } }); + }); +}); + +describe('#4347 — predicate validation covers region graphs', () => { + const braceTrapFlow = (placeNested: boolean) => { + const bad = '{record.rating} >= 4'; // #1491 — braces in a CEL predicate + const body = { + nodes: [{ id: 'gate', type: 'decision', label: 'Gate' }, { id: 'inner', type: 'mark', label: 'Inner' }], + edges: [{ + id: 'b1', source: 'gate', target: 'inner', type: 'conditional' as const, + ...(placeNested ? { condition: bad } : {}), + }], + }; + return { + name: 'trap', label: 'Trap', type: 'schedule' as const, status: 'active' as const, runAs: 'system' as const, + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { schedule: '0 0 * * *' } }, + { id: 'loop', type: 'loop', label: 'Loop', config: { collection: [1], iteratorVariable: 'row', body } }, + { id: 'outer', type: 'mark', label: 'Outer' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'loop', type: 'default' as const }, + { + id: 'e2', source: 'loop', target: 'outer', type: 'conditional' as const, + ...(placeNested ? {} : { condition: bad }), + }, + ], + }; + }; + + let engine: AutomationEngine; + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + registerLoopNode(engine, ctx()); + registerLogicNodes(engine, ctx()); + engine.registerNodeExecutor({ type: 'mark', async execute() { return { success: true }; } } as NodeExecutor); + }); + + it('rejects the brace trap at the top level (unchanged)', () => { + expect(() => engine.registerFlow('trap', braceTrapFlow(false))).toThrow(/invalid expression/i); + }); + + it('rejects the SAME brace trap inside a loop body, naming the region', () => { + expect(() => engine.registerFlow('trap', braceTrapFlow(true))).toThrow(/invalid expression/i); + expect(() => engine.registerFlow('trap', braceTrapFlow(true))).toThrow(/loop 'loop' body/); + }); +}); + +describe("#4347 — the legacy `{var}` path refuses an unresolved reference", () => { + let engine: AutomationEngine; + beforeEach(() => { engine = new AutomationEngine(silentLogger()); }); + + it('refuses a dotted reference instead of comparing it lexicographically', () => { + const vars = new Map([['oppRecord', { amount: 10 }]]); + // The reported footgun: 'oppRecord.amount' > '500000' compares 'o' to '5', + // so this used to be TRUE for every record regardless of the amount. + expect(() => engine.evaluateCondition('oppRecord.amount > 500000', vars)).toThrow(/unresolved expression reference/); + expect(() => engine.evaluateCondition('oppRecord.amount > 500000', vars)).toThrow(/dialect: 'cel'/); + }); + + it('refuses it on either side of the operator', () => { + const vars = new Map(); + expect(() => engine.evaluateCondition('5 == row.shouldRun', vars)).toThrow(/unresolved expression reference/); + }); + + it('evaluates the same predicate correctly once it is a CEL envelope', () => { + const vars = new Map([['oppRecord', { amount: 10 }]]); + expect(engine.evaluateCondition({ dialect: 'cel', source: 'oppRecord.amount > 500000' }, vars)).toBe(false); + }); + + it('leaves the documented legacy dialects alone', () => { + const vars = new Map([['amount', 500], ['status', 'active'], ['rate', 1.5]]); + // `{var}` templates: substitution resolves the reference before comparison. + expect(engine.evaluateCondition('{amount} > 100', vars)).toBe(true); + // A decimal literal is not a reference. + expect(engine.evaluateCondition('{rate} > 1.25', vars)).toBe(true); + // Bare words compare as strings on purpose — the documented spelling. + expect(engine.evaluateCondition('{status} == active', vars)).toBe(true); + expect(engine.evaluateCondition('true', vars)).toBe(true); + // A dotted key that DID resolve is a value by the time the compare runs. + expect(engine.evaluateCondition('{a.b} == 7', new Map([['a.b', 7]]))).toBe(true); + }); +}); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index aff1d07f2e..ff08e0f4f0 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -2135,6 +2135,7 @@ "FlowEdge (type)", "FlowEdgeParsed (type)", "FlowEdgeSchema (const)", + "FlowGraph (interface)", "FlowNode (type)", "FlowNodeAction (const)", "FlowNodeExpressionPath (interface)", @@ -2250,6 +2251,7 @@ "analyzeRegion (function)", "approverTypeIsOrgScoped (function)", "canonicalApproverType (function)", + "collectFlowGraphs (function)", "defineActionDescriptor (function)", "defineFlow (function)", "defineWebhook (function)", @@ -2258,6 +2260,7 @@ "flowForm (const)", "getApprovalNodeConfigJsonSchema (function)", "importBpmnToConstructs (function)", + "normalizeControlFlowRegions (function)", "normalizeDecisionOutputs (function)", "resolveFlowNodeExpressions (function)", "validateControlFlow (function)" diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index 9b9194bfaf..bdda745fa5 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -300,6 +300,78 @@ export function findRegionEntry(region: { nodes: FlowNodeParsed[]; edges?: FlowE return analysis.entryId; } +// ─── Where the containers keep their regions ───────────────────────── + +/** A dict — region-shaped enough to reach its `nodes` / `edges`. */ +function isRegionDict(v: unknown): v is Record { + return typeof v === 'object' && v !== null && !Array.isArray(v); +} + +/** One region slot found on a node: its raw value, its `config` key, and a label. */ +interface RegionSlot { + /** The raw value at `config[key]` — region-shaped or not; callers check. */ + readonly raw: unknown; + /** Replace this `config` key to write a normalized region back. */ + readonly key: string; + /** Index within `key`, for the array-valued slot (`parallel.branches`). */ + readonly index?: number; + /** Diagnostic label, e.g. `loop 'sweep' body` / `parallel 'fan' branch 0`. */ + readonly label: string; + /** The schema this slot's value parses as. */ + readonly schema: z.ZodTypeAny; +} + +/** + * The region slots one node carries — the single place that knows where each + * ADR-0031 container keeps its nested graph(s). Three passes read it + * ({@link validateControlFlow}, {@link normalizeControlFlowRegions}, + * {@link collectFlowGraphs}), which is exactly why it is one function: a fourth + * container construct is added here once, not in three walks that drift. + * + * Emits a slot for a declared key even when its value is not region-shaped — + * `validateControlFlow` needs to reject that, not skip it. + */ +function regionSlotsOf(node: FlowNodeParsed): RegionSlot[] { + const cfg = node.config as Record | undefined; + if (!cfg) return []; + + if (node.type === LOOP_NODE_TYPE) { + return cfg.body == null + ? [] + : [{ raw: cfg.body, key: 'body', label: `loop '${node.id}' body`, schema: FlowRegionSchema }]; + } + if (node.type === PARALLEL_NODE_TYPE && Array.isArray(cfg.branches)) { + return cfg.branches.map((raw, index) => ({ + raw, + key: 'branches', + index, + label: `parallel '${node.id}' branch ${index}`, + // A branch also carries an optional `name`, which the plain region schema + // (a non-strict `z.object`) would strip. + schema: ParallelBranchSchema, + })); + } + if (node.type === TRY_CATCH_NODE_TYPE) { + const slots: RegionSlot[] = []; + if (cfg.try != null) { + slots.push({ raw: cfg.try, key: 'try', label: `try_catch '${node.id}' try`, schema: FlowRegionSchema }); + } + if (cfg.catch != null) { + slots.push({ raw: cfg.catch, key: 'catch', label: `try_catch '${node.id}' catch`, schema: FlowRegionSchema }); + } + return slots; + } + return []; +} + +/** + * Depth ceiling for the recursive region walks below. Regions nest (a `loop` + * inside a `try_catch` inside a `loop`) but not deeply, and a flow arriving as + * hand-built objects rather than parsed JSON could carry a self-reference — + * which would otherwise be an unbounded recursion at the load seam. + */ +const MAX_REGION_DEPTH = 32; + /** * Validate every structured control-flow construct in a flow, throwing on the * first malformed region (ADR-0031 — "reject the malformed before run"). Covers @@ -310,31 +382,158 @@ export function findRegionEntry(region: { nodes: FlowNodeParsed[]; edges?: FlowE * Intended to be called from `registerFlow()` after DAG cycle detection. */ export function validateControlFlow(flow: { nodes: FlowNodeParsed[] }): void { - const assertRegion = (raw: unknown, where: string): void => { - const parsed = FlowRegionSchema.safeParse(raw); - if (!parsed.success) { - throw new Error(`${where}: invalid region — ${parsed.error.issues.map(i => i.message).join('; ')}`); - } - const analysis = analyzeRegion(parsed.data); - if (analysis.errors.length > 0) { - throw new Error(`${where}: ${analysis.errors.join('; ')}`); - } - }; - for (const node of flow.nodes) { const cfg = node.config as Record | undefined; if (!cfg) continue; - - if (node.type === LOOP_NODE_TYPE && cfg.body != null) { - assertRegion(cfg.body, `loop '${node.id}' body`); - } else if (node.type === PARALLEL_NODE_TYPE && Array.isArray(cfg.branches)) { - if (cfg.branches.length < 2) { - throw new Error(`parallel '${node.id}': a parallel block needs at least 2 branches`); + if (node.type === PARALLEL_NODE_TYPE && Array.isArray(cfg.branches) && cfg.branches.length < 2) { + throw new Error(`parallel '${node.id}': a parallel block needs at least 2 branches`); + } + for (const slot of regionSlotsOf(node)) { + const parsed = slot.schema.safeParse(slot.raw); + if (!parsed.success) { + throw new Error( + `${slot.label}: invalid region — ${parsed.error.issues.map(i => i.message).join('; ')}`, + ); + } + // Both region schemas produce `{ nodes, edges }` (a parallel branch adds + // `name` — a superset); `z.ZodTypeAny` just cannot say so. + const analysis = analyzeRegion(parsed.data as { nodes: FlowNodeParsed[]; edges?: FlowEdgeParsed[] }); + if (analysis.errors.length > 0) { + throw new Error(`${slot.label}: ${analysis.errors.join('; ')}`); } - cfg.branches.forEach((branch, i) => assertRegion(branch, `parallel '${node.id}' branch ${i}`)); - } else if (node.type === TRY_CATCH_NODE_TYPE) { - if (cfg.try != null) assertRegion(cfg.try, `try_catch '${node.id}' try`); - if (cfg.catch != null) assertRegion(cfg.catch, `try_catch '${node.id}' catch`); } } } + +// ─── Region normalization ──────────────────────────────────────────── + +/** + * Parse ONE region value through its own schema, then recurse into the + * containers its nodes carry. + * + * A value that does not parse is returned untouched: rejecting a malformed + * region is {@link validateControlFlow}'s job (and, at run time, the container + * executor's `parseNodeConfig`). A normalization pass that also threw would + * change *which* flows register, which is not what it is for. + */ +function normalizeRegion(slot: RegionSlot, depth: number): unknown { + if (!isRegionDict(slot.raw)) return slot.raw; + const parsed = slot.schema.safeParse(slot.raw); + if (!parsed.success) return slot.raw; + const region = parsed.data as { nodes?: FlowNodeParsed[] }; + if (!Array.isArray(region.nodes)) return region; + return { ...region, nodes: region.nodes.map(n => normalizeNodeRegions(n, depth + 1)) }; +} + +/** Normalize every region one node carries — recursively, since regions nest. */ +function normalizeNodeRegions(node: FlowNodeParsed, depth: number): FlowNodeParsed { + if (depth >= MAX_REGION_DEPTH) return node; + const cfg = node.config as Record | undefined; + if (!cfg) return node; + + let next = cfg; + for (const slot of regionSlotsOf(node)) { + const normalized = normalizeRegion(slot, depth); + if (normalized === slot.raw) continue; + if (slot.index === undefined) { + next = { ...next, [slot.key]: normalized }; + } else { + const branches = [...(next[slot.key] as unknown[])]; + branches[slot.index] = normalized; + next = { ...next, [slot.key]: branches }; + } + } + + return next === cfg ? node : { ...node, config: next }; +} + +/** + * Canonicalize the metadata **inside** every structured region of a flow (#4347). + * + * `FlowSchema.parse` normalizes a flow's own `nodes[]` / `edges[]` — most + * visibly, `FlowEdgeSchema.condition` is `ExpressionInputSchema`, so a + * bare-string predicate becomes the canonical `{ dialect: 'cel', source }` + * envelope. It does not reach a region, because a region lives inside + * `FlowNodeSchema.config`, which is deliberately `z.record(z.unknown())` — open, + * per node type. So the *same predicate* was stored enveloped on a top-level edge + * and left a bare string on a loop-body edge: a representation that depended on + * where in the graph it sat, which no flow author can be expected to predict. + * + * This pass closes that. Each region is run through its own schema — recursively, + * because regions nest — producing a flow whose nested edges and nodes carry the + * same canonical shapes as its top-level ones. Copy-on-write: a flow with no + * structured container comes back untouched. + * + * Call it at the load seam, after `FlowSchema.parse` and `validateControlFlow`. + * The container executors parse their own config at run time (`parseNodeConfig`, + * #4277), so this is not what makes a nested predicate *evaluate* correctly — it + * is what makes the stored flow SAY so, for every reader that is not the + * executor: the Studio designer, `getFlow`, the version history, and any + * consumer that reads a region without re-parsing it. + */ +export function normalizeControlFlowRegions(flow: T): T { + if (!Array.isArray(flow.nodes)) return flow; + let changed = false; + const nodes = flow.nodes.map(node => { + const next = normalizeNodeRegions(node, 0); + if (next !== node) changed = true; + return next; + }); + return changed ? { ...flow, nodes } : flow; +} + +// ─── Whole-flow graph traversal ────────────────────────────────────── + +/** One executable graph within a flow: the flow's own, or a nested region's. */ +export interface FlowGraph { + /** + * Where this graph sits. Empty string for the flow's own `nodes`/`edges`; + * otherwise the region path, e.g. `loop 'sweep' body` or + * `loop 'sweep' body → try_catch 'guard' catch`. + */ + readonly scope: string; + readonly nodes: readonly FlowNodeParsed[]; + readonly edges: readonly FlowEdgeParsed[]; +} + +/** + * Every graph in a flow — its own, plus each nested structured region, depth + * first (#4347). + * + * A flow's nodes and edges are not all in `flow.nodes` / `flow.edges`: an + * ADR-0031 container keeps a whole sub-graph in its `config`. A validator that + * iterates only the top-level arrays therefore checks *part* of the flow while + * reporting on all of it — which is how a `{record.x}` brace-trap inside a loop + * body passed registration while the identical predicate one level out was a + * hard error. Iterate this instead of `flow.nodes` wherever a pass means "every + * node in this flow", and use {@link FlowGraph.scope} to say where a finding is. + */ +export function collectFlowGraphs( + flow: { nodes?: readonly FlowNodeParsed[]; edges?: readonly FlowEdgeParsed[] }, +): FlowGraph[] { + const graphs: FlowGraph[] = []; + + const visit = ( + nodes: readonly FlowNodeParsed[], + edges: readonly FlowEdgeParsed[], + scope: string, + depth: number, + ): void => { + graphs.push({ scope, nodes, edges }); + if (depth >= MAX_REGION_DEPTH) return; + for (const node of nodes) { + for (const slot of regionSlotsOf(node)) { + if (!isRegionDict(slot.raw) || !Array.isArray(slot.raw.nodes)) continue; + visit( + slot.raw.nodes as FlowNodeParsed[], + Array.isArray(slot.raw.edges) ? (slot.raw.edges as FlowEdgeParsed[]) : [], + scope ? `${scope} → ${slot.label}` : slot.label, + depth + 1, + ); + } + } + }; + + visit(flow.nodes ?? [], flow.edges ?? [], '', 0); + return graphs; +} diff --git a/packages/spec/src/automation/region-normalization.test.ts b/packages/spec/src/automation/region-normalization.test.ts new file mode 100644 index 0000000000..d60260536a --- /dev/null +++ b/packages/spec/src/automation/region-normalization.test.ts @@ -0,0 +1,181 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Region metadata is canonicalized and reachable — `normalizeControlFlowRegions` + * and `collectFlowGraphs` (#4347). + * + * `FlowSchema.parse` normalizes a flow's own `nodes[]`/`edges[]` but stops at a + * container, because an ADR-0031 region lives inside `FlowNodeSchema.config` — + * an open `z.record`. The result was position-dependent metadata: the same + * predicate stored as a `{ dialect: 'cel', source }` envelope on a top-level + * edge and as a bare string one level in, and every pass that iterated + * `flow.nodes` checking "the whole flow" silently checked only part of it. + */ + +import { describe, expect, it } from 'vitest'; + +import { FlowSchema } from './flow.zod.js'; +import { + LOOP_NODE_TYPE, + PARALLEL_NODE_TYPE, + TRY_CATCH_NODE_TYPE, + collectFlowGraphs, + normalizeControlFlowRegions, +} from './control-flow.zod.js'; + +const CONDITION = 'row.shouldRun == true'; +const ENVELOPE = { dialect: 'cel', source: CONDITION }; + +const gate = { id: 'gate', type: 'decision', label: 'Gate' }; +const write = { id: 'write', type: 'create_record', label: 'Write' }; +/** A well-formed region whose single edge carries a BARE STRING condition. */ +const gatedRegion = () => ({ + nodes: [structuredClone(gate), structuredClone(write)], + edges: [{ id: 'b1', source: 'gate', target: 'write', type: 'conditional', condition: CONDITION }], +}); + +const flowWith = (containerNode: Record) => FlowSchema.parse({ + name: 'repro', label: 'Repro', type: 'schedule', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + containerNode, + { id: 'end', type: 'end', label: 'End' }, + ], + edges: [ + { id: 'e1', source: 'start', target: containerNode.id as string, type: 'default' }, + // The SAME predicate at the top level, for the parity assertion. + { id: 'e2', source: containerNode.id as string, target: 'end', type: 'conditional', condition: CONDITION }, + ], +}); + +const loopWith = (body: unknown) => ({ + id: 'loop', type: LOOP_NODE_TYPE, label: 'Loop', config: { collection: '{rows}', iteratorVariable: 'row', body }, +}); + +describe('#4347 — normalizeControlFlowRegions', () => { + it('envelopes a loop-body edge condition, matching what the top-level edge already got', () => { + const parsed = flowWith(loopWith(gatedRegion())); + // Baseline: FlowSchema.parse enveloped the top-level edge and NOT the nested one. + expect(parsed.edges.find(e => e.id === 'e2')!.condition).toEqual(ENVELOPE); + expect((parsed.nodes[1]!.config as any).body.edges[0].condition).toBe(CONDITION); + + const flow = normalizeControlFlowRegions(parsed); + expect((flow.nodes[1]!.config as any).body.edges[0].condition).toEqual(ENVELOPE); + // Parity: the same predicate now has the same representation either side of + // the container boundary. + expect((flow.nodes[1]!.config as any).body.edges[0].condition) + .toEqual(flow.edges.find(e => e.id === 'e2')!.condition); + }); + + it('normalizes every parallel branch and keeps the branch `name`', () => { + const flow = normalizeControlFlowRegions(flowWith({ + id: 'par', type: PARALLEL_NODE_TYPE, label: 'Fan', + config: { branches: [{ name: 'left', ...gatedRegion() }, { name: 'right', ...gatedRegion() }] }, + })); + + const branches = (flow.nodes[1]!.config as any).branches; + for (const branch of branches) expect(branch.edges[0].condition).toEqual(ENVELOPE); + // `FlowRegionSchema` would have stripped `name`; branches parse as branches. + expect(branches.map((b: { name: string }) => b.name)).toEqual(['left', 'right']); + }); + + it('normalizes both try_catch regions', () => { + const flow = normalizeControlFlowRegions(flowWith({ + id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', + config: { try: gatedRegion(), catch: gatedRegion(), errorVariable: '$err' }, + })); + + const cfg = (flow.nodes[1]!.config as any); + expect(cfg.try.edges[0].condition).toEqual(ENVELOPE); + expect(cfg.catch.edges[0].condition).toEqual(ENVELOPE); + // Sibling config keys are untouched — only the region slots are rewritten. + expect(cfg.errorVariable).toBe('$err'); + }); + + it('recurses — a condition three containers deep is enveloped too', () => { + const flow = normalizeControlFlowRegions(flowWith(loopWith({ + nodes: [{ + id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', + config: { try: { nodes: [loopWith(gatedRegion())], edges: [] } }, + }], + edges: [], + }))); + + const deep = (flow.nodes[1]!.config as any) + .body.nodes[0].config.try + .nodes[0].config.body; + expect(deep.edges[0].condition).toEqual(ENVELOPE); + }); + + it('is idempotent and copy-on-write', () => { + const once = normalizeControlFlowRegions(flowWith(loopWith(gatedRegion()))); + expect(normalizeControlFlowRegions(once)).toEqual(once); + + // No structured container → the exact same reference comes back. + const plain = FlowSchema.parse({ + name: 'plain', label: 'Plain', type: 'schedule', + nodes: [{ id: 'start', type: 'start', label: 'Start' }, { id: 'end', type: 'end', label: 'End' }], + edges: [{ id: 'e1', source: 'start', target: 'end' }], + }); + expect(normalizeControlFlowRegions(plain)).toBe(plain); + }); + + it('leaves a region it cannot parse untouched — rejecting is validateControlFlow\'s job', () => { + // `outputSchema` is a tombstoned key: the region will not parse. Normalizing + // must not throw here, or it would change WHICH flows register. + const raw = { nodes: [{ ...gate, outputSchema: {} }], edges: [] }; + const flow = normalizeControlFlowRegions(flowWith(loopWith(raw))); + expect((flow.nodes[1]!.config as any).body).toBe(raw); + }); + + it('ignores a legacy flat-graph loop (no body)', () => { + const parsed = flowWith({ id: 'loop', type: LOOP_NODE_TYPE, label: 'Loop', config: { collection: '{rows}' } }); + expect(normalizeControlFlowRegions(parsed)).toBe(parsed); + }); +}); + +describe('#4347 — collectFlowGraphs', () => { + it('yields the flow graph plus every region, each scoped', () => { + const flow = normalizeControlFlowRegions(flowWith(loopWith(gatedRegion()))); + const graphs = collectFlowGraphs(flow); + + expect(graphs.map(g => g.scope)).toEqual(['', "loop 'loop' body"]); + expect(graphs[0]!.nodes.map(n => n.id)).toEqual(['start', 'loop', 'end']); + expect(graphs[1]!.nodes.map(n => n.id)).toEqual(['gate', 'write']); + expect(graphs[1]!.edges.map(e => e.id)).toEqual(['b1']); + }); + + it('names each parallel branch and both try_catch regions', () => { + expect(collectFlowGraphs(normalizeControlFlowRegions(flowWith({ + id: 'par', type: PARALLEL_NODE_TYPE, label: 'Fan', + config: { branches: [gatedRegion(), gatedRegion()] }, + }))).map(g => g.scope)).toEqual(['', "parallel 'par' branch 0", "parallel 'par' branch 1"]); + + expect(collectFlowGraphs(normalizeControlFlowRegions(flowWith({ + id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', + config: { try: gatedRegion(), catch: gatedRegion() }, + }))).map(g => g.scope)).toEqual(['', "try_catch 'tc' try", "try_catch 'tc' catch"]); + }); + + it('chains the scope of a nested region so a finding says where it is', () => { + const flow = normalizeControlFlowRegions(flowWith(loopWith({ + nodes: [{ + id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', + config: { catch: gatedRegion() }, + }], + edges: [], + }))); + expect(collectFlowGraphs(flow).map(g => g.scope)) + .toEqual(['', "loop 'loop' body", "loop 'loop' body → try_catch 'tc' catch"]); + }); + + it('terminates on a self-referential region instead of recursing forever', () => { + // Hand-built flows are objects, not parsed JSON, so a cycle is reachable. + const selfRegion: { nodes: unknown[]; edges: unknown[] } = { nodes: [], edges: [] }; + selfRegion.nodes.push({ id: 'l', type: LOOP_NODE_TYPE, label: 'L', config: { collection: '{r}', body: selfRegion } }); + const graphs = collectFlowGraphs({ nodes: selfRegion.nodes as never, edges: [] }); + // It descended (so the walk is real) and it stopped (so the ceiling holds). + expect(graphs.length).toBeGreaterThan(1); + expect(graphs.length).toBeLessThan(64); + }); +}); diff --git a/packages/spec/src/conversions/region-walk.test.ts b/packages/spec/src/conversions/region-walk.test.ts new file mode 100644 index 0000000000..4fd3242d30 --- /dev/null +++ b/packages/spec/src/conversions/region-walk.test.ts @@ -0,0 +1,252 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The conversion pass reaches inside ADR-0031 structured regions (#4347). + * + * Before this, `mapFlowNodes` walked `flows[].nodes[]` and stopped at a + * container: a `loop` body / `parallel` branch / `try_catch` block is a whole + * sub-graph living in the container's open `config`, so every conversion in the + * table skipped it. Metadata therefore behaved differently by nesting depth — + * the assertion these tests make over and over is the *parity* one: whatever the + * table does to a node at the top level, it must do to the identical node one + * (or three) levels in. + */ + +import { describe, expect, it } from 'vitest'; + +import { + LOOP_NODE_TYPE, + PARALLEL_NODE_TYPE, + TRY_CATCH_NODE_TYPE, + LoopConfigSchema, + ParallelConfigSchema, + TryCatchConfigSchema, +} from '../automation/control-flow.zod.js'; +import { applyConversions, applyConversionsToFlow, collectConversionNotices } from './apply.js'; +import { FLOW_REGION_SLOTS } from './walk.js'; + +/** A protocol-11 callout alias — renamed to `http` by the table. */ +const webhookNode = (id: string) => ({ id, type: 'webhook', label: id, config: { url: 'https://x' } }); +/** The converted form of {@link webhookNode}. */ +const httpNode = (id: string) => ({ id, type: 'http', label: id, config: { url: 'https://x' } }); + +const region = (...nodes: unknown[]) => ({ nodes, edges: [] }); + +describe('#4347 — conversions reach nodes inside structured regions', () => { + it('converts a node in a `loop` body exactly as it converts its top-level twin', () => { + const { stack, notices } = collectConversionNotices({ + flows: [{ + name: 'sweep', + nodes: [ + webhookNode('top'), + { id: 'loop', type: 'loop', label: 'Loop', config: { collection: '{rows}', body: region(webhookNode('inner')) } }, + ], + }], + }); + + const flow = (stack.flows as any[])[0]; + expect(flow.nodes[0]).toEqual(httpNode('top')); + expect(flow.nodes[1].config.body.nodes[0]).toEqual(httpNode('inner')); + // One notice each — the nested one carries the region in its path, so the + // warning the load seam logs points at the node the author has to edit. + expect(notices.map(n => n.path)).toEqual([ + 'flows[0].nodes[0].type', + 'flows[0].nodes[1].config.body.nodes[0].type', + ]); + }); + + it('converts nodes in every `parallel` branch', () => { + const { stack, notices } = collectConversionNotices({ + flows: [{ + name: 'fan', + nodes: [{ + id: 'par', type: 'parallel', label: 'Fan', + config: { branches: [region(webhookNode('a')), region(webhookNode('b'))] }, + }], + }], + }); + + const branches = (stack.flows as any[])[0].nodes[0].config.branches; + expect(branches[0].nodes[0]).toEqual(httpNode('a')); + expect(branches[1].nodes[0]).toEqual(httpNode('b')); + expect(notices.map(n => n.path)).toEqual([ + 'flows[0].nodes[0].config.branches[0].nodes[0].type', + 'flows[0].nodes[0].config.branches[1].nodes[0].type', + ]); + }); + + it('converts nodes in both `try_catch` regions', () => { + const { stack, notices } = collectConversionNotices({ + flows: [{ + name: 'guarded', + nodes: [{ + id: 'tc', type: 'try_catch', label: 'Guard', + config: { try: region(webhookNode('t')), catch: region(webhookNode('c')) }, + }], + }], + }); + + const cfg = (stack.flows as any[])[0].nodes[0].config; + expect(cfg.try.nodes[0]).toEqual(httpNode('t')); + expect(cfg.catch.nodes[0]).toEqual(httpNode('c')); + expect(notices).toHaveLength(2); + }); + + it('recurses through nested containers (loop → try_catch → loop)', () => { + const { stack, notices } = collectConversionNotices({ + flows: [{ + name: 'deep', + nodes: [{ + id: 'l1', type: 'loop', label: 'Outer', + config: { + collection: '{a}', + body: region({ + id: 'tc', type: 'try_catch', label: 'Guard', + config: { + try: region({ + id: 'l2', type: 'loop', label: 'Inner', + config: { collection: '{b}', body: region(webhookNode('deep')) }, + }), + }, + }), + }, + }], + }], + }); + + const deep = (stack.flows as any[])[0] + .nodes[0].config.body + .nodes[0].config.try + .nodes[0].config.body + .nodes[0]; + expect(deep).toEqual(httpNode('deep')); + expect(notices[0]!.path).toBe( + 'flows[0].nodes[0].config.body.nodes[0].config.try.nodes[0].config.body.nodes[0].type', + ); + }); + + it('applies a config-key conversion to a nested node — the erased-condition hazard', () => { + // `filters` → `filter` is the highest-stakes entry in the table: the + // `delete_record` executor reads the canonical key, so leaving the alias + // unconverted leaves it with NO filter at all. It has to reach a loop body. + const { stack } = collectConversionNotices({ + flows: [{ + name: 'purge', + nodes: [{ + id: 'loop', type: 'loop', label: 'Loop', + config: { + collection: '{rows}', + body: region({ + id: 'del', type: 'delete_record', label: 'Del', + config: { object: 'lead', filters: { status: 'stale' } }, + }), + }, + }], + }], + }); + + expect((stack.flows as any[])[0].nodes[0].config.body.nodes[0].config) + .toEqual({ objectName: 'lead', filter: { status: 'stale' } }); + }); + + it('reaches a region through `applyConversionsToFlow` — the runtime rehydration seam', () => { + const flow = { + name: 'sweep', + nodes: [{ id: 'loop', type: 'loop', label: 'Loop', config: { collection: '{rows}', body: region(webhookNode('inner')) } }], + }; + const converted = applyConversionsToFlow(flow, { includeRetired: true }); + expect((converted as any).nodes[0].config.body.nodes[0]).toEqual(httpNode('inner')); + }); +}); + +describe('#4347 — the region walk stays copy-on-write and shape-gated', () => { + it('returns the identical reference when nothing inside a region converts', () => { + const clean = { + flows: [{ + name: 'clean', + nodes: [{ id: 'loop', type: 'loop', label: 'Loop', config: { collection: '{rows}', body: region(httpNode('inner')) } }], + }], + }; + expect(applyConversions(clean)).toBe(clean); + }); + + it('shares untouched branches — only the changed path is copied', () => { + const untouchedBranch = region(httpNode('already-canonical')); + const stack = { + flows: [{ + name: 'fan', + nodes: [{ + id: 'par', type: 'parallel', label: 'Fan', + config: { branches: [region(webhookNode('stale')), untouchedBranch] }, + }], + }], + }; + const out = applyConversions(stack) as any; + expect(out).not.toBe(stack); + expect(out.flows[0].nodes[0].config.branches[1]).toBe(untouchedBranch); + }); + + it('leaves a non-region `config.body` alone — an http payload is not a sub-graph', () => { + // `body` is an ordinary config key elsewhere. The walk is gated on the + // container node type AND on the region shape, so neither an `http` node's + // request payload nor a `loop` carrying a non-region `body` is descended. + const stack = { + flows: [{ + name: 'post', + nodes: [ + { id: 'h', type: 'http', label: 'Post', config: { url: 'https://x', body: { nodes: 'not-an-array' } } }, + { id: 'l', type: 'loop', label: 'Loop', config: { collection: '{r}', body: 'PT1M' } }, + ], + }], + }; + expect(applyConversions(stack)).toBe(stack); + }); + + it('terminates on a self-referential region instead of recursing forever', () => { + // A stack handed to `defineStack` is hand-built objects, not parsed JSON, + // so a cycle is reachable on the load path. + const selfRegion: { nodes: unknown[]; edges: unknown[] } = { nodes: [], edges: [] }; + selfRegion.nodes.push({ id: 'l', type: 'loop', label: 'L', config: { collection: '{r}', body: selfRegion } }); + expect(() => applyConversions({ flows: [{ name: 'cyclic', nodes: selfRegion.nodes }] })).not.toThrow(); + }); + + it('cannot be steered onto Object.prototype by a hostile node type', () => { + const stack = { + flows: [{ + name: 'evil', + nodes: [{ id: 'x', type: 'constructor', label: 'X', config: { body: region(webhookNode('inner')) } }], + }], + }; + // `constructor` is not a container: the region is left alone, and the lookup + // must not resolve through the prototype chain and throw. + expect(applyConversions(stack)).toBe(stack); + }); +}); + +/** + * The slot table in `walk.ts` is declared locally so the conversion walker stays + * free of schema imports. This is what keeps that copy honest: a fourth ADR-0031 + * container, or a renamed region key, fails here instead of silently going + * unwalked — which is exactly the shape of the #4347 defect. + */ +describe('#4347 — FLOW_REGION_SLOTS reconciles with the ADR-0031 constructs', () => { + it('covers every canonical container type id', () => { + expect([...FLOW_REGION_SLOTS.keys()].sort()) + .toEqual([LOOP_NODE_TYPE, PARALLEL_NODE_TYPE, TRY_CATCH_NODE_TYPE].sort()); + }); + + it('names region keys the container config schemas actually declare', () => { + const declared = (schema: { shape: Record }) => Object.keys(schema.shape); + const slotKeys = (type: string) => FLOW_REGION_SLOTS.get(type)!.map(s => s.key); + + expect(declared(LoopConfigSchema as never)).toEqual(expect.arrayContaining(slotKeys(LOOP_NODE_TYPE))); + expect(declared(ParallelConfigSchema as never)).toEqual(expect.arrayContaining(slotKeys(PARALLEL_NODE_TYPE))); + expect(declared(TryCatchConfigSchema as never)).toEqual(expect.arrayContaining(slotKeys(TRY_CATCH_NODE_TYPE))); + }); + + it('marks exactly the array-valued slot as `many`', () => { + const many = [...FLOW_REGION_SLOTS].flatMap(([type, slots]) => + slots.filter(s => s.many).map(s => `${type}.${s.key}`)); + expect(many).toEqual(['parallel.branches']); + }); +}); diff --git a/packages/spec/src/conversions/walk.ts b/packages/spec/src/conversions/walk.ts index 307cd60e64..3a8bc9d186 100644 --- a/packages/spec/src/conversions/walk.ts +++ b/packages/spec/src/conversions/walk.ts @@ -20,12 +20,127 @@ function isDict(v: unknown): v is Dict { } /** - * Immutably map every flow node in `stack.flows[].nodes[]`. + * Where each ADR-0031 structured container carries its nested region(s) — the + * map that makes {@link mapFlowNodes} reach a node inside a `loop` body, a + * `parallel` branch, or a `try_catch` block (#4347). * - * `mapper` receives each node dict and its path (`flows[i].nodes[j]`) and - * returns either the same reference (no change) or a new dict. The stack, the - * `flows` array, an individual flow, and its `nodes` array are each copied only - * when a descendant actually changed. + * A region is a self-contained mini-flow (`{ nodes, edges }`) living in the + * container's OPEN `config` record, so a pass that walked only + * `flows[].nodes[]` saw the container and stopped there. The consequence was + * **position-dependent metadata**: the same node converted at the top level and + * did not one level in — a `webhook` callout inside a loop body kept its + * protocol-11 type (no executor owns that id, so the run fails), and a + * `delete_record` kept `config.filters`, leaving the canonical `filter` the + * executor reads absent — the erased-condition hazard + * `flow-node-crud-filter-alias` exists to prevent. Same flow, same + * `registerFlow` call, different outcome by nesting depth. + * + * Declared here rather than imported from `automation/control-flow.zod.ts` to + * keep this module free of schema imports (it is a pure shape walker). The ids + * and keys are pinned to that module's `LOOP_NODE_TYPE` / `PARALLEL_NODE_TYPE` / + * `TRY_CATCH_NODE_TYPE` and to the `LoopConfigSchema` / `ParallelConfigSchema` / + * `TryCatchConfigSchema` shapes by a reconciliation test (`region-walk.test.ts`), + * so a new construct cannot be added there and silently go unwalked here. + * + * A `Map` rather than an object literal so a node whose `type` is + * `'constructor'` / `'__proto__'` cannot reach `Object`'s prototype chain. + */ +export const FLOW_REGION_SLOTS: ReadonlyMap< + string, + ReadonlyArray<{ readonly key: string; readonly many: boolean }> +> = new Map([ + ['loop', [{ key: 'body', many: false }]], + ['parallel', [{ key: 'branches', many: true }]], + ['try_catch', [{ key: 'try', many: false }, { key: 'catch', many: false }]], +]); + +/** + * Depth ceiling for the region recursion. Containers nest, but not deeply, and + * a stack handed to `defineStack` is hand-built objects rather than parsed JSON + * — so a self-referencing region is reachable, and would otherwise be an + * unbounded recursion on the load path. Mirrors the ceiling the region walks in + * `automation/control-flow.zod.ts` use. + */ +const MAX_REGION_DEPTH = 32; + +/** + * Immutably map every node of one structured region (`{ nodes, edges }`). + * + * A value that is not region-shaped passes through untouched: the `config` keys + * above are only *conventionally* regions (`config` is an open record, and + * `body` in particular is also an ordinary key elsewhere — an `http` node's + * request payload), so the shape is checked, never assumed. + */ +function mapRegionNodes( + region: unknown, + path: string, + mapper: (node: Dict, path: string) => Dict, + depth: number, +): unknown { + if (!isDict(region) || !Array.isArray(region.nodes)) return region; + let changed = false; + const nextNodes = region.nodes.map((node, i) => { + if (!isDict(node)) return node; + const mapped = mapNodeTree(node, `${path}.nodes[${i}]`, mapper, depth); + if (mapped !== node) changed = true; + return mapped; + }); + return changed ? { ...region, nodes: nextNodes } : region; +} + +/** + * Map one flow node **and everything nested under it**: the node itself, then + * the nodes of every region its `config` carries, recursively — regions nest (a + * `loop` inside a `try_catch` inside a `loop`). + * + * The mapper runs on the container FIRST and the region lookup keys off the + * *mapped* node's `type`, so a conversion that renames a container type still + * has its body walked, under the canonical id. + */ +function mapNodeTree( + node: Dict, + path: string, + mapper: (node: Dict, path: string) => Dict, + depth: number, +): Dict { + const mapped = mapper(node, path); + if (depth >= MAX_REGION_DEPTH) return mapped; + const slots = typeof mapped.type === 'string' ? FLOW_REGION_SLOTS.get(mapped.type) : undefined; + if (!slots) return mapped; + const config = mapped.config; + if (!isDict(config)) return mapped; + + let nextConfig = config; + for (const { key, many } of slots) { + const raw = nextConfig[key]; + if (many) { + if (!Array.isArray(raw)) continue; + let branchesChanged = false; + const nextBranches = raw.map((branch, i) => { + const next = mapRegionNodes(branch, `${path}.config.${key}[${i}]`, mapper, depth + 1); + if (next !== branch) branchesChanged = true; + return next; + }); + if (branchesChanged) nextConfig = { ...nextConfig, [key]: nextBranches }; + } else { + const next = mapRegionNodes(raw, `${path}.config.${key}`, mapper, depth + 1); + if (next !== raw) nextConfig = { ...nextConfig, [key]: next }; + } + } + + return nextConfig === config ? mapped : { ...mapped, config: nextConfig }; +} + +/** + * Immutably map every flow node in `stack.flows[].nodes[]` — **including the + * nodes nested inside ADR-0031 structured regions** (`loop.config.body`, + * `parallel.config.branches[]`, `try_catch.config.try`/`.catch`), to any depth. + * + * `mapper` receives each node dict and its path (`flows[i].nodes[j]`, or + * `flows[i].nodes[j].config.body.nodes[k]` for a nested one) and returns either + * the same reference (no change) or a new dict. The stack, the `flows` array, an + * individual flow, its `nodes` array, and every container `config` on the way + * down are each copied only when a descendant actually changed. */ export function mapFlowNodes( stack: Dict, @@ -40,7 +155,7 @@ export function mapFlowNodes( let nodesChanged = false; const nextNodes = flow.nodes.map((node, ni) => { if (!isDict(node)) return node; - const mapped = mapper(node, `flows[${fi}].nodes[${ni}]`); + const mapped = mapNodeTree(node, `flows[${fi}].nodes[${ni}]`, mapper, 0); if (mapped !== node) nodesChanged = true; return mapped; });