diff --git a/.changeset/region-slots-single-declaration.md b/.changeset/region-slots-single-declaration.md new file mode 100644 index 0000000000..d88b382488 --- /dev/null +++ b/.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. diff --git a/packages/lint/src/flow-walk.test.ts b/packages/lint/src/flow-walk.test.ts index 033ab8435c..98c881519a 100644 --- a/packages/lint/src/flow-walk.test.ts +++ b/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 = {}) => ({ 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 = { - // 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; - 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).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); }); }); diff --git a/packages/lint/src/flow-walk.ts b/packages/lint/src/flow-walk.ts index 455df54ab0..a534cb53f4 100644 --- a/packages/lint/src/flow-walk.ts +++ b/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; 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 = 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 = 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 = new Set( - [...REGION_SLOTS.values()].flat(), -); +export const REGION_CONFIG_KEYS: ReadonlySet = FLOW_REGION_CONFIG_KEYS; /** * Depth cap. Regions are a tree in parsed metadata, so this is not a cycle diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index cf70b02d62..6663b80681 100644 --- a/packages/spec/api-surface.json +++ b/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)", diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index ca720ce5cd..9a29ccfc7b 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/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,11 +323,16 @@ 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. @@ -334,34 +340,34 @@ interface RegionSlot { function regionSlotsOf(node: FlowNodeParsed): RegionSlot[] { const cfg = node.config as Record | 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$/, ''); } /** diff --git a/packages/spec/src/automation/index.ts b/packages/spec/src/automation/index.ts index 4e4840a6bd..df4f6a8f3a 100644 --- a/packages/spec/src/automation/index.ts +++ b/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'; diff --git a/packages/spec/src/automation/region-slots.test.ts b/packages/spec/src/automation/region-slots.test.ts new file mode 100644 index 0000000000..01679b668f --- /dev/null +++ b/packages/spec/src/automation/region-slots.test.ts @@ -0,0 +1,131 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The one reconciliation test for the one region-slot declaration (#4401). + * + * There used to be three of these — one beside each walk that needed the table + * (`spec/conversions/walk.ts`, `spec/automation/control-flow.zod.ts` and + * `lint/flow-walk.ts`). Each pinned its own copy, so every copy was protected + * from drifting away from the schemas and *none* of them would have failed if + * the copies drifted from each other. One table, one test. + * + * The probe is behavioural rather than structural — it asks each container + * config schema what it actually ACCEPTS in a region shape, instead of reading + * key names off `.shape`. A slot renamed in the schema fails here; so does a + * slot that stops accepting a region. (Approach carried over from the + * `flow-walk.test.ts` version it replaces.) + */ + +import { describe, it, expect } from 'vitest'; + +import * as controlFlow from './control-flow.zod.js'; +import { LoopConfigSchema, ParallelConfigSchema, TryCatchConfigSchema } from './control-flow.zod.js'; +import { + FLOW_REGION_SLOTS, + FLOW_REGION_SLOTS_BY_TYPE, + FLOW_REGION_CONFIG_KEYS, +} from './region-slots.js'; + +/** Every container construct that carries a region, and its config schema. */ +const REGION_BEARING_CONFIGS = { + loop: LoopConfigSchema, + parallel: ParallelConfigSchema, + try_catch: TryCatchConfigSchema, +} as const; + +/** A minimal well-formed region. `label` is required by `FlowNodeSchema`. */ +const region = () => ({ nodes: [{ id: 'probe', type: 'script', label: 'Probe' }], edges: [] }); + +/** + * Ask a config schema which of its keys accept a region — by handing it every + * candidate at once and seeing which survive the parse in a region shape. + * Returns each surviving key with the arity it parsed at. + */ +function regionSlotsAccepted( + schema: { safeParse: (v: unknown) => { success: boolean; data?: unknown } }, +): Array<{ key: string; arity: 'one' | 'many' }> { + const probes: Record = { + // Required siblings, so the parse reaches the region keys at all. + collection: '{items}', + body: region(), + try: region(), + catch: region(), + branches: [region(), region()], + }; + const parsed = schema.safeParse(probes); + if (!parsed.success) return []; + const data = parsed.data as Record; + const isRegion = (v: unknown) => !!v && typeof v === 'object' && Array.isArray((v as Record).nodes); + + const out: Array<{ key: string; arity: 'one' | 'many' }> = []; + for (const [key, value] of Object.entries(data)) { + if (Array.isArray(value) && value.length > 0 && value.every(isRegion)) out.push({ key, arity: 'many' }); + else if (isRegion(value)) out.push({ key, arity: 'one' }); + } + return out; +} + +const byKey = (a: { key: string }, b: { key: string }) => a.key.localeCompare(b.key); + +describe('#4401 — FLOW_REGION_SLOTS reconciles with the ADR-0031 construct schemas', () => { + it('declares exactly the slots each construct accepts, at the right arity', () => { + for (const [nodeType, schema] of Object.entries(REGION_BEARING_CONFIGS)) { + const declared = FLOW_REGION_SLOTS + .filter(s => s.nodeType === nodeType) + .map(s => ({ key: s.key, arity: s.arity })) + .sort(byKey); + expect(declared, `region slots for '${nodeType}'`).toEqual(regionSlotsAccepted(schema).sort(byKey)); + } + }); + + it('covers every region-bearing construct — no fourth one goes unlisted', () => { + expect([...FLOW_REGION_SLOTS_BY_TYPE.keys()].sort()) + .toEqual(Object.keys(REGION_BEARING_CONFIGS).sort()); + }); + + /** + * Closes the loop the list above would otherwise leave open: a new construct + * whose author forgets BOTH the table and this test's map would pass silently. + * Every `*ConfigSchema` this module exports is probed, so the only way to add + * a region-bearing construct without failing here is to declare it. + */ + it('and no other exported config schema quietly accepts one', () => { + const accounted = new Set(Object.values(REGION_BEARING_CONFIGS)); + const probed: string[] = []; + for (const [name, exported] of Object.entries(controlFlow)) { + if (!name.endsWith('ConfigSchema') || accounted.has(exported)) continue; + probed.push(name); + expect( + regionSlotsAccepted(exported as never), + `${name} accepts a region shape but is not in FLOW_REGION_SLOTS`, + ).toEqual([]); + } + // Guard against the probe going vacuous if the naming convention changes. + expect(probed.length + accounted.size).toBeGreaterThanOrEqual(3); + }); + + it('marks exactly the array-valued slot as `many`', () => { + expect(FLOW_REGION_SLOTS.filter(s => s.arity === 'many').map(s => `${s.nodeType}.${s.key}`)) + .toEqual(['parallel.branches']); + }); +}); + +describe('#4401 — the derived views stay in step with the source list', () => { + it('indexes every slot by its container type', () => { + expect([...FLOW_REGION_SLOTS_BY_TYPE.values()].flat().sort(byKey)) + .toEqual([...FLOW_REGION_SLOTS].sort(byKey)); + }); + + it('flattens every config key into FLOW_REGION_CONFIG_KEYS', () => { + expect([...FLOW_REGION_CONFIG_KEYS].sort()) + .toEqual([...new Set(FLOW_REGION_SLOTS.map(s => s.key))].sort()); + }); + + it('resolves a hostile node type to nothing instead of Object.prototype', () => { + // `node.type` is an open, author-controlled namespace (ADR-0018). An object + // literal would hand a walk `Object`'s constructor here. + for (const hostile of ['constructor', '__proto__', 'toString', 'hasOwnProperty']) { + expect(FLOW_REGION_SLOTS_BY_TYPE.get(hostile)).toBeUndefined(); + } + }); +}); diff --git a/packages/spec/src/automation/region-slots.ts b/packages/spec/src/automation/region-slots.ts new file mode 100644 index 0000000000..c10a9278ea --- /dev/null +++ b/packages/spec/src/automation/region-slots.ts @@ -0,0 +1,98 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * @module automation/region-slots + * + * **Where an ADR-0031 container keeps its nested region(s)** — the one + * declaration of that fact (#4401). + * + * A region is a self-contained sub-graph living inside `FlowNodeSchema.config`, + * which is deliberately 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: + * + * | pass | package | unit it walks | + * |------|---------|---------------| + * | `mapFlowNodes` (ADR-0087 conversions) | `spec` | node, copy-on-write rewrite | + * | `collectFlowGraphs` / `validateControlFlow` / `normalizeControlFlowRegions` | `spec` | graph (nodes + edges) | + * | `walkFlowNodes` (lint flow rules) | `lint` | node, with diagnostic path | + * + * Each carried its own copy of the table below, and each pinned its own copy + * with its own reconciliation test — so every copy was individually protected + * from drift and *nothing* would have failed if they drifted from each other. + * Adding a fourth construct meant editing three places, and the failure mode + * for missing one is precisely the silent blind spot #4347 and #4380 were both + * filed about. + * + * The **walks** are deliberately NOT merged: they take different inputs (parsed + * `FlowNodeParsed` vs raw authored records), yield different units (a graph, a + * node, a rewritten tree), and one of them formats human diagnostic trails, + * which is consumer logic rather than protocol (Prime Directive #2). Only the + * fact they all need — *this* table — is shared. + * + * Deliberately **import-free**: `spec/conversions/walk.ts` is a pure shape + * walker that takes no schema dependency, and this module has to be usable from + * there. It is data, not schema; `control-flow.zod.ts` is what maps a slot onto + * the Zod schema its value parses as. + */ + +/** How many regions a slot holds. */ +export type FlowRegionArity = + /** The slot's value IS a region (`loop.config.body`). */ + | 'one' + /** The slot's value is an ARRAY of regions (`parallel.config.branches`). */ + | 'many'; + +/** One `config` key on one container node type that holds a region. */ +export interface FlowRegionSlotDecl { + /** The container's `node.type`. */ + readonly nodeType: string; + /** The `config` key holding the region(s). */ + readonly key: string; + readonly arity: FlowRegionArity; +} + +/** + * Every region-bearing `config` slot, across every ADR-0031 container. + * + * Pinned against the container config schemas that actually declare these keys + * (`LoopConfigSchema` / `ParallelConfigSchema` / `TryCatchConfigSchema`) by + * `region-slots.test.ts`, so a renamed or added slot fails there rather than + * going quietly unwalked by all three passes at once. + */ +export const FLOW_REGION_SLOTS: readonly FlowRegionSlotDecl[] = [ + { nodeType: 'loop', key: 'body', arity: 'one' }, + { nodeType: 'parallel', key: 'branches', arity: 'many' }, + { nodeType: 'try_catch', key: 'try', arity: 'one' }, + { nodeType: 'try_catch', key: 'catch', arity: 'one' }, +]; + +/** + * {@link FLOW_REGION_SLOTS} indexed by container type — the lookup a walk does + * per node. + * + * A `Map` rather than an object literal on purpose: `node.type` is + * author-controlled and an open namespace (ADR-0018 removed the enum gate), so + * an object lookup would resolve `'constructor'` / `'__proto__'` through + * `Object`'s prototype chain and hand a walk something that is not a slot list. + */ +export const FLOW_REGION_SLOTS_BY_TYPE: ReadonlyMap = + new Map( + [...new Set(FLOW_REGION_SLOTS.map(s => s.nodeType))].map(nodeType => [ + nodeType, + FLOW_REGION_SLOTS.filter(s => s.nodeType === nodeType), + ]), + ); + +/** + * Every `config` key that may hold a region, across all container types. + * + * For the walks that need the flat key set rather than the per-type lookup — + * chiefly "give me this container's config *without* its regions", the view a + * rule scanning config recursively must read or it reports every descendant's + * finding a second time against the container that physically contains it. + */ +export const FLOW_REGION_CONFIG_KEYS: ReadonlySet = new Set( + FLOW_REGION_SLOTS.map(s => s.key), +); diff --git a/packages/spec/src/conversions/region-walk.test.ts b/packages/spec/src/conversions/region-walk.test.ts index 4fd3242d30..3851b17df8 100644 --- a/packages/spec/src/conversions/region-walk.test.ts +++ b/packages/spec/src/conversions/region-walk.test.ts @@ -14,16 +14,8 @@ import { describe, expect, it } from 'vitest'; -import { - LOOP_NODE_TYPE, - PARALLEL_NODE_TYPE, - TRY_CATCH_NODE_TYPE, - LoopConfigSchema, - ParallelConfigSchema, - TryCatchConfigSchema, -} from '../automation/control-flow.zod.js'; +import { FLOW_REGION_SLOTS_BY_TYPE } from '../automation/region-slots.js'; import { applyConversions, applyConversionsToFlow, collectConversionNotices } from './apply.js'; -import { FLOW_REGION_SLOTS } from './walk.js'; /** A protocol-11 callout alias — renamed to `http` by the table. */ const webhookNode = (id: string) => ({ id, type: 'webhook', label: id, config: { url: 'https://x' } }); @@ -224,29 +216,24 @@ describe('#4347 — the region walk stays copy-on-write and shape-gated', () => }); /** - * The slot table in `walk.ts` is declared locally so the conversion walker stays - * free of schema imports. This is what keeps that copy honest: a fourth ADR-0031 - * container, or a renamed region key, fails here instead of silently going - * unwalked — which is exactly the shape of the #4347 defect. + * The conversion walk no longer states WHERE regions live — it reads the shared + * declaration (#4401), which `region-slots.test.ts` reconciles against the + * construct schemas. What is worth pinning here is that this walk actually + * covers every declared container, so a fourth construct added to the shared + * table cannot land while the conversion pass keeps skipping it. */ -describe('#4347 — FLOW_REGION_SLOTS reconciles with the ADR-0031 constructs', () => { - it('covers every canonical container type id', () => { - expect([...FLOW_REGION_SLOTS.keys()].sort()) - .toEqual([LOOP_NODE_TYPE, PARALLEL_NODE_TYPE, TRY_CATCH_NODE_TYPE].sort()); - }); - - it('names region keys the container config schemas actually declare', () => { - const declared = (schema: { shape: Record }) => Object.keys(schema.shape); - const slotKeys = (type: string) => FLOW_REGION_SLOTS.get(type)!.map(s => s.key); - - expect(declared(LoopConfigSchema as never)).toEqual(expect.arrayContaining(slotKeys(LOOP_NODE_TYPE))); - expect(declared(ParallelConfigSchema as never)).toEqual(expect.arrayContaining(slotKeys(PARALLEL_NODE_TYPE))); - expect(declared(TryCatchConfigSchema as never)).toEqual(expect.arrayContaining(slotKeys(TRY_CATCH_NODE_TYPE))); - }); - - it('marks exactly the array-valued slot as `many`', () => { - const many = [...FLOW_REGION_SLOTS].flatMap(([type, slots]) => - slots.filter(s => s.many).map(s => `${type}.${s.key}`)); - expect(many).toEqual(['parallel.branches']); +describe('#4401 — the conversion walk descends into every declared container', () => { + it('reaches a region node under each container type in the shared table', () => { + for (const nodeType of FLOW_REGION_SLOTS_BY_TYPE.keys()) { + for (const { key, arity } of FLOW_REGION_SLOTS_BY_TYPE.get(nodeType)!) { + const value = arity === 'many' ? [region(webhookNode('inner'))] : region(webhookNode('inner')); + const { stack } = collectConversionNotices({ + flows: [{ name: 'f', nodes: [{ id: 'c', type: nodeType, label: 'C', config: { [key]: value } }] }], + }); + const slot = ((stack.flows as any[])[0].nodes[0].config)[key]; + const converted = (arity === 'many' ? slot[0] : slot).nodes[0]; + expect(converted, `${nodeType}.${key}`).toEqual(httpNode('inner')); + } + } }); }); diff --git a/packages/spec/src/conversions/walk.ts b/packages/spec/src/conversions/walk.ts index 3a8bc9d186..e4a2381757 100644 --- a/packages/spec/src/conversions/walk.ts +++ b/packages/spec/src/conversions/walk.ts @@ -13,46 +13,28 @@ * preserved all the way up. */ +import { FLOW_REGION_SLOTS_BY_TYPE } from '../automation/region-slots.js'; + type Dict = Record; function isDict(v: unknown): v is Dict { return typeof v === 'object' && v !== null && !Array.isArray(v); } -/** - * Where each ADR-0031 structured container carries its nested region(s) — the - * map that makes {@link mapFlowNodes} reach a node inside a `loop` body, a - * `parallel` branch, or a `try_catch` block (#4347). - * - * A region is a self-contained mini-flow (`{ nodes, edges }`) living in the - * container's OPEN `config` record, so a pass that walked only - * `flows[].nodes[]` saw the container and stopped there. The consequence was - * **position-dependent metadata**: the same node converted at the top level and - * did not one level in — a `webhook` callout inside a loop body kept its - * protocol-11 type (no executor owns that id, so the run fails), 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. Same flow, same - * `registerFlow` call, different outcome by nesting depth. - * - * Declared here rather than imported from `automation/control-flow.zod.ts` to - * keep this module free of schema imports (it is a pure shape walker). The ids - * and keys are pinned to that module's `LOOP_NODE_TYPE` / `PARALLEL_NODE_TYPE` / - * `TRY_CATCH_NODE_TYPE` and to the `LoopConfigSchema` / `ParallelConfigSchema` / - * `TryCatchConfigSchema` shapes by a reconciliation test (`region-walk.test.ts`), - * so a new construct cannot be added there and silently go unwalked here. +/* + * Reaching a node inside an ADR-0031 region (#4347) is what makes a conversion + * position-independent. A pass that walked only `flows[].nodes[]` saw the + * container and stopped: the same node converted at the top level and did not + * one level in — a `webhook` callout inside a loop body kept a type no executor + * owns (the run fails), and a `delete_record` kept `config.filters`, leaving the + * canonical `filter` the executor reads absent, which is the erased-condition + * hazard `flow-node-crud-filter-alias` exists to prevent. * - * A `Map` rather than an object literal so a node whose `type` is - * `'constructor'` / `'__proto__'` cannot reach `Object`'s prototype chain. + * WHERE those regions live is declared once in `automation/region-slots.ts` + * (#4401) — an import-free data module, so this stays the pure shape walker it + * was written as and still shares one table with the schema-side walks and the + * lint walk. */ -export const FLOW_REGION_SLOTS: ReadonlyMap< - string, - ReadonlyArray<{ readonly key: string; readonly many: boolean }> -> = new Map([ - ['loop', [{ key: 'body', many: false }]], - ['parallel', [{ key: 'branches', many: true }]], - ['try_catch', [{ key: 'try', many: false }, { key: 'catch', many: false }]], -]); /** * Depth ceiling for the region recursion. Containers nest, but not deeply, and @@ -105,15 +87,15 @@ function mapNodeTree( ): Dict { const mapped = mapper(node, path); if (depth >= MAX_REGION_DEPTH) return mapped; - const slots = typeof mapped.type === 'string' ? FLOW_REGION_SLOTS.get(mapped.type) : undefined; + const slots = typeof mapped.type === 'string' ? FLOW_REGION_SLOTS_BY_TYPE.get(mapped.type) : undefined; if (!slots) return mapped; const config = mapped.config; if (!isDict(config)) return mapped; let nextConfig = config; - for (const { key, many } of slots) { + for (const { key, arity } of slots) { const raw = nextConfig[key]; - if (many) { + if (arity === 'many') { if (!Array.isArray(raw)) continue; let branchesChanged = false; const nextBranches = raw.map((branch, i) => {