refactor(spec,lint): one declaration of where ADR-0031 regions live (#4401)

.changeset/region-slots-single-declaration.md

@@ -0,0 +1,44 @@
+---
+'@objectstack/spec': patch
+'@objectstack/lint': patch
+---
+
+One declaration of where ADR-0031 regions live (#4401).
+
+A region is a sub-graph inside `FlowNodeSchema.config`, an open `z.record`. Nothing in the
+type system says which key on which node type holds one, so every pass that needs to reach
+a region node has to be told — and within one week three of them were told separately, by
+two changes that were each correct on their own:
+
+| pass | package | table it carried |
+|------|---------|------------------|
+| `mapFlowNodes` (ADR-0087 conversions) | `spec` | `FLOW_REGION_SLOTS` |
+| `validateControlFlow` / `normalizeControlFlowRegions` / `collectFlowGraphs` | `spec` | `regionSlotsOf` |
+| `walkFlowNodes` (lint flow rules) | `lint` | `REGION_SLOTS` |
+
+Each pinned its own copy with its own reconciliation test. So every copy was protected from
+drifting away from the schemas, and **nothing would have failed if the copies drifted from
+each other** — while adding a fourth construct meant editing three places, and missing one
+reproduces exactly the silent blind spot #4347 and #4380 were both filed about.
+
+- New `@objectstack/spec/automation` export `FLOW_REGION_SLOTS` (plus the
+  `FLOW_REGION_SLOTS_BY_TYPE` / `FLOW_REGION_CONFIG_KEYS` views) is now the only statement
+  of the fact. It lives in an **import-free** module so `spec/conversions/walk.ts` can read
+  it and stay the pure shape walker it was written as; mapping a slot onto the Zod schema
+  its value parses as stays in `control-flow.zod.ts`, which is schema business.
+- The three reconciliation tests collapse into one, `region-slots.test.ts`, keeping the
+  strongest of them: it derives each construct's region keys **behaviourally**, by asking
+  the config schema what it actually accepts in a region shape, rather than reading names
+  off `.shape`. It also probes every other exported `*ConfigSchema`, so a new
+  region-bearing construct cannot be added without either declaring its slots or failing
+  here.
+
+The three **walks** are deliberately left separate. They take different inputs (parsed
+`FlowNodeParsed` vs raw authored records), yield different units (a graph, a node, a
+copy-on-write rewritten tree), and the lint one formats human diagnostic trails from node
+labels — consumer logic, not protocol (Prime Directive #2). Merging them would trade a
+duplicated four-line table for a walker that serves nobody well. Only the fact they all
+need is shared.
+
+No behaviour change: every existing test passes unchanged, which is the point of the
+exercise.

packages/lint/src/flow-walk.test.ts

@@ -2,9 +2,8 @@
 
 import { describe, it, expect } from 'vitest';
 import {
-  LoopConfigSchema,
-  ParallelConfigSchema,
-  TryCatchConfigSchema,
+  FLOW_REGION_SLOTS,
+  FLOW_REGION_CONFIG_KEYS,
 } from '@objectstack/spec/automation';
 
 import {
@@ -17,49 +16,21 @@ import {
 
 const node = (id: string, extra: Record<string, unknown> = {}) => ({ id, type: 'script', ...extra });
 
-describe('REGION_SLOTS — pinned against the spec, not restated', () => {
-  // The ledger's job is to make a NEW region-bearing construct fail here rather
-  // than become a fifth silent blind spot. Derived behaviourally from the
-  // spec's own config schemas: a region slot is one that accepts `{nodes: […]}`.
-  const REGION_BEARING_CONFIGS = {
-    try_catch: TryCatchConfigSchema,
-    loop: LoopConfigSchema,
-    parallel: ParallelConfigSchema,
-  } as const;
-
-  /** Keys of `schema` that accept a region (or an array of them). */
-  const regionKeysOf = (schema: { safeParse: (v: unknown) => { success: boolean; data?: unknown } }): string[] => {
-    // `label` is required by FlowNodeSchema — a probe node without it fails the
-    // parse and would make every slot look non-region.
-    const region = { nodes: [{ id: 'probe', type: 'script', label: 'Probe' }], edges: [] };
-    const probes: Record<string, unknown> = {
-      // Required siblings so the parse reaches the region keys at all.
-      collection: '{items}',
-      try: region,
-      catch: region,
-      body: region,
-      branches: [{ nodes: region.nodes, edges: [] }, { nodes: region.nodes, edges: [] }],
-    };
-    const parsed = schema.safeParse(probes);
-    if (!parsed.success) return [];
-    const data = parsed.data as Record<string, unknown>;
-    return Object.keys(data).filter((k) => {
-      const v = data[k];
-      if (Array.isArray(v)) return v.every((e) => !!e && typeof e === 'object' && Array.isArray((e as never)['nodes']));
-      return !!v && typeof v === 'object' && Array.isArray((v as Record<string, unknown>).nodes);
-    });
-  };
-
-  it('declares exactly the region slots each construct actually accepts', () => {
-    for (const [type, schema] of Object.entries(REGION_BEARING_CONFIGS)) {
-      expect([...(REGION_SLOTS.get(type) ?? [])].sort(), `region slots for '${type}'`).toEqual(
-        regionKeysOf(schema).sort(),
-      );
-    }
+/**
+ * The behavioural probe that used to live here — deriving each construct's
+ * region keys from its own config schema — moved to `region-slots.test.ts` in
+ * the spec, next to the single declaration it now reconciles (#4401). This side
+ * only has to prove the projection is faithful; a renamed or added slot fails
+ * over there, once, for all three walks.
+ */
+describe('REGION_SLOTS — projected from the spec, not restated', () => {
+  it('carries every declared slot, grouped by container type', () => {
+    const projected = [...REGION_SLOTS].flatMap(([type, keys]) => keys.map(key => `${type}.${key}`)).sort();
+    expect(projected).toEqual(FLOW_REGION_SLOTS.map(s => `${s.nodeType}.${s.key}`).sort());
   });
 
-  it('derives REGION_CONFIG_KEYS from the per-type slots', () => {
-    expect([...REGION_CONFIG_KEYS].sort()).toEqual([...new Set([...REGION_SLOTS.values()].flat())].sort());
+  it('shares the flat config-key set by reference', () => {
+    expect(REGION_CONFIG_KEYS).toBe(FLOW_REGION_CONFIG_KEYS);
   });
 });
 

packages/lint/src/flow-walk.ts

@@ -48,6 +48,8 @@
  * read named keys (`config.fields`, `config.objectName`) can use either.
  */
 
+import { FLOW_REGION_SLOTS_BY_TYPE, FLOW_REGION_CONFIG_KEYS } from '@objectstack/spec/automation';
+
 export type AnyRec = Record<string, unknown>;
 
 function isRec(v: unknown): v is AnyRec {
@@ -59,24 +61,28 @@ function strName(v: unknown): string | undefined {
 }
 
 /**
- * Config keys that hold a nested region, by owning node type. Declared as data
- * so a new construct is one entry here rather than a fifth silent blind spot,
- * and pinned against the spec's own region-bearing schemas by
- * `flow-walk.test.ts`.
+ * Config keys that hold a nested region, by owning node type.
+ *
+ * Projected from `@objectstack/spec/automation` rather than declared here
+ * (#4401). This used to be a local copy with its own reconciliation test — as
+ * did the ADR-0087 conversion walk's copy and the spec-side control-flow walk's.
+ * Three tables, three tests each pinning its own copy, and nothing that would
+ * fail if they drifted from ONE ANOTHER: every copy was individually protected
+ * and the set was not. A fourth construct is now a single entry in
+ * `spec/src/automation/region-slots.ts`.
+ *
+ * The **walk** below stays here. It takes raw authored records (not
+ * `FlowNodeParsed`), and it yields per-node diagnostic paths and label trails —
+ * formatting that is lint's business, not the protocol's (Prime Directive #2).
+ * Only the table is shared; the three traversals stay separate because they walk
+ * different units for different consumers.
  */
-export const REGION_SLOTS: ReadonlyMap<string, readonly string[]> = new Map([
-  ['try_catch', ['try', 'catch']],
-  ['loop', ['body']],
-  // `parallel` is the odd one: `config.branches[]` is an ARRAY of regions, not
-  // a region. Handled separately in the walk; named here so the key set stays
-  // one list.
-  ['parallel', ['branches']],
-]);
+export const REGION_SLOTS: ReadonlyMap<string, readonly string[]> = new Map(
+  [...FLOW_REGION_SLOTS_BY_TYPE].map(([type, slots]) => [type, slots.map(s => s.key)]),
+);
 
 /** Every config key that may hold region nodes, across all node types. */
-export const REGION_CONFIG_KEYS: ReadonlySet<string> = new Set(
-  [...REGION_SLOTS.values()].flat(),
-);
+export const REGION_CONFIG_KEYS: ReadonlySet<string> = FLOW_REGION_CONFIG_KEYS;
 
 /**
  * Depth cap. Regions are a tree in parsed metadata, so this is not a cycle

packages/spec/api-surface.json

@@ -2141,6 +2141,9 @@
     "ExecutionStepSkipReasonSchema (const)",
     "FLOW_BUILTIN_NODE_TYPES (const)",
     "FLOW_NODE_EXPRESSION_PATHS (const)",
+    "FLOW_REGION_CONFIG_KEYS (const)",
+    "FLOW_REGION_SLOTS (const)",
+    "FLOW_REGION_SLOTS_BY_TYPE (const)",
     "FLOW_STRUCTURAL_NODE_TYPES (const)",
     "Flow (type)",
     "FlowEdge (type)",
@@ -2155,8 +2158,10 @@
     "FlowNodeSchema (const)",
     "FlowParsed (type)",
     "FlowRegion (type)",
+    "FlowRegionArity (type)",
     "FlowRegionParsed (type)",
     "FlowRegionSchema (const)",
+    "FlowRegionSlotDecl (interface)",
     "FlowRunGateSummary (type)",
     "FlowRunGateSummarySchema (const)",
     "FlowRunNodeSummary (type)",

packages/spec/src/automation/control-flow.zod.ts

@@ -49,6 +49,7 @@ import { z } from 'zod';
 import { lazySchema } from '../shared/lazy-schema';
 import { FlowNodeSchema, FlowEdgeSchema } from './flow.zod';
 import type { FlowNodeParsed, FlowEdgeParsed } from './flow.zod';
+import { FLOW_REGION_SLOTS_BY_TYPE } from './region-slots';
 
 // ─── Canonical construct type ids ────────────────────────────────────
 
@@ -322,46 +323,51 @@ interface RegionSlot {
 }
 
 /**
- * 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.
+ * Resolve one node's region slots against the shared declaration
+ * ({@link FLOW_REGION_SLOTS_BY_TYPE}, #4401) — binding each declared slot to
+ * the value it holds, the Zod schema that value parses as, and a diagnostic
+ * label.
+ *
+ * The three passes in this module read it ({@link validateControlFlow},
+ * {@link normalizeControlFlowRegions}, {@link collectFlowGraphs}). WHERE the
+ * slots are is no longer stated here — that moved to `region-slots.ts` so the
+ * conversion walk and the lint walk read the same list. What stays here is the
+ * schema half, which is this module's business.
  *
  * 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<string, unknown> | 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 });
+  const declared = FLOW_REGION_SLOTS_BY_TYPE.get(node.type);
+  if (!declared) return [];
+
+  const slots: RegionSlot[] = [];
+  for (const { key, arity } of declared) {
+    const value = cfg[key];
+    if (arity === 'many') {
+      if (!Array.isArray(value)) continue;
+      value.forEach((raw, index) => slots.push({
+        raw,
+        key,
+        index,
+        label: `${node.type} '${node.id}' ${singularize(key)} ${index}`,
+        // A branch also carries an optional `name`, which the plain region
+        // schema (a non-strict `z.object`) would strip.
+        schema: ParallelBranchSchema,
+      }));
+      continue;
     }
-    if (cfg.catch != null) {
-      slots.push({ raw: cfg.catch, key: 'catch', label: `try_catch '${node.id}' catch`, schema: FlowRegionSchema });
-    }
-    return slots;
+    if (value == null) continue;
+    slots.push({ raw: value, key, label: `${node.type} '${node.id}' ${key}`, schema: FlowRegionSchema });
   }
-  return [];
+  return slots;
+}
+
+/** `branches` → `branch`, for the per-item label of a `many` slot. */
+function singularize(key: string): string {
+  return key.endsWith('es') ? key.slice(0, -2) : key.replace(/s$/, '');
 }
 
 /**

packages/spec/src/automation/index.ts

@@ -2,6 +2,7 @@
 
 
 export * from './flow.zod';
+export * from './region-slots';
 export * from './control-flow.zod';
 export * from './io-node-config.zod';
 export * from './builtin-node-config.zod';