From 862c4f53936f9fa490e27bfb08b8d22eca2a1c3d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:02:09 +0000 Subject: [PATCH] refactor(spec,lint): one declaration of where ADR-0031 regions live (#4401) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#4381 and #4388): 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` (+ the `FLOW_REGION_SLOTS_BY_TYPE` / `FLOW_REGION_CONFIG_KEYS` views) is the only statement of the fact. 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`. - The three reconciliation tests collapse into `region-slots.test.ts`, keeping the strongest of them (#4388's): it derives each construct's region keys BEHAVIOURALLY, by asking the config schema what it 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: different inputs (parsed vs raw authored records), 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). No behaviour change: every existing test passes unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HSBbKMdDgHjGpQdrvQUQXj --- .changeset/region-slots-single-declaration.md | 44 ++++++ packages/lint/src/flow-walk.test.ts | 59 ++------ packages/lint/src/flow-walk.ts | 36 +++-- packages/spec/api-surface.json | 5 + .../spec/src/automation/control-flow.zod.ts | 68 ++++----- packages/spec/src/automation/index.ts | 1 + .../spec/src/automation/region-slots.test.ts | 131 ++++++++++++++++++ packages/spec/src/automation/region-slots.ts | 98 +++++++++++++ .../spec/src/conversions/region-walk.test.ts | 51 +++---- packages/spec/src/conversions/walk.ts | 52 +++---- 10 files changed, 388 insertions(+), 157 deletions(-) create mode 100644 .changeset/region-slots-single-declaration.md create mode 100644 packages/spec/src/automation/region-slots.test.ts create mode 100644 packages/spec/src/automation/region-slots.ts 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) => {