test(spec): make the remaining un-runnable flow.test.ts fixture shapes runnable, and pin them (#5500) (#5686)

Follows #4924 / PR #5502 on the same file. Four of the five classes #5500
listed were real and are fixed; the first class's stated mechanism did not
survive verification and is reported instead of forced.

- assignment (`assign_output`): `{ variable, value }` -> `{ assignments: { … } }`.
  logic-nodes.ts normalizes three shapes and its last branch makes top-level
  config keys the variable NAMES, so the old config declared two variables
  literally named `variable` and `value` while `contactId` (declared
  `isOutput: true`) was never written. Measured end to end before the fix.

- loop (`loop_records`): legacy flat-graph loop -> ADR-0031 structured
  container. With no `config.body` loop-node.ts reads `config.collection` as a
  bare VARIABLE NAME, so `'{get_old_records.records}'` matched nothing, bound
  nothing and fell through; the `loop -> delete -> loop` back-edge was ordinary
  traversal and `{item.id}` referenced a variable no one set. The delete node
  moves into `config.body` and `iteratorVariable` binds `item`.

- get_record (`get_old_records`): string `filter` -> record form. The contract
  declares `z.record(z.string(), z.unknown())`, so the string failed safeParse
  outright, and `DAYS_AGO()` is implemented nowhere. The date window is spelled
  `{90_days_ago}` — a spec date macro whose slot ownership is declared: a known
  filter token passes through interpolateFilter verbatim for the query engine's
  resolveFilterTokens to expand, and date-macros.zod.ts names "flow node
  filters" as a consumer. `limit` added because it selects the `find`/`records`
  branch the loop needs.

- `object` -> `objectName` at the three remaining sites (ADR-0087 D2
  `flow-node-crud-object-alias`).

- edge conditions: `{…}` template braces -> bare CEL (ADR-0032 §1a). Verified
  that registerFlow rejects the braced form outright, so those fixtures could
  not register at all.

Class 1 ("the `{<node id>.<field>}` output dialect is never bound") is NOT
implemented, because it is false: engine.ts writes every node's `result.output`
under `<nodeId>.<key>` and template.ts resolves that flat key, which
crud-nodes.ts documents as the default binding. `{create_contact.id}` resolves
and is kept verbatim. The real defect at the other cited site was narrower —
get_record only emits `records` on the `limit > 1` branch — and is fixed above.

Each fix carries a contract assertion, and each was reverse-verified by
restoring the broken shape. That caught a phantom in the first draft of the
brace pin: ExpressionInputSchema normalizes a bare-string predicate into a
`{ dialect, source }` envelope, so asserting `toContain` on the parsed
`condition` object passed regardless of the predicate. The pins now read
`condition.source`.

Refs #5500

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

Author: os-zhuang

packages/spec/src/automation/flow.test.ts

@@ -18,9 +18,16 @@ import {
 // builtin's config could run. These are the contracts that do.
 import {
   GetRecordConfigSchema,
+  CreateRecordConfigSchema,
   UpdateRecordConfigSchema,
   DeleteRecordConfigSchema,
 } from './builtin-node-config.zod';
+// #5500 — the `loop` container contract (ADR-0031). A `loop` with no
+// `config.body` is the LEGACY flat-graph shape: `loop-node.ts` reads
+// `config.collection` as a bare VARIABLE NAME, binds `$loopItems`/`$loopIndex`
+// and falls through without iterating — so no `iteratorVariable` is ever set
+// and a `{item.…}` token downstream references nothing.
+import { LoopConfigSchema } from './control-flow.zod';
 
 describe('FlowNodeAction', () => {
   it('should accept all node action types', () => {
@@ -114,8 +121,12 @@ describe('FlowNodeSchema', () => {
       id: 'node_3',
       type: 'create_record',
       label: 'Create Account',
+      // #5500 — was `object:`, the retired spelling the ADR-0087 D2 conversion
+      // `flow-node-crud-object-alias` rewrites at load (live through this major,
+      // retires at 18). A fixture is a teaching surface, so it spells the
+      // canonical key the executor actually reads.
       config: {
-        object: 'account',
+        objectName: 'account',
         fields: {
           name: '{input.companyName}',
           status: 'active',
@@ -124,6 +135,13 @@ describe('FlowNodeSchema', () => {
     };
 
     expect(() => FlowNodeSchema.parse(node)).not.toThrow();
+
+    // #5500 — `FlowNodeSchema.config` is deliberately open (ADR-0018), so the
+    // parse above stays green on a config no executor could run. Pin the config
+    // against the contract `create_record` actually parses: this is a VALUE
+    // verdict (the executor refuses the node without `objectName`), so full
+    // safeParse green is the right bar, and the alias spelling turns it red.
+    expect(CreateRecordConfigSchema.safeParse(node.config).success).toBe(true);
   });
 
   it('should accept all node types', () => {
@@ -481,8 +499,11 @@ describe('FlowSchema', () => {
             id: 'create_contact',
             type: 'create_record',
             label: 'Create Contact',
+            // #5500 — `object:` → `objectName:` (see `should accept node with
+            // config`). The `{firstName}` &c. tokens are declared INPUT
+            // variables, which the engine binds by name, so they resolve.
             config: {
-              object: 'contact',
+              objectName: 'contact',
               fields: {
                 first_name: '{firstName}',
                 last_name: '{lastName}',
@@ -494,9 +515,21 @@ describe('FlowSchema', () => {
             id: 'assign_output',
             type: 'assignment',
             label: 'Set Output',
+            // #5500 — was `{ variable: 'contactId', value: '{create_contact.id}' }`,
+            // which set NEITHER. `logic-nodes.ts` normalizes three assignment
+            // shapes and its last branch is "no `assignments` wrapper → the
+            // top-level config keys ARE the variable names", so that config
+            // declared two variables literally named `variable` and `value`,
+            // and `contactId` — declared `isOutput: true` right above — was
+            // never written. Measured: with the old shape the run ends with
+            // `variable='contactId'`, `value='<the new id>'`, `contactId=undefined`.
+            //
+            // The VALUE token was fine and is kept verbatim: the engine binds
+            // every node's `result.output` under `<nodeId>.<key>` and the
+            // template resolver reads that flat key, so `{create_contact.id}`
+            // resolves to the created row's id (`create_record` outputs `id`).
             config: {
-              variable: 'contactId',
-              value: '{create_contact.id}',
+              assignments: { contactId: '{create_contact.id}' },
             },
           },
           { id: 'end', type: 'end', label: 'End' },
@@ -509,6 +542,23 @@ describe('FlowSchema', () => {
       };
 
       expect(() => FlowSchema.parse(screenFlow)).not.toThrow();
+
+      // #5500 — pin the create node against the contract its executor parses.
+      const cfgOf = (id: string) => screenFlow.nodes.find(n => n.id === id)?.config;
+      expect(CreateRecordConfigSchema.safeParse(cfgOf('create_contact')).success).toBe(true);
+
+      // #5500 — pin the assignment's SHAPE, which no spec schema governs (the
+      // executor reads `config` directly, so `FlowSchema.parse` can never catch
+      // this). The writes must live under `assignments`, and every name written
+      // must be a variable this flow declares — reverting to the bare
+      // `{ variable, value }` shape leaves `assignments` undefined and turns
+      // both assertions red.
+      const assignCfg = cfgOf('assign_output') as { assignments?: Record<string, unknown> };
+      expect(Object.keys(assignCfg?.assignments ?? {})).toEqual(['contactId']);
+      const declared = new Set((screenFlow.variables ?? []).map(v => v.name));
+      for (const written of Object.keys(assignCfg?.assignments ?? {})) {
+        expect(declared.has(written)).toBe(true);
+      }
     });
 
     it('should accept scheduled flow', () => {
@@ -523,55 +573,120 @@ describe('FlowSchema', () => {
             id: 'get_old_records',
             type: 'get_record',
             label: 'Find Old Records',
+            // #5500 — three defects in two keys:
+            //  • `object:` → `objectName:` (the ADR-0087 D2 alias, as above).
+            //  • `filter` was the STRING `'created_at < DAYS_AGO(90)'`. The
+            //    contract declares `z.record(z.string(), z.unknown())`, so a
+            //    string fails safeParse outright ("expected record, received
+            //    string"), and `DAYS_AGO()` is a function no layer implements.
+            //    The date window is spelled in the dialect that OWNS a filter
+            //    value position: `{90_days_ago}` is a spec date macro
+            //    (`DATE_MACRO_PARAM_RE`), and `interpolateFilter` hands a known
+            //    filter token through VERBATIM for the query engine's
+            //    `resolveFilterTokens` to expand (#3810 ownership transfer) —
+            //    `date-macros.zod.ts` names "flow node filters" as a consumer.
+            //  • `limit` was absent. The executor branches on it: `limit > 1`
+            //    runs `find` and outputs a `records` LIST; otherwise `findOne`
+            //    and a single `record`. A cleanup sweep wants the list, and the
+            //    loop below needs an array, so the limit is what makes the
+            //    downstream `collection` an array at all.
             config: {
-              object: 'log_entry',
-              filter: 'created_at < DAYS_AGO(90)',
+              objectName: 'log_entry',
+              filter: { created_at: { $lt: '{90_days_ago}' } },
+              limit: 200,
+              outputVariable: 'oldRecords',
             },
           },
           {
             id: 'loop_records',
             type: 'loop',
             label: 'For Each Record',
+            // #5500 — was a LEGACY flat-graph loop: no `config.body`, so
+            // `loop-node.ts` took its back-compat branch, which reads
+            // `config.collection` as a bare VARIABLE NAME (not a template),
+            // found no variable literally named `{get_old_records.records}`,
+            // bound nothing and returned success. The `loop → delete → loop`
+            // back-edge was ordinary graph traversal, and `{item.id}` in the
+            // delete node below referenced a variable no one ever set.
+            // Measured on the old shape: `$loopItems` unset, `item` undefined.
+            //
+            // This is now the ADR-0031 structured container: the per-item steps
+            // live in `config.body` (a single-entry/single-exit region run in
+            // the enclosing scope) and `iteratorVariable` is what binds `item`.
             config: {
-              collection: '{get_old_records.records}',
-            },
-          },
-          {
-            id: 'delete_record',
-            type: 'delete_record',
-            label: 'Delete Record',
-            // #4924 — this was the worst of the three shapes: `recordId` was the
-            // node's ONLY key, no executor reads it, and a `delete_record` whose
-            // single "constraint" is unread is a match-everything delete (#3810)
-            // wearing a key that reads like a constraint. The executor locates rows
-            // through `filter` and refuses the node without `objectName`.
-            // The per-item token is the loop's `iteratorVariable` (default `item`),
-            // NOT `{<node id>.item}` — node outputs are never bound under a node id.
-            config: {
-              objectName: 'log_entry',
-              filter: { id: '{item.id}' },
+              collection: '{oldRecords}',
+              iteratorVariable: 'item',
+              maxIterations: 200,
+              body: {
+                nodes: [
+                  {
+                    id: 'delete_record',
+                    type: 'delete_record',
+                    label: 'Delete Record',
+                    // #4924 — this was the worst of the three shapes: `recordId` was the
+                    // node's ONLY key, no executor reads it, and a `delete_record` whose
+                    // single "constraint" is unread is a match-everything delete (#3810)
+                    // wearing a key that reads like a constraint. The executor locates rows
+                    // through `filter` and refuses the node without `objectName`.
+                    // The per-item token is the loop's `iteratorVariable` (default `item`),
+                    // NOT `{<node id>.item}` — and #5500 moved the node INSIDE the loop
+                    // body, which is what makes `item` actually bound per iteration.
+                    config: {
+                      objectName: 'log_entry',
+                      filter: { id: '{item.id}' },
+                    },
+                  },
+                ],
+                edges: [],
+              },
             },
           },
           { id: 'end', type: 'end', label: 'End' },
         ],
         edges: [
           { id: 'e1', source: 'start', target: 'get_old_records' },
           { id: 'e2', source: 'get_old_records', target: 'loop_records' },
-          { id: 'e3', source: 'loop_records', target: 'delete_record' },
-          { id: 'e4', source: 'delete_record', target: 'loop_records' },
-          { id: 'e5', source: 'loop_records', target: 'end', label: 'Done' },
+          // #5500 — the `loop → delete → loop` back-edge pair is gone: the
+          // delete node now lives in `config.body`, so the loop's ordinary
+          // out-edge is simply the after-loop continuation (ADR-0031).
+          { id: 'e3', source: 'loop_records', target: 'end', label: 'Done' },
         ],
         runAs: 'system',
       };
 
       expect(() => FlowSchema.parse(scheduledFlow)).not.toThrow();
 
       // #4924 — the delete node addresses rows the only way the executor does.
-      // (The `get_old_records` / `loop_records` pair upstream still carries
-      // shapes of its own — a string `filter`, the `object` alias and a
-      // `{<node id>.…}` output reference — tracked separately, see the PR.)
-      const deleteConfig = scheduledFlow.nodes.find(n => n.id === 'delete_record')?.config;
+      // #5500 — it is now reached through the loop's body region.
+      const loopConfig = scheduledFlow.nodes.find(n => n.id === 'loop_records')?.config;
+      const parsedLoop = LoopConfigSchema.safeParse(loopConfig);
+      expect(parsedLoop.success).toBe(true);
+      const deleteConfig = parsedLoop.success
+        ? parsedLoop.data.body?.nodes.find(n => n.id === 'delete_record')?.config
+        : undefined;
       expect(DeleteRecordConfigSchema.safeParse(deleteConfig).success).toBe(true);
+
+      // #5500 — pin the loop as a STRUCTURED container. `body` is what separates
+      // it from the legacy flat-graph shape that iterated nothing, and
+      // `iteratorVariable` is the only thing that binds the `{item.…}` token the
+      // body's filter reads. Dropping `body` puts the fixture back on the
+      // legacy branch and turns both of these red.
+      expect(parsedLoop.success && parsedLoop.data.body).toBeDefined();
+      expect(parsedLoop.success && parsedLoop.data.iteratorVariable).toBe('item');
+      // …and no main-graph edge targets a body node any more.
+      const bodyNodeIds = new Set(
+        parsedLoop.success ? (parsedLoop.data.body?.nodes ?? []).map(n => n.id) : [],
+      );
+      expect(scheduledFlow.edges.filter(e => bodyNodeIds.has(e.target))).toHaveLength(0);
+
+      // #5500 — the upstream read must produce an ARRAY for the loop to iterate:
+      // `limit > 1` is what selects the `find`/`records` branch over
+      // `findOne`/`record`, so it is a contract detail, not a tuning knob.
+      const getConfig = scheduledFlow.nodes.find(n => n.id === 'get_old_records')?.config;
+      const parsedGet = GetRecordConfigSchema.safeParse(getConfig);
+      expect(parsedGet.success).toBe(true);
+      expect(parsedGet.success && (parsedGet.data.limit ?? 0) > 1).toBe(true);
+      expect(parsedGet.success && parsedGet.data.outputVariable).toBe('oldRecords');
     });
 
     it('should accept API flow with webhook', () => {
@@ -975,11 +1090,29 @@ describe('BPMN — Default Sequence Flow (isDefault)', () => {
       source: 'decision_1',
       target: 'branch_a',
       type: 'conditional',
-      condition: '{amount} > 1000',
+      // #5500 — was `'{amount} > 1000'`. An edge condition is BARE CEL
+      // (ADR-0032 §1a); `{…}` template braces parse as a CEL map literal, and
+      // `AutomationEngine.registerFlow` parse-validates every predicate at
+      // registration, so the braced form is a HARD registration failure:
+      // "Flow '…' has 1 invalid expression (ADR-0032 §1a). Predicates … must
+      // not wrap references in `{…}` template braces". This is the #1491 trap.
+      condition: 'amount > 1000',
       label: 'High Value',
     });
     expect(result.type).toBe('conditional');
     expect(result.isDefault).toBe(false);
+    // #5500 — only an explicit assertion keeps this fixture from teaching the
+    // braced form back in. Braces are correct in TEMPLATE slots
+    // (`loop.collection`) and wrong here — the distinction is the whole point.
+    //
+    // Read `.source`, NOT the condition itself: `ExpressionInputSchema`
+    // normalizes a bare-string predicate into the canonical
+    // `{ dialect: 'cel', source }` envelope, so `expect(result.condition)
+    // .not.toContain('{')` asserts against an OBJECT and passes no matter what
+    // the predicate says. That phantom was written here first and caught by
+    // reverse-verification (the braced spelling stayed green) — hence this note.
+    expect(result.condition?.dialect).toBe('cel');
+    expect(result.condition?.source).not.toContain('{');
   });
 
   it('should validate a decision with default and conditional branches', () => {
@@ -997,8 +1130,11 @@ describe('BPMN — Default Sequence Flow (isDefault)', () => {
       ],
       edges: [
         { id: 'e1', source: 'start', target: 'check_priority' },
-        { id: 'e2', source: 'check_priority', target: 'high_path', type: 'conditional', condition: '{priority} == "high"' },
-        { id: 'e3', source: 'check_priority', target: 'medium_path', type: 'conditional', condition: '{priority} == "medium"' },
+        // #5500 — bare CEL, not `{…}` template braces (ADR-0032 §1a; see
+        // 'should accept conditional edge type'). Registering the braced form
+        // threw at `registerFlow`, so this whole fixture was un-runnable.
+        { id: 'e2', source: 'check_priority', target: 'high_path', type: 'conditional', condition: 'priority == "high"' },
+        { id: 'e3', source: 'check_priority', target: 'medium_path', type: 'conditional', condition: 'priority == "medium"' },
         { id: 'e4', source: 'check_priority', target: 'default_path', isDefault: true, label: 'Default' },
         { id: 'e5', source: 'high_path', target: 'end' },
         { id: 'e6', source: 'medium_path', target: 'end' },
@@ -1012,6 +1148,17 @@ describe('BPMN — Default Sequence Flow (isDefault)', () => {
       const defaultEdge = result.data.edges.find(e => e.isDefault);
       expect(defaultEdge).toBeDefined();
       expect(defaultEdge!.target).toBe('default_path');
+      // #5500 — every guarded branch is bare CEL. `FlowSchema.parse` accepts any
+      // string here, so without this the fixture could teach the #1491 braced
+      // form back in while staying green. Assert on the normalized
+      // `condition.source` (see 'should accept conditional edge type') — the
+      // parsed `condition` is an Expression ENVELOPE, and `toContain` against
+      // the envelope object pins nothing.
+      const guardedEdges = result.data.edges.filter(e => e.condition);
+      expect(guardedEdges).toHaveLength(2);
+      for (const guarded of guardedEdges) {
+        expect(guarded.condition?.source).not.toContain('{');
+      }
     }
   });
 });