fix(spec,service-automation): registerFlow's remaining validators cover structured regions (#4389)

.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.

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<string>([
@@ -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(

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<Record<string, unknown>>) => ({
+    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()); });

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<string, unknown> | 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<string, unknown> | 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 ────────────────────────────────────────────
 
 /**