fix(spec,service-automation,lint): flow metadata is canonicalized inside structured regions (#4347) (#4381)

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

- `mapFlowNodes` recurses into `loop.config.body`, `parallel.config.branches[]`
  and `try_catch.config.try`/`.catch`, to any depth. Notice paths carry the
  region. Matters most for the conversions that change behaviour rather than
  spelling: a `webhook` callout inside a loop body kept a type no executor owns,
  and a `delete_record` kept `config.filters`, leaving the canonical `filter`
  the executor reads absent.
- New `normalizeControlFlowRegions`, called at the load seam after
  `validateControlFlow`: each region parses through its own schema, recursively,
  so nested edges and nodes carry the same canonical shapes as top-level ones. A
  region that does not parse is left untouched, so which flows register is
  unchanged.
- New `collectFlowGraphs` yields a flow's own graph plus every nested region with
  a scope label. Both predicate validators iterate it — the engine's
  `validateFlowExpressions` and lint's `validateStackExpressions` — so the
  `{record.x}` brace-trap they exist to catch is caught inside a region too.
- `evaluateCondition`'s legacy `{var}` path refuses an unresolved dotted
  reference instead of comparing it as a string: `'oppRecord.amount > 500000'`
  compared `'o'` against `'5'` and was constantly true regardless of the amount.
  The `try/catch { return false }` around that block goes with it — nothing in it
  throws, so it guarded nothing and would have swallowed the refusal.


Claude-Session: https://claude.ai/code/session_01HSBbKMdDgHjGpQdrvQUQXj

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

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

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

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);
+      }
     }
   }