From 470a0d2b7005223320424f262f19dfbca60ef24a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 11:15:17 +0000 Subject: [PATCH] fix(spec,service-automation): registerFlow's remaining validators cover structured regions (#4389) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4347 closed the conversion and predicate halves of "metadata behaves differently depending on how deep it sits". Three validators were left walking `flow.nodes` only, so the same class stayed open one layer over: an ADR-0031 container keeps a whole sub-graph in its open `config`, and each of these checked part of the flow while reporting on all of it. - `validateControlFlow` recurses. A container nested inside another container's region was never validated at registration — it reached run time, where `runRegion` → `findRegionEntry` throws mid-iteration, after the enclosing loop has begun and its side effects have landed. Cannot break a working flow: everything newly rejected was already guaranteed to throw on execution. Also closes cycle detection over nested regions, since region bodies are cycle-checked by `analyzeRegion` here rather than by `detectCycles`. - `validateNodeTypes` covers region nodes (soft-fail). - `validateNodeConfigKeys` covers region nodes (hard-fail), with the region in each violation. No double-reporting from the container side: all three container descriptors declare their region slot as a bare `nodes: { type: 'array' }` with no `items`. Measured before extending the two hard-fail checks, since widening a rejecting validator is a behaviour change rather than a bugfix: registering every flow in app-showcase, app-crm and app-todo through the real `registerFlow` and re-running each validator's own code over all 9 region graphs produced 0 new findings. So they land at existing severity rather than staged through a warning window. `validateNodeInputSchemas` is deliberately NOT extended — 0 uses across all 159 example flow nodes, and it compares a config value's runtime type against the declared one, so extending it would newly fail a region node carrying a `{var}` template string in a `number`-typed slot. Noted on #4389 instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HSBbKMdDgHjGpQdrvQUQXj --- .changeset/region-validator-coverage.md | 43 ++++++ .../services/service-automation/src/engine.ts | 33 +++-- .../src/nested-region-parity.test.ts | 131 ++++++++++++++++++ .../spec/src/automation/control-flow.zod.ts | 59 +++++--- 4 files changed, 237 insertions(+), 29 deletions(-) create mode 100644 .changeset/region-validator-coverage.md diff --git a/.changeset/region-validator-coverage.md b/.changeset/region-validator-coverage.md new file mode 100644 index 0000000000..ed07fbd116 --- /dev/null +++ b/.changeset/region-validator-coverage.md @@ -0,0 +1,43 @@ +--- +'@objectstack/spec': patch +'@objectstack/service-automation': patch +--- + +`registerFlow`'s remaining validators cover structured regions (#4389). + +#4347 closed the conversion and predicate halves of "metadata behaves differently +depending on how deep it sits". Three validators were left walking `flow.nodes` only, so +the same class stayed open one layer over: an ADR-0031 container keeps a whole sub-graph +in its open `config`, and each of these checked *part* of the flow while reporting on all +of it. + +- **`validateControlFlow` recurses.** A container nested inside another container's region + was never validated at registration — it reached run time, where `runRegion` → + `findRegionEntry` throws mid-iteration, after the enclosing loop has begun and its side + effects have landed. This cannot break a working flow: everything newly rejected was + already guaranteed to throw on execution. It also closes cycle detection over nested + regions, since region bodies are cycle-checked by `analyzeRegion` here rather than by + `detectCycles`. +- **`validateNodeTypes` covers region nodes.** Soft-fail. A node in a `loop` body is as + executable as one beside it, so the warning that exists to predict `NO_EXECUTOR` went + quiet on exactly the nodes whose run-time failure is hardest to place. +- **`validateNodeConfigKeys` covers region nodes.** Hard-fail. `visibleIf` is the typo + #4277 exists to catch, and moving the node into a region restored the silence #4277 + closed. Violations carry the region (`loop 'sweep' body · node 'w' …`). No + double-reporting from the container side: all three container descriptors declare their + region slot as a bare `nodes: { type: 'array' }` with no `items`, so the schema-lockstep + walk stops there instead of descending twice. + +**Measured before extending the two hard-fail checks**, since widening a rejecting +validator is a behaviour change rather than a bugfix: registering every flow in +`app-showcase`, `app-crm` and `app-todo` through the real `registerFlow` and re-running +each validator's own code over all 9 region graphs produced **0 new findings**. Nothing +that registers today stops registering, so the checks land at their existing severity +rather than staged through a warning window. + +`validateNodeInputSchemas` is deliberately **not** extended. It declares 0 uses across all +159 example flow nodes, and its check compares a config value's runtime type against the +declared one — so extending it would newly fail a region node carrying a `{var}` template +string in a `number`-typed slot, which is a live authoring shape. Widening a check with a +known false-positive mode and no demonstrated reader is not worth it; the traversal gap is +noted on #4389 instead. diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 5f76879809..512157f5e4 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -2923,6 +2923,11 @@ export class AutomationEngine implements IAutomationService { * warned about (not rejected) so flows authored against a temporarily * absent plugin still register; the runtime surfaces a hard NO_EXECUTOR * error if such a node is actually executed. + * + * Covers nodes inside ADR-0031 regions (#4389). A node in a `loop` body is + * as executable as one beside it, so leaving regions out meant the warning + * that exists to predict NO_EXECUTOR went quiet on exactly the nodes whose + * failure is hardest to place at run time. */ private validateNodeTypes(flowName: string, flow: FlowParsed): void { const known = new Set([ @@ -2931,7 +2936,9 @@ export class AutomationEngine implements IAutomationService { ...this.actionDescriptors.keys(), ]); const unknown = [...new Set( - flow.nodes.map(n => n.type).filter(t => !known.has(t)), + collectFlowGraphs(flow) + .flatMap(g => g.nodes.map(n => n.type)) + .filter(t => !known.has(t)), )]; if (unknown.length > 0) { this.logger.warn( @@ -2995,13 +3002,23 @@ export class AutomationEngine implements IAutomationService { */ private validateNodeConfigKeys(flowName: string, flow: FlowParsed): void { const violations: string[] = []; - for (const node of flow.nodes) { - // `assignment` config keys are the author's variable names — see the - // exemption note above. - if (node.type === 'assignment') continue; - const schema = this.actionDescriptors.get(node.type)?.configSchema as ConfigSchemaNode | undefined; - if (!schema) continue; - this.collectUndeclaredConfigKeys(node, schema, node.config, 'config', violations); + // #4389 — every graph, not just the flow's own. `visibleIf` is the typo + // this check exists to catch, and moving the node into a `loop` body + // used to restore the silence #4277 closed. No double-reporting from the + // container side: all three container descriptors declare their region + // slot as a bare `nodes: { type: 'array' }` with no `items`, so the + // schema-lockstep walk stops there rather than descending twice. + for (const graph of collectFlowGraphs(flow)) { + const scoped: string[] = []; + for (const node of graph.nodes) { + // `assignment` config keys are the author's variable names — see the + // exemption note above. + if (node.type === 'assignment') continue; + const schema = this.actionDescriptors.get(node.type)?.configSchema as ConfigSchemaNode | undefined; + if (!schema) continue; + this.collectUndeclaredConfigKeys(node, schema, node.config, 'config', scoped); + } + for (const v of scoped) violations.push(graph.scope ? `${graph.scope} · ${v}` : v); } if (violations.length > 0) { throw new Error( diff --git a/packages/services/service-automation/src/nested-region-parity.test.ts b/packages/services/service-automation/src/nested-region-parity.test.ts index d46cf27d04..0b0976e71b 100644 --- a/packages/services/service-automation/src/nested-region-parity.test.ts +++ b/packages/services/service-automation/src/nested-region-parity.test.ts @@ -21,10 +21,21 @@ 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'; +import { registerCrudNodes } from './builtin/crud-nodes.js'; function silentLogger() { return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } } as any; } + +/** A logger that records `warn` lines, for the soft-fail validators. */ +function recordingLogger(sink: string[]) { + const l: any = { + info() {}, error() {}, debug() {}, + warn(msg: string) { sink.push(String(msg)); }, + child() { return l; }, + }; + return l; +} function ctx() { return { logger: silentLogger(), getService() { return undefined; } } as any; } @@ -215,6 +226,126 @@ describe('#4347 — predicate validation covers region graphs', () => { }); }); +/** + * #4389 — the three registration validators #4347 left walking `flow.nodes` + * only. Same parity shape: identical bad metadata at the top level and one + * level in must get the identical verdict. + * + * Measured before extending them: 0 new findings across app-showcase / app-crm + * / app-todo (9 region graphs), so nothing that registers today stops. + */ +describe('#4389 — registration validators cover region graphs', () => { + /** Wrap `nodes` in a loop body when `nested`, else splice them in flat. */ + const flowWith = (nested: boolean, bodyNodes: Array>) => ({ + name: 'sweep', label: 'Sweep', type: 'schedule' as const, status: 'active' as const, runAs: 'system' as const, + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: { schedule: '0 0 * * *' } }, + ...(nested + ? [{ + id: 'loop', type: 'loop', label: 'Loop', + config: { + collection: [1], iteratorVariable: 'row', + body: { nodes: bodyNodes, edges: [] }, + }, + }] + : bodyNodes), + ], + edges: [{ id: 'e1', source: 'start', target: nested ? 'loop' : String(bodyNodes[0]!.id), type: 'default' as const }], + }); + + describe('validateNodeConfigKeys (hard-fail)', () => { + let engine: AutomationEngine; + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + registerLoopNode(engine, ctx()); + registerCrudNodes(engine, ctx()); + }); + + // `fieldz` is not on the `create_record` descriptor — the #4277 typo class. + const typoNode = [{ id: 'w', type: 'create_record', label: 'W', config: { objectName: 'lead', fieldz: { a: 1 } } }]; + + it('rejects an undeclared config key at the top level (unchanged)', () => { + expect(() => engine.registerFlow('sweep', flowWith(false, typoNode))).toThrow(/undeclared config key/); + }); + + it('rejects the SAME key inside a loop body, naming the region', () => { + expect(() => engine.registerFlow('sweep', flowWith(true, typoNode))).toThrow(/undeclared config key/); + expect(() => engine.registerFlow('sweep', flowWith(true, typoNode))).toThrow(/loop 'loop' body · node 'w'/); + }); + + it('reports each nested key ONCE — the container config physically contains it', () => { + try { + engine.registerFlow('sweep', flowWith(true, typoNode)); + throw new Error('expected a rejection'); + } catch (err) { + const msg = (err as Error).message; + expect(msg).toMatch(/1 undeclared config key/); + expect(msg.match(/`fieldz`/g)).toHaveLength(1); + } + }); + + it('leaves a correct region alone', () => { + const ok = [{ id: 'w', type: 'create_record', label: 'W', config: { objectName: 'lead', fields: { a: 1 } } }]; + expect(() => engine.registerFlow('sweep', flowWith(true, ok))).not.toThrow(); + }); + }); + + describe('validateNodeTypes (soft-fail)', () => { + const unknownNode = [{ id: 'x', type: 'no_such_node_type', label: 'X' }]; + + const warningsFor = (nested: boolean) => { + const warnings: string[] = []; + const engine = new AutomationEngine(recordingLogger(warnings)); + registerLoopNode(engine, ctx()); + engine.registerFlow('sweep', flowWith(nested, unknownNode)); + return warnings.filter(w => w.includes('no registered executor')); + }; + + it('warns about an unknown node type at the top level (unchanged)', () => { + expect(warningsFor(false)).toHaveLength(1); + }); + + it('warns about the SAME type inside a loop body', () => { + const warnings = warningsFor(true); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('no_such_node_type'); + }); + }); + + describe('validateControlFlow (hard-fail)', () => { + let engine: AutomationEngine; + beforeEach(() => { + engine = new AutomationEngine(silentLogger()); + registerLoopNode(engine, ctx()); + engine.registerNodeExecutor({ type: 'mark', async execute() { return { success: true }; } } as NodeExecutor); + }); + + /** A two-entry (malformed) region — `analyzeRegion`'s single-entry rule. */ + const malformed = { nodes: [{ id: 'a', type: 'mark', label: 'A' }, { id: 'b', type: 'mark', label: 'B' }], edges: [] }; + const innerLoop = { id: 'inner', type: 'loop', label: 'Inner', config: { collection: [1], body: malformed } }; + + it('rejects a malformed region on a top-level container (unchanged)', () => { + expect(() => engine.registerFlow('sweep', flowWith(false, [innerLoop]))).toThrow(/single-entry/); + }); + + it('rejects a malformed region on a NESTED container, naming both levels', () => { + // Previously this registered clean and threw mid-run from `findRegionEntry` + // — after the outer loop had begun iterating. + expect(() => engine.registerFlow('sweep', flowWith(true, [innerLoop]))).toThrow(/single-entry/); + expect(() => engine.registerFlow('sweep', flowWith(true, [innerLoop]))) + .toThrow(/loop 'loop' body → loop 'inner' body/); + }); + + it('leaves a well-formed nested container alone', () => { + const wellFormed = { + id: 'inner', type: 'loop', label: 'Inner', + config: { collection: [1], body: { nodes: [{ id: 'a', type: 'mark', label: 'A' }], edges: [] } }, + }; + expect(() => engine.registerFlow('sweep', flowWith(true, [wellFormed]))).not.toThrow(); + }); + }); +}); + describe("#4347 — the legacy `{var}` path refuses an unresolved reference", () => { let engine: AutomationEngine; beforeEach(() => { engine = new AutomationEngine(silentLogger()); }); diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index bdda745fa5..ca720ce5cd 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -375,36 +375,53 @@ 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 - * `loop` bodies, `parallel` branches, and `try_catch` try/catch regions. Only - * validates the *nested structure* when present, so legacy flat-graph `loop` - * nodes (no `config.body`) are untouched — the constructs are additive. + * `loop` bodies, `parallel` branches, and `try_catch` try/catch regions — + * **at every depth** (#4389), so a container nested inside another container's + * region is checked too. Only validates the *nested structure* when present, so + * legacy flat-graph `loop` nodes (no `config.body`) are untouched — the + * constructs are additive. * - * Intended to be called from `registerFlow()` after DAG cycle detection. + * The recursion is not extra strictness looking for work: a malformed nested + * region already failed, just later and worse. `runRegion` calls + * `findRegionEntry`, which throws mid-run — after the enclosing container has + * begun iterating and its side effects have landed. Refusing it at + * `registerFlow` is what ADR-0031's "reject the malformed before it can run" + * asks for, and it cannot break a flow that works today: everything newly + * rejected here was already guaranteed to throw on execution. + * + * Intended to be called from `registerFlow()` after DAG cycle detection. Region + * bodies are cycle-checked here rather than by `detectCycles` — `analyzeRegion` + * carries its own DAG pass — so this recursion is also what closes cycle + * detection over nested regions. */ export function validateControlFlow(flow: { nodes: FlowNodeParsed[] }): void { - for (const node of flow.nodes) { - const cfg = node.config as Record | undefined; - if (!cfg) continue; - 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('; ')}`, - ); + for (const graph of collectFlowGraphs(flow)) { + for (const node of graph.nodes) { + const cfg = node.config as Record | undefined; + if (!cfg) continue; + 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`); } - // 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('; ')}`); + for (const slot of regionSlotsOf(node)) { + const where = graph.scope ? `${graph.scope} → ${slot.label}` : slot.label; + const parsed = slot.schema.safeParse(slot.raw); + if (!parsed.success) { + throw new Error( + `${where}: 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(`${where}: ${analysis.errors.join('; ')}`); + } } } } } + // ─── Region normalization ──────────────────────────────────────────── /**