From a1e297bba29f2d9eac88c6471a203320c206c121 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 07:35:36 +0000 Subject: [PATCH 1/5] =?UTF-8?q?feat(spec)!:=20one=20node-id=20space=20?= =?UTF-8?q?=E2=80=94=20`FlowSchema`=20refuses=20a=20region=20node=20whose?= =?UTF-8?q?=20id=20is=20declared=20elsewhere=20in=20the=20flow=20(#16134)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `superRefine` pass that refused a duplicate in the top-level `nodes[]` now walks every graph in the flow via `collectFlowGraphs` — top-level first, then each region in document order, depth first — keeping one map of first declarations. A later occurrence raises the same single `custom` issue, anchored at the later node's own `id` inside its region and naming both locations as a top-level index (`nodes[1]`) or a region path (`loop 'sweep' body → nodes[0]`). One refusal, one shape. `collectFlowGraphs` gains `path` beside `scope` (the key path to the graph, so the issue can be anchored where the author wrote it) and skips a non-object element of a region its own schema refused: that walk now runs inside the parse, where the TypeError it used to throw from `validateControlFlow` would escape `safeParse`. `analyzeRegion` keeps its per-region uniqueness line as an invariant for raw-region callers; a flow that parses never reaches it with a collision. The #15713 boundary pin moves deliberately from "parses" to "is refused"; the #16134 describe pins the region path, formatZodError line, sibling branches, nested depth, walk-order anchoring, one issue for a within-region duplicate, a raw region's authored ids still judged, the non-object guard, and the accept side. ADR-0031 gains the one sentence the ruling asked for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x --- .changeset/flow-one-node-id-space.md | 91 ++++++++ ...31-advanced-flow-node-executors-and-dag.md | 4 + .../spec/src/automation/control-flow.zod.ts | 34 ++- packages/spec/src/automation/flow.test.ts | 213 +++++++++++++++++- packages/spec/src/automation/flow.zod.ts | 69 +++--- .../automation/region-normalization.test.ts | 19 ++ 6 files changed, 389 insertions(+), 41 deletions(-) create mode 100644 .changeset/flow-one-node-id-space.md diff --git a/.changeset/flow-one-node-id-space.md b/.changeset/flow-one-node-id-space.md new file mode 100644 index 0000000000..180bd0b3f7 --- /dev/null +++ b/.changeset/flow-one-node-id-space.md @@ -0,0 +1,91 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec)!: `FlowSchema` refuses a region node whose id is already declared elsewhere in the flow — one node-id space across the top-level `nodes[]` and every region body (#16134) + + + +**BREAKING** accept-set narrowing on `FlowSchema` — a flow has **one node-id +space**. A node inside an ADR-0031 region body (`loop.config.body`, +`try_catch.config.try` / `.catch`, each `parallel.config.branches[]`, nested to +any depth) whose `id` is already declared by a top-level node, or by a node in +any other region of the same flow, is now **refused at parse time** — by +`FlowSchema.parse` / `safeParse`, `defineFlow`, and every door that validates a +flow through the schema (`objectstack validate`, the runtime publish gate, a +stack's `flows[]`) — where it used to parse on green. Shipped as `minor` under +the repo's launch-window convention for breaking changes. Maintainer ruling +(director seat, decision batch #61, 2026-09-07, 「同意」): ADR-0031's +"self-contained single-entry / single-exit sub-graph" describes control flow and +variable scope, not id reuse; every reader that flattens a flow may key on the +bare id. The ADR gains one sentence saying so in this same change. + +Before this change uniqueness was enforced **inside** each array — the +top-level `nodes[]` by `FlowSchema` (#15713) and each region body by +`analyzeRegion` at `registerFlow()` — and never **across** them: a loop-body +node could carry the same `id` as a top-level node, or as a node in a sibling +branch, and both rules stayed green. Every edge's `source` / `target` names a +node by id, and the designer canvas, the BPMN export, a flow diff and a +checkpoint's `completedNodeIds` all key on the bare id, so such a collision was +silently wrong wherever a flow is flattened. + +**What changes** (`packages/spec/src/automation/flow.zod.ts`): the existing +`superRefine` pass over `nodes[]` now walks every graph in the flow via +`collectFlowGraphs` — the top-level graph first, then each region in document +order, depth first — keeping one map of first declarations. A later occurrence +raises the same single `custom` issue as before, anchored at the later node's +own `id` (inside the region, e.g. `nodes.1.config.body.nodes.0.id`) and naming +both locations — a top-level index (`nodes[1]`) or a region path +(`loop 'sweep' body → nodes[0]`): + +```text +✗ nodes.1.config.body.nodes.0.id: Duplicate node id `start` — `loop 'n' body → nodes[0]` reuses the id already declared by `nodes[0]`; every node id in a flow must be unique. Rename one of them: … +``` + +One refusal, one message shape: an author never sees two issues for one +collision. `analyzeRegion` keeps its per-region uniqueness line as an invariant +for direct raw-region callers (`bpmn-mapping`), but a flow that parses never +reaches it with a collision, and a flow with one never parses. +`collectFlowGraphs` gains a `path` field beside `scope` — the same location as +a key path — so the issue can be anchored where the author wrote the node; it +also now skips a non-object element in a region its own schema refused (such a +region is left raw for `validateControlFlow` to name), where it used to throw a +`TypeError` from inside that validator. + +**What does NOT change:** `nodes[].id` keeps its name, type and describe; the +open node-type vocabulary (ADR-0018), the region rules (edge integrity, +single-entry / single-exit, acyclicity) and every other refusal are untouched; +a flow whose node ids are unique across the whole flow parses exactly as +before, region nodes included, in authored order. + +The shape that is refused, and what the author does about it — the region node +renamed, and any region edge that meant it re-pointed: + +```ts +// before — parsed on green, `start` declared twice (top level + loop body) +nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'sweep', type: 'loop', label: 'Sweep', config: { collection: '{items}', body: { + nodes: [{ id: 'start', type: 'assignment', label: 'First step' }], + } } }, + { id: 'end', type: 'end', label: 'End' }, +] + +// after — refused at parse (nodes.1.config.body.nodes.0.id: Duplicate node id `start` …); +// rename the region node and point the region's edges that meant it at the new id: +nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'sweep', type: 'loop', label: 'Sweep', config: { collection: '{items}', body: { + nodes: [{ id: 'sweep_first', type: 'assignment', label: 'First step' }], + } } }, + { id: 'end', type: 'end', label: 'End' }, +] +``` + +**Remedy.** Rename the later node to an id nothing else in that flow carries — +no top-level node, no node in any region — then re-point at the new id the +edges whose `source` / `target` meant it; nothing else in the flow needs to +move. The census over this repository found no flow to migrate, so this is a +release note, not a migration: no shipped example, fixture or seed in +`packages/**` or `examples/**` declares a region node id that collides with a +top-level or another region's node id. diff --git a/docs/adr/0031-advanced-flow-node-executors-and-dag.md b/docs/adr/0031-advanced-flow-node-executors-and-dag.md index 0b25464745..d3974a8bbf 100644 --- a/docs/adr/0031-advanced-flow-node-executors-and-dag.md +++ b/docs/adr/0031-advanced-flow-node-executors-and-dag.md @@ -139,6 +139,10 @@ self-contained region in `config` — `config.body` for `loop`, 1. **Well-formed by construction.** A nested region is its *own* graph, so single-entry is intrinsic and there are no scope markers to balance or leak across — validation (`analyzeRegion`/`validateControlFlow`) is local. + "Self-contained" describes control flow and variable scope, not id reuse: a + flow's node ids are **one space** across its top-level `nodes[]` and every + region body at every depth, and `FlowSchema` refuses a collision at parse + (#16134). 2. **The shared `engine.ts` traversal stays untouched.** The container executor runs its body via a scoped `AutomationEngine.runRegion()`; the main DAG `traverseNext` never learns about scope markers (deliberate, given the diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index 23fd589344..dbe74ab979 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -406,7 +406,11 @@ export function analyzeRegion(region: { nodes: FlowNodeParsed[]; edges?: FlowEdg return { errors: ['region has no nodes'] }; } - // Unique ids. + // Unique ids — an invariant this analysis needs (the degree maps below key + // on id), not the author-facing rule. A flow has ONE node-id space, judged + // by `FlowSchema` at parse over every depth (#16134), so a parsed flow never + // arrives here carrying a collision and no author sees this line; it guards + // direct callers that hand in a raw region (`bpmn-mapping`). const ids = new Set(); for (const n of nodes) { if (ids.has(n.id)) errors.push(`duplicate node id '${n.id}'`); @@ -681,6 +685,15 @@ export interface FlowGraph { * `loop 'sweep' body → try_catch 'guard' catch`. */ readonly scope: string; + /** + * The same location as {@link scope}, as the key path from the flow root to + * the object holding this graph's `nodes` / `edges`: `[]` for the flow + * itself, `['nodes', 1, 'config', 'body']` for a loop body, + * `['nodes', 1, 'config', 'branches', 0]` for a parallel branch. A Zod issue + * about a region node is anchored where the author wrote it — + * `[...path, 'nodes', i, 'id']` — rather than described in prose (#16134). + */ + readonly path: readonly (string | number)[]; readonly nodes: readonly FlowNodeParsed[]; readonly edges: readonly FlowEdgeParsed[]; } @@ -706,23 +719,34 @@ export function collectFlowGraphs( nodes: readonly FlowNodeParsed[], edges: readonly FlowEdgeParsed[], scope: string, + path: readonly (string | number)[], depth: number, ): void => { - graphs.push({ scope, nodes, edges }); + graphs.push({ scope, path, nodes, edges }); if (depth >= MAX_REGION_DEPTH) return; - for (const node of nodes) { + nodes.forEach((node, index) => { + // A region its own schema refused is left RAW by `parseFlowNodeRegions` + // for `validateControlFlow` to name, so an element here can be whatever + // the author typed — `null` included. Skip what is not a node object + // rather than read `.config` off it: this walk runs inside `FlowSchema`'s + // parse (#16134), where a thrown TypeError would escape `safeParse`. The + // schema refusal that owns the malformed region still fires — reached now, + // where the throw used to pre-empt it. + const raw: unknown = node; + if (raw === null || typeof raw !== 'object') return; for (const slot of regionSlotsOf(node)) { if (!isRegionDict(slot.raw) || !Array.isArray(slot.raw.nodes)) continue; visit( slot.raw.nodes as FlowNodeParsed[], Array.isArray(slot.raw.edges) ? (slot.raw.edges as FlowEdgeParsed[]) : [], scope ? `${scope} → ${slot.label}` : slot.label, + [...path, 'nodes', index, 'config', slot.key, ...(slot.index === undefined ? [] : [slot.index])], depth + 1, ); } - } + }); }; - visit(flow.nodes ?? [], flow.edges ?? [], '', 0); + visit(flow.nodes ?? [], flow.edges ?? [], '', [], 0); return graphs; } diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 05cd06b1c5..48776db0b4 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -27,7 +27,7 @@ import { // `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'; +import { LoopConfigSchema, collectFlowGraphs, validateControlFlow } from './control-flow.zod'; import { formatZodError } from '../shared/error-map.zod'; describe('FlowNodeAction', () => { @@ -2121,14 +2121,14 @@ describe('FlowSchema — top-level node ids are unique (#15713)', () => { expect(issues![0].message).toContain('Duplicate node id `n`'); }); - // Scope boundary, pinned so the rule cannot silently widen: it judges the - // flow's OWN top-level `nodes[]`. A region body (`loop.config.body.nodes`) is - // `analyzeRegion`'s to judge, at `registerFlow()`, and whether a region node - // may reuse a top-level id — one id space or two — is an open decision - // (#16134) that this rule neither takes nor pre-empts. This pin records - // today's accept set at that boundary; the decision, when taken, moves it - // deliberately. - it('judges the top-level nodes[] only — a region node reusing a top-level id is outside this rule', () => { + // Scope boundary, MOVED deliberately (#16134, maintainer ruling 2026-09-07: + // one node-id space). #15713 pinned that this rule judged the flow's OWN + // top-level `nodes[]` and that a region node reusing a top-level id parsed — + // recording the accept set at the boundary so the decision, when taken, + // would move it rather than drift. Taken: the same shape is now refused, by + // the same rule, in the same shape. The full region pin set is the #16134 + // describe below; this one is the boundary itself. + it('a region node reusing a top-level id is refused — the #15713 boundary, moved by #16134', () => { const result = FlowSchema.safeParse(flowWith([ { id: 'start', type: 'start', label: 'Start' }, { @@ -2142,8 +2142,201 @@ describe('FlowSchema — top-level node ids are unique (#15713)', () => { }, { id: 'end', type: 'end', label: 'End' }, ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 1, 'config', 'body', 'nodes', 0, 'id']], + ]); + expect(result.error.issues[0].message).toContain('Duplicate node id `start`'); + }); +}); + +describe('FlowSchema — one node-id space across the top-level nodes[] and every region (#16134)', () => { + // The ruling (director seat, decision batch #61, 2026-09-07, maintainer + // 「同意」): top-level `nodes[]` and every region body (`loop` / `try_catch` / + // `parallel`, at every depth) share ONE id space; a collision is refused at + // parse, by the rule that already refused top-level duplicates, in its one + // message shape, naming both locations. "Declared by" is the earlier + // position in the `collectFlowGraphs` walk — the top-level graph first, then + // each region in document order, depth first — so the top-level array is + // always the first declaration and a region node is the one that moves. + const edges: Flow['edges'] = [ + { id: 'e1', source: 'start', target: 'n' }, + { id: 'e2', source: 'n', target: 'end' }, + ]; + const flowWith = (nodes: Flow['nodes']): Flow => ({ + name: 'one_id_space', + label: 'One id space', + type: 'autolaunched', + nodes, + edges, + }); + const step = (id: string, label = id): FlowNode => ({ id, type: 'assignment', label }); + const loopOver = (bodyNodes: FlowNode[], bodyEdges: FlowEdge[] = []): FlowNode => ({ + id: 'n', type: 'loop', label: 'Loop', + config: { collection: '{items}', body: { nodes: bodyNodes, edges: bodyEdges } }, + }); + const bodyReusesStart = flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([step('start', 'Body step (reuses the top-level start)')]), + { id: 'end', type: 'end', label: 'End' }, + ]); + + it('refuses a loop-body node that reuses a top-level id — ONE issue, anchored at the region node, naming the region path and the top-level index', () => { + const result = FlowSchema.safeParse(bodyReusesStart); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues).toHaveLength(1); + const [issue] = result.error.issues; + expect(issue.code).toBe('custom'); + expect(issue.path).toEqual(['nodes', 1, 'config', 'body', 'nodes', 0, 'id']); + expect(issue.message).toMatch(/^Duplicate node id `start` — `loop 'n' body → nodes\[0\]` reuses the id already declared by `nodes\[0\]`; every node id in a flow must be unique\. /); + }); + + it('renders through formatZodError as a line that points INTO the region, in the same shape as a top-level duplicate', () => { + const result = FlowSchema.safeParse(bodyReusesStart); + expect(result.success).toBe(false); + if (result.success) return; + const rendered = formatZodError(result.error); + expect(rendered).toContain('Validation failed (1 issue):'); + expect(rendered).toContain( + "✗ nodes.1.config.body.nodes.0.id: Duplicate node id `start` — `loop 'n' body → nodes[0]` reuses the id already declared by `nodes[0]`", + ); + }); + + it('defineFlow refuses it with the same anchored issue', () => { + let caught: unknown; + try { + defineFlow(bodyReusesStart); + } catch (error) { + caught = error; + } + const issues = (caught as { issues?: Array<{ code: string; path: PropertyKey[] }> })?.issues; + expect(issues).toBeDefined(); + expect(issues!.map((i) => [i.code, i.path])).toEqual([['custom', ['nodes', 1, 'config', 'body', 'nodes', 0, 'id']]]); + }); + + it('refuses a node in one parallel branch that reuses an id declared in a sibling branch — both locations are region paths', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'n', type: 'parallel', label: 'Fan out', + config: { branches: [{ nodes: [step('work')] }, { nodes: [step('work')] }] }, + }, + { id: 'end', type: 'end', label: 'End' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 1, 'config', 'branches', 1, 'nodes', 0, 'id']], + ]); + expect(result.error.issues[0].message).toContain( + "`parallel 'n' branch 1 → nodes[0]` reuses the id already declared by `parallel 'n' branch 0 → nodes[0]`", + ); + }); + + it('walks every depth — a try_catch catch-region node nested inside a loop body that reuses a top-level id is refused with the chained region path', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([{ + id: 'tc', type: 'try_catch', label: 'Guard', + config: { try: { nodes: [step('attempt')] }, catch: { nodes: [step('end', 'Reuses the top-level end')] } }, + }]), + { id: 'end', type: 'end', label: 'End' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 1, 'config', 'body', 'nodes', 0, 'config', 'catch', 'nodes', 0, 'id']], + ]); + expect(result.error.issues[0].message).toContain( + "`loop 'n' body → try_catch 'tc' catch → nodes[0]` reuses the id already declared by `nodes[2]`", + ); + }); + + it('the top-level array is always the first declaration — a region node colliding with a LATER top-level node is the one refused', () => { + // Document order would put the body node first; walk order puts the + // whole top-level graph first. Pinned so the anchor cannot flip by drift. + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([step('end', 'Body step declared before the top-level end in document order')]), + { id: 'end', type: 'end', label: 'End' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => i.path)).toEqual([['nodes', 1, 'config', 'body', 'nodes', 0, 'id']]); + expect(result.error.issues[0].message).toContain("`loop 'n' body → nodes[0]` reuses the id already declared by `nodes[2]`"); + }); + + it('a duplicate WITHIN one region is refused by this rule alone — one issue, so analyzeRegion is never a second refusal for the author', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([step('a'), step('a', 'A again')], [{ id: 'b1', source: 'a', target: 'a' }]), + { id: 'end', type: 'end', label: 'End' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 1, 'config', 'body', 'nodes', 1, 'id']], + ]); + expect(result.error.issues[0].message).toContain("`loop 'n' body → nodes[1]` reuses the id already declared by `loop 'n' body → nodes[0]`"); + }); + + it('a region its own schema refused still has its authored ids judged — the collision is refused at parse, the malformed region stays validateControlFlow\'s', () => { + // `label` is required on every node, so this body fails `FlowRegionSchema` + // and `parseFlowNodeRegions` leaves it raw. The id the author wrote is + // still an id in the one space. + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([{ id: 'start', type: 'assignment' } as never]), + { id: 'end', type: 'end', label: 'End' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 1, 'config', 'body', 'nodes', 0, 'id']], + ]); + }); + + it('a non-object element in a raw region does not crash the parse-time walk — safeParse returns, and validateControlFlow names the malformed region instead of throwing a TypeError', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([null as never]), + { id: 'end', type: 'end', label: 'End' }, + ])); + // Today's contract, unchanged: a region that fails its own schema is left + // raw by the node transform, and refusing it is `validateControlFlow`'s. expect(result.success).toBe(true); if (!result.success) return; - expect(result.data.nodes.map((n) => n.id)).toEqual(['start', 'n', 'end']); + let caught: unknown; + try { + validateControlFlow(result.data); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + expect(caught).not.toBeInstanceOf(TypeError); + expect((caught as Error).message).toContain("loop 'n' body: invalid region"); + }); + + it('accepts a flow whose ids are unique across the whole flow, keeping every region node in authored order', () => { + const unique = flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopOver([step('sweep_first'), step('sweep_second')], [{ id: 'b1', source: 'sweep_first', target: 'sweep_second' }]), + { + id: 'p', type: 'parallel', label: 'Fan out', + config: { branches: [{ nodes: [step('left')] }, { nodes: [step('right')] }] }, + }, + { id: 'end', type: 'end', label: 'End' }, + ]); + const result = FlowSchema.safeParse(unique); + expect(result.success).toBe(true); + if (!result.success) return; + expect(collectFlowGraphs(result.data).map((g) => g.nodes.map((n) => n.id))).toEqual([ + ['start', 'n', 'p', 'end'], + ['sweep_first', 'sweep_second'], + ['left'], + ['right'], + ]); }); }); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 174bff4182..6599fc30ab 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -22,7 +22,7 @@ import { lazySchema } from '../shared/lazy-schema'; import { retiredKey } from '../shared/retired-key'; import { retryPolicyShape } from '../shared/retry-policy.zod'; import { strictObject } from '../shared/strict-object'; -import { parseFlowNodeRegions } from './control-flow.zod'; +import { collectFlowGraphs, parseFlowNodeRegions } from './control-flow.zod'; import { EndConfigSchema } from './builtin-node-config.zod'; export const FlowNodeAction = z.enum([ 'start', // Trigger @@ -927,32 +927,49 @@ export const FlowSchema = lazySchema(() => strictObject( // element and naming BOTH positions, so the formatted error points at the // one to rename. // - // Nodes (#15713): every edge's `source` / `target` names a node by id and - // the engine picks out-edges by `source`, so two top-level nodes sharing an - // id make every edge from that id ambiguous — whichever node wins is decided - // by array order, silently. Only region bodies were checked (`analyzeRegion` - // in `control-flow.zod.ts`, at `registerFlow()`); the flow's own top-level - // `nodes[]` parsed with the collision intact. This pass judges the top-level - // array ALONE: a region's nodes are judged by `analyzeRegion`, and whether the - // two spaces are one is a separate decision (#16134), not taken here. - const firstNodeIndexById = new Map(); - flow.nodes.forEach((node, index) => { - const first = firstNodeIndexById.get(node.id); - if (first === undefined) { - firstNodeIndexById.set(node.id, index); - return; - } - ctx.addIssue({ - code: 'custom', - path: ['nodes', index, 'id'], - message: - `Duplicate node id \`${node.id}\` — \`nodes[${index}]\` reuses the id already declared by ` + - `\`nodes[${first}]\`; every node id in a flow must be unique. Rename one of them: a ` + - "node id is the handle every edge's `source`/`target` resolves and a designer, a BPMN " + - 'export or a flow diff keys on, so a collision routes edges by array order silently ' + - 'rather than failing loudly.', + // Nodes (#15713, one space at every depth since #16134): every edge's + // `source` / `target` names a node by id and the engine picks out-edges by + // `source`, so two nodes sharing an id make every edge from that id ambiguous + // — whichever node wins is decided by array order, silently. A flow has ONE + // node-id space: its top-level `nodes[]` and every ADR-0031 region body + // (`loop` / `try_catch` / `parallel`, at every depth) share it, so the walk + // is `collectFlowGraphs` — the top-level graph first, then each region in + // document order, depth first — and "already declared by" is the earlier + // position in that walk, named as a top-level index (`nodes[1]`) or a region + // path (`loop 'sweep' body → nodes[0]`). This is the one refusal an author + // meets: `analyzeRegion` (at `registerFlow()`) keeps a per-region uniqueness + // invariant for direct raw-region callers, but a flow that parses never + // reaches it with a collision, and a flow with one never parses. + const firstNodeLocationById = new Map(); + for (const graph of collectFlowGraphs(flow)) { + graph.nodes.forEach((node, index) => { + // A region its own schema refused stays raw (`parseFlowNodeRegions`), so + // an element here may carry no string id at all; the region refusal in + // `validateControlFlow` owns that shape, and this rule judges only the ids + // an author actually wrote. + const id: unknown = (node as { id?: unknown } | null)?.id; + if (typeof id !== 'string') return; + const location = graph.scope ? `${graph.scope} → nodes[${index}]` : `nodes[${index}]`; + const first = firstNodeLocationById.get(id); + if (first === undefined) { + firstNodeLocationById.set(id, location); + return; + } + ctx.addIssue({ + code: 'custom', + path: [...graph.path, 'nodes', index, 'id'], + message: + `Duplicate node id \`${id}\` — \`${location}\` reuses the id already declared by ` + + `\`${first}\`; every node id in a flow must be unique. Rename one of them: a ` + + "node id is the handle every edge's `source`/`target` resolves and a designer, a BPMN " + + 'export or a flow diff keys on, so a collision routes edges by array order silently ' + + 'rather than failing loudly. The id space is one across the whole flow — the ' + + 'top-level `nodes[]` and every region body (`loop` / `try_catch` / `parallel`, at ' + + 'every depth) share it — so a region node may not reuse an id declared outside its ' + + 'region either.', + }); }); - }); + } // Edges (#14964): every reader of `edges[].id` assumes the ids are unique — // a designer, a BPMN export, a flow diff, any traversal that dedupes by id — diff --git a/packages/spec/src/automation/region-normalization.test.ts b/packages/spec/src/automation/region-normalization.test.ts index 75dd1da77e..6f5342e32d 100644 --- a/packages/spec/src/automation/region-normalization.test.ts +++ b/packages/spec/src/automation/region-normalization.test.ts @@ -228,6 +228,25 @@ describe('#4347 — collectFlowGraphs', () => { .toEqual(['', "loop 'loop' body", "loop 'loop' body → try_catch 'tc' catch"]); }); + it('carries each graph\'s key path beside its scope, so a finding can be anchored where the author wrote it (#16134)', () => { + const flow = flowWith(loopWith({ + nodes: [{ + id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', + config: { catch: gatedRegion() }, + }], + edges: [], + })); + expect(collectFlowGraphs(flow).map(g => g.path)).toEqual([ + [], + ['nodes', 1, 'config', 'body'], + ['nodes', 1, 'config', 'body', 'nodes', 0, 'config', 'catch'], + ]); + expect(collectFlowGraphs(flowWith({ + id: 'par', type: PARALLEL_NODE_TYPE, label: 'Fan', + config: { branches: [gatedRegion(), gatedRegion()] }, + })).map(g => g.path)).toEqual([[], ['nodes', 1, 'config', 'branches', 0], ['nodes', 1, 'config', 'branches', 1]]); + }); + it('terminates on a self-referential region instead of recursing forever', () => { // Hand-built flows are objects, not parsed JSON, so a cycle is reachable. const selfRegion: { nodes: unknown[]; edges: unknown[] } = { nodes: [], edges: [] }; From 38c0589cf1a0cc7f0a9033611d10a57f24e41897 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 07:44:10 +0000 Subject: [PATCH 2/5] test(spec): region-normalization fixtures carry distinct ids per region under one node-id space (#16134) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gatedRegion()` / `loopWith()` reused `gate` / `write` / `loop` in sibling branches, in try + catch, and in a loop nested three deep — call-assembled fixtures the card's literal-array census declares blind, and the first in-repo flows the one-id-space rule refuses. They pin normalization, not id reuse, so they take a suffix / an id parameter rather than pin the collision. The self-referential termination pin now reads "a bounded ZodError, never a RangeError": the self-reference makes `l` its own body node at every depth, which the rule refuses once the walk stops at the ceiling. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x --- .../automation/region-normalization.test.ts | 55 +++++++++++++------ 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/packages/spec/src/automation/region-normalization.test.ts b/packages/spec/src/automation/region-normalization.test.ts index 6f5342e32d..2d34ce7a3d 100644 --- a/packages/spec/src/automation/region-normalization.test.ts +++ b/packages/spec/src/automation/region-normalization.test.ts @@ -33,10 +33,18 @@ const ENVELOPE = { dialect: 'cel', source: CONDITION }; const gate = { id: 'gate', type: 'decision', label: 'Gate' }; const write = { id: 'write', type: 'create_record', label: 'Write' }; -/** A well-formed region whose single edge carries a BARE STRING condition. */ -const gatedRegion = () => ({ - nodes: [structuredClone(gate), structuredClone(write)], - edges: [{ id: 'b1', source: 'gate', target: 'write', type: 'conditional', condition: CONDITION }], +/** + * A well-formed region whose single edge carries a BARE STRING condition. + * + * `suffix` keeps two regions of one flow apart: a flow has ONE node-id space + * across its top-level `nodes[]` and every region (#16134), so sibling + * branches, `try` + `catch`, and a container nested in a container may not + * repeat `gate` / `write` / `loop`. These fixtures pin normalization, not id + * reuse, so they carry distinct ids rather than pin the collision. + */ +const gatedRegion = (suffix = '') => ({ + nodes: [{ ...gate, id: `gate${suffix}` }, { ...write, id: `write${suffix}` }], + edges: [{ id: `b1${suffix}`, source: `gate${suffix}`, target: `write${suffix}`, type: 'conditional', condition: CONDITION }], }); const flowWith = (containerNode: Record) => FlowSchema.parse({ @@ -53,8 +61,8 @@ const flowWith = (containerNode: Record) => FlowSchema.parse({ ], }); -const loopWith = (body: unknown) => ({ - id: 'loop', type: LOOP_NODE_TYPE, label: 'Loop', config: { collection: '{rows}', iteratorVariable: 'row', body }, +const loopWith = (body: unknown, id = 'loop') => ({ + id, type: LOOP_NODE_TYPE, label: 'Loop', config: { collection: '{rows}', iteratorVariable: 'row', body }, }); describe('#4415 — FlowSchema.parse canonicalizes regions with no second call', () => { @@ -99,7 +107,7 @@ describe('#4415 — FlowSchema.parse canonicalizes regions with no second call', it('normalizes every parallel branch and keeps the branch `name`', () => { const flow = flowWith({ id: 'par', type: PARALLEL_NODE_TYPE, label: 'Fan', - config: { branches: [{ name: 'left', ...gatedRegion() }, { name: 'right', ...gatedRegion() }] }, + config: { branches: [{ name: 'left', ...gatedRegion() }, { name: 'right', ...gatedRegion('_r') }] }, }); const branches = (flow.nodes[1]!.config as any).branches; @@ -113,7 +121,7 @@ describe('#4415 — FlowSchema.parse canonicalizes regions with no second call', it('normalizes both try_catch regions', () => { const flow = flowWith({ id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', - config: { try: gatedRegion(), catch: gatedRegion(), errorVariable: '$err' }, + config: { try: gatedRegion(), catch: gatedRegion('_c'), errorVariable: '$err' }, }); const cfg = (flow.nodes[1]!.config as any); @@ -127,7 +135,7 @@ describe('#4415 — FlowSchema.parse canonicalizes regions with no second call', const flow = flowWith(loopWith({ nodes: [{ id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', - config: { try: { nodes: [loopWith(gatedRegion())], edges: [] } }, + config: { try: { nodes: [loopWith(gatedRegion(), 'inner')], edges: [] } }, }], edges: [], })); @@ -168,10 +176,25 @@ describe('#4415 — FlowSchema.parse canonicalizes regions with no second call', config: { collection: '{r}', iteratorVariable: 'r', body: selfRegion }, }); - expect(() => FlowSchema.parse({ - name: 'cyclic', label: 'Cyclic', type: 'schedule', - nodes: selfRegion.nodes, edges: [], - })).not.toThrow(); + // #16134 — a flow has one node-id space, and the self-reference makes `l` + // its own body node at every depth, so the parse now REFUSES it; the + // termination pin therefore reads "a bounded ZodError, never a RangeError": + // the walk reached the depth ceiling and stopped. + let caught: unknown; + try { + FlowSchema.parse({ + name: 'cyclic', label: 'Cyclic', type: 'schedule', + nodes: selfRegion.nodes, edges: [], + }); + } catch (error) { + caught = error; + } + expect(caught).not.toBeInstanceOf(RangeError); + const issues = (caught as { issues?: Array<{ code: string; message: string }> })?.issues; + expect(issues).toBeDefined(); + expect(issues!.length).toBeGreaterThan(0); + expect(issues!.length).toBeLessThan(64); + expect(issues!.every(i => i.code === 'custom' && i.message.startsWith('Duplicate node id `l`'))).toBe(true); }); it('is copy-on-write at the node level', () => { @@ -207,12 +230,12 @@ describe('#4347 — collectFlowGraphs', () => { it('names each parallel branch and both try_catch regions', () => { expect(collectFlowGraphs(flowWith({ id: 'par', type: PARALLEL_NODE_TYPE, label: 'Fan', - config: { branches: [gatedRegion(), gatedRegion()] }, + config: { branches: [gatedRegion(), gatedRegion('_r')] }, })).map(g => g.scope)).toEqual(['', "parallel 'par' branch 0", "parallel 'par' branch 1"]); expect(collectFlowGraphs(flowWith({ id: 'tc', type: TRY_CATCH_NODE_TYPE, label: 'Guard', - config: { try: gatedRegion(), catch: gatedRegion() }, + config: { try: gatedRegion(), catch: gatedRegion('_c') }, })).map(g => g.scope)).toEqual(['', "try_catch 'tc' try", "try_catch 'tc' catch"]); }); @@ -243,7 +266,7 @@ describe('#4347 — collectFlowGraphs', () => { ]); expect(collectFlowGraphs(flowWith({ id: 'par', type: PARALLEL_NODE_TYPE, label: 'Fan', - config: { branches: [gatedRegion(), gatedRegion()] }, + config: { branches: [gatedRegion(), gatedRegion('_r')] }, })).map(g => g.path)).toEqual([[], ['nodes', 1, 'config', 'branches', 0], ['nodes', 1, 'config', 'branches', 1]]); }); From c07ea3249e3e8d265df740776f65cb82979029bd Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 08:42:49 +0000 Subject: [PATCH 3/5] docs(automation): the flow authoring guide states the one node-id space (#16134) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flows.mdx` said `id` was a "Unique node identifier" with no scope and called a region "self-contained" with no id caveat — the two places an author would now meet the parse refusal without warning. Both now say node ids are one space across the top-level `nodes[]` and every region body, refused at parse. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x --- content/docs/automation/flows.mdx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index d81206d9b0..965baa3de9 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -134,7 +134,7 @@ Each node performs a specific action in the flow. | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| `id` | `string` | ✅ | Unique node identifier | +| `id` | `string` | ✅ | Unique node identifier — unique across the **whole flow**: the top-level `nodes[]` and every region body (`loop.body`, `parallel.branches[]`, `try_catch.try` / `.catch`, at any depth) share one id space, and `FlowSchema` refuses a reused id at parse (`Duplicate node id …`, naming both locations) | | `type` | `string` | ✅ | Node type — a built-in id from the table above **or** a plugin-registered one. Per ADR-0018 the spec does not gate this with a closed enum; it is checked against the live action registry once that registry is complete — plugins contribute node types while they start, so flows registered during boot are checked in one pass when the vocabulary closes (all plugins started), and anything registered after that (Studio publish, dev reload) is checked immediately. Unknown types warn, never reject; executing one fails with `NO_EXECUTOR` | | `label` | `string` | ✅ | Display label | | `config` | `object` | optional | Type-specific configuration — the registered executor's `configSchema` owns its shape. Keys that schema does not declare are rejected at `registerFlow()`, and the built-in executors `parse()` the value against their Zod contract before running (#4277) | @@ -496,7 +496,11 @@ a malformed construct is rejected before the flow can run. A region runs in the **enclosing variable scope** (the iterator value and any body mutations are visible to the surrounding flow) — it is *not* a separate `subflow` invocation. The container node's ordinary out-edges are the -"after-loop / after-block" continuation. +"after-loop / after-block" continuation. "Self-contained" describes control +flow and variable scope, not ids: node ids are **one space across the whole +flow**, so a region node may not reuse an id declared at the top level or in +any other region — the collision is refused at parse (see `id` under +[Node Structure](#node-structure)). ### Loop container From 2baa5d480b9f7fd928d7df51b7dfdefbae1e94b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 09:06:18 +0000 Subject: [PATCH 4/5] =?UTF-8?q?docs(spec):=20the=20parse=20refuses=20a=20c?= =?UTF-8?q?ollision=20at=20every=20depth=20it=20walks,=20not=20at=20every?= =?UTF-8?q?=20depth=20=E2=80=94=20and=20pin=20the=20seam=20(#16134)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Contract review measured the claim "a flow that parses never reaches analyzeRegion with a collision" as one nesting level too strong: collectFlowGraphs stops at MAX_REGION_DEPTH (32), so at nesting 33 the region is left raw, the parse accepts, and validateControlFlow refuses a within-region duplicate in analyzeRegion's own line — the base tree behaves the same there. Reword the two code comments, the changeset paragraph that ships in CHANGELOG.md, and pin the seam: nesting 32 refused at parse, nesting 33 accepted by the parse and refused by validateControlFlow, unique ids at 33 accepted end to end. analyzeRegion's line is kept on purpose — past the ceiling it is the only refusal of a within-region duplicate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x --- .changeset/flow-one-node-id-space.md | 21 ++++--- .../spec/src/automation/control-flow.zod.ts | 13 +++-- packages/spec/src/automation/flow.test.ts | 56 ++++++++++++++++++- packages/spec/src/automation/flow.zod.ts | 9 ++- 4 files changed, 82 insertions(+), 17 deletions(-) diff --git a/.changeset/flow-one-node-id-space.md b/.changeset/flow-one-node-id-space.md index 180bd0b3f7..d02eb9369e 100644 --- a/.changeset/flow-one-node-id-space.md +++ b/.changeset/flow-one-node-id-space.md @@ -4,12 +4,13 @@ feat(spec)!: `FlowSchema` refuses a region node whose id is already declared elsewhere in the flow — one node-id space across the top-level `nodes[]` and every region body (#16134) - + **BREAKING** accept-set narrowing on `FlowSchema` — a flow has **one node-id space**. A node inside an ADR-0031 region body (`loop.config.body`, `try_catch.config.try` / `.catch`, each `parallel.config.branches[]`, nested to -any depth) whose `id` is already declared by a top-level node, or by a node in +any depth the parse walks — up to `MAX_REGION_DEPTH` = 32 levels) whose `id` is +already declared by a top-level node, or by a node in any other region of the same flow, is now **refused at parse time** — by `FlowSchema.parse` / `safeParse`, `defineFlow`, and every door that validates a flow through the schema (`objectstack validate`, the runtime publish gate, a @@ -30,9 +31,10 @@ checkpoint's `completedNodeIds` all key on the bare id, so such a collision was silently wrong wherever a flow is flattened. **What changes** (`packages/spec/src/automation/flow.zod.ts`): the existing -`superRefine` pass over `nodes[]` now walks every graph in the flow via +`superRefine` pass over `nodes[]` now walks every graph the parse reaches via `collectFlowGraphs` — the top-level graph first, then each region in document -order, depth first — keeping one map of first declarations. A later occurrence +order, depth first, down to `MAX_REGION_DEPTH` (32) — keeping one map of first +declarations. A later occurrence raises the same single `custom` issue as before, anchored at the later node's own `id` (inside the region, e.g. `nodes.1.config.body.nodes.0.id`) and naming both locations — a top-level index (`nodes[1]`) or a region path @@ -42,10 +44,13 @@ both locations — a top-level index (`nodes[1]`) or a region path ✗ nodes.1.config.body.nodes.0.id: Duplicate node id `start` — `loop 'n' body → nodes[0]` reuses the id already declared by `nodes[0]`; every node id in a flow must be unique. Rename one of them: … ``` -One refusal, one message shape: an author never sees two issues for one -collision. `analyzeRegion` keeps its per-region uniqueness line as an invariant -for direct raw-region callers (`bpmn-mapping`), but a flow that parses never -reaches it with a collision, and a flow with one never parses. +One refusal, one message shape, at every depth the parse walks: within +`MAX_REGION_DEPTH` an author never sees two issues for one collision. A region +nested beyond that ceiling is left raw by the parse and stays +`validateControlFlow`'s, in its own line — there `analyzeRegion`'s +`duplicate node id 'X'` is the only refusal of a within-region duplicate (a +cross-region collision past the ceiling is not judged), and the same line +guards `bpmn-mapping`'s raw-region caller, so it is kept on purpose. `collectFlowGraphs` gains a `path` field beside `scope` — the same location as a key path — so the issue can be anchored where the author wrote the node; it also now skips a non-object element in a region its own schema refused (such a diff --git a/packages/spec/src/automation/control-flow.zod.ts b/packages/spec/src/automation/control-flow.zod.ts index dbe74ab979..7a0539c415 100644 --- a/packages/spec/src/automation/control-flow.zod.ts +++ b/packages/spec/src/automation/control-flow.zod.ts @@ -407,10 +407,15 @@ export function analyzeRegion(region: { nodes: FlowNodeParsed[]; edges?: FlowEdg } // Unique ids — an invariant this analysis needs (the degree maps below key - // on id), not the author-facing rule. A flow has ONE node-id space, judged - // by `FlowSchema` at parse over every depth (#16134), so a parsed flow never - // arrives here carrying a collision and no author sees this line; it guards - // direct callers that hand in a raw region (`bpmn-mapping`). + // on id), and the author-facing rule's last line of defence. A flow has ONE + // node-id space, judged by `FlowSchema` at parse over every depth + // `collectFlowGraphs` walks — nesting up to `MAX_REGION_DEPTH` (#16134) — so + // within that ceiling a parsed flow never arrives here carrying a collision. + // Beyond it a region is left raw and reaches this line through + // `validateControlFlow`, where this is the ONLY refusal of a within-region + // duplicate: delete it and the degree maps would silently de-duplicate the + // collision instead. It also guards direct callers that hand in a raw region + // (`bpmn-mapping`). const ids = new Set(); for (const n of nodes) { if (ids.has(n.id)) errors.push(`duplicate node id '${n.id}'`); diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 48776db0b4..6878097663 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -2156,7 +2156,9 @@ describe('FlowSchema — one node-id space across the top-level nodes[] and ever // 「同意」): top-level `nodes[]` and every region body (`loop` / `try_catch` / // `parallel`, at every depth) share ONE id space; a collision is refused at // parse, by the rule that already refused top-level duplicates, in its one - // message shape, naming both locations. "Declared by" is the earlier + // message shape, naming both locations — at every depth the walk reaches: + // `collectFlowGraphs` stops at `MAX_REGION_DEPTH` (32), and that seam is + // pinned last, so it moves deliberately. "Declared by" is the earlier // position in the `collectFlowGraphs` walk — the top-level graph first, then // each region in document order, depth first — so the top-level array is // always the first declaration and a region node is the one that moves. @@ -2235,7 +2237,7 @@ describe('FlowSchema — one node-id space across the top-level nodes[] and ever ); }); - it('walks every depth — a try_catch catch-region node nested inside a loop body that reuses a top-level id is refused with the chained region path', () => { + it('walks nested depth — a try_catch catch-region node nested inside a loop body that reuses a top-level id is refused with the chained region path', () => { const result = FlowSchema.safeParse(flowWith([ { id: 'start', type: 'start', label: 'Start' }, loopOver([{ @@ -2319,6 +2321,56 @@ describe('FlowSchema — one node-id space across the top-level nodes[] and ever expect((caught as Error).message).toContain("loop 'n' body: invalid region"); }); + // The seam, pinned so it moves deliberately (as #15713's boundary did): the + // parse walk judges nesting 0..MAX_REGION_DEPTH (32). One level further the + // region is left raw, `safeParse` succeeds, and the within-region duplicate is + // `validateControlFlow`'s — `analyzeRegion`'s own line, its own shape. + const loopsNestedTo = (nesting: number, innermost: FlowNode[]): FlowNode => { + // Outermost loop is `n` (the edges above point at it), inner ones `l1..`; + // `l${k}` sits at nesting k and its body is nesting k + 1. + let body: { nodes: FlowNode[]; edges: FlowEdge[] } = { nodes: innermost, edges: [] }; + for (let k = nesting - 1; k >= 1; k--) { + body = { nodes: [{ id: `l${k}`, type: 'loop', label: `L${k}`, config: { collection: '{items}', body } }], edges: [] }; + } + return { id: 'n', type: 'loop', label: 'Loop', config: { collection: '{items}', body } }; + }; + const roundTripped = (nesting: number, innermost: FlowNode[]): Flow => JSON.parse(JSON.stringify(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + loopsNestedTo(nesting, innermost), + { id: 'end', type: 'end', label: 'End' }, + ]))); + + it('the seam at MAX_REGION_DEPTH: a within-region duplicate at nesting 32 is refused at parse; at nesting 33 the parse accepts and validateControlFlow refuses it in analyzeRegion\'s own line', () => { + const dup = [step('dup', 'Dup A'), step('dup', 'Dup B')]; + + const atCeiling = FlowSchema.safeParse(roundTripped(32, dup)); + expect(atCeiling.success).toBe(false); + if (atCeiling.success) return; + expect(atCeiling.error.issues).toHaveLength(1); + expect(atCeiling.error.issues[0].message).toContain('Duplicate node id `dup`'); + expect(atCeiling.error.issues[0].path.slice(-3)).toEqual(['nodes', 1, 'id']); + + const pastCeiling = FlowSchema.safeParse(roundTripped(33, dup)); + expect(pastCeiling.success).toBe(true); + if (!pastCeiling.success) return; + let caught: unknown; + try { + validateControlFlow(pastCeiling.data); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + expect(caught).not.toBeInstanceOf(TypeError); + expect((caught as Error).message).toContain("loop 'l32' body: duplicate node id 'dup'"); + expect((caught as Error).message).not.toContain('Duplicate node id'); + + // Control: the same nesting with unique ids is accepted end to end. + const unique = FlowSchema.safeParse(roundTripped(33, [step('u1'), step('u2')])); + expect(unique.success).toBe(true); + if (!unique.success) return; + expect(() => validateControlFlow(unique.data)).not.toThrow(); + }); + it('accepts a flow whose ids are unique across the whole flow, keeping every region node in authored order', () => { const unique = flowWith([ { id: 'start', type: 'start', label: 'Start' }, diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 6599fc30ab..b4fb9995ef 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -937,9 +937,12 @@ export const FlowSchema = lazySchema(() => strictObject( // document order, depth first — and "already declared by" is the earlier // position in that walk, named as a top-level index (`nodes[1]`) or a region // path (`loop 'sweep' body → nodes[0]`). This is the one refusal an author - // meets: `analyzeRegion` (at `registerFlow()`) keeps a per-region uniqueness - // invariant for direct raw-region callers, but a flow that parses never - // reaches it with a collision, and a flow with one never parses. + // meets at every depth the walk reaches: `collectFlowGraphs` stops at + // `MAX_REGION_DEPTH` (the ceiling `parseFlowNodeRegions` shares), so a region + // nested beyond it is left raw and stays `validateControlFlow`'s — there + // `analyzeRegion`'s own `duplicate node id` line is the only refusal of a + // within-region duplicate (a cross-region collision past the ceiling is not + // judged), and it also guards `bpmn-mapping`'s raw-region caller. Kept. const firstNodeLocationById = new Map(); for (const graph of collectFlowGraphs(flow)) { graph.nodes.forEach((node, index) => { From 64c41f20c80b6d8cbe93419b088071cf3e311466 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 09:08:53 +0000 Subject: [PATCH 5/5] test(spec): the seam pin's control region is single-entry / single-exit (#16134) The unique-id control at nesting 33 carried two unconnected nodes, which analyzeRegion refuses for a different reason (two entries, two exits); chain them so only the ids differ from the duplicate leg. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016N6xmWt5hYm94ffVEwGH8x --- packages/spec/src/automation/flow.test.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 6878097663..a1f5dc97bb 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -2325,18 +2325,18 @@ describe('FlowSchema — one node-id space across the top-level nodes[] and ever // parse walk judges nesting 0..MAX_REGION_DEPTH (32). One level further the // region is left raw, `safeParse` succeeds, and the within-region duplicate is // `validateControlFlow`'s — `analyzeRegion`'s own line, its own shape. - const loopsNestedTo = (nesting: number, innermost: FlowNode[]): FlowNode => { + const loopsNestedTo = (nesting: number, innermost: FlowNode[], innermostEdges: FlowEdge[] = []): FlowNode => { // Outermost loop is `n` (the edges above point at it), inner ones `l1..`; // `l${k}` sits at nesting k and its body is nesting k + 1. - let body: { nodes: FlowNode[]; edges: FlowEdge[] } = { nodes: innermost, edges: [] }; + let body: { nodes: FlowNode[]; edges: FlowEdge[] } = { nodes: innermost, edges: innermostEdges }; for (let k = nesting - 1; k >= 1; k--) { body = { nodes: [{ id: `l${k}`, type: 'loop', label: `L${k}`, config: { collection: '{items}', body } }], edges: [] }; } return { id: 'n', type: 'loop', label: 'Loop', config: { collection: '{items}', body } }; }; - const roundTripped = (nesting: number, innermost: FlowNode[]): Flow => JSON.parse(JSON.stringify(flowWith([ + const roundTripped = (nesting: number, innermost: FlowNode[], innermostEdges: FlowEdge[] = []): Flow => JSON.parse(JSON.stringify(flowWith([ { id: 'start', type: 'start', label: 'Start' }, - loopsNestedTo(nesting, innermost), + loopsNestedTo(nesting, innermost, innermostEdges), { id: 'end', type: 'end', label: 'End' }, ]))); @@ -2364,8 +2364,9 @@ describe('FlowSchema — one node-id space across the top-level nodes[] and ever expect((caught as Error).message).toContain("loop 'l32' body: duplicate node id 'dup'"); expect((caught as Error).message).not.toContain('Duplicate node id'); - // Control: the same nesting with unique ids is accepted end to end. - const unique = FlowSchema.safeParse(roundTripped(33, [step('u1'), step('u2')])); + // Control: the same nesting with unique ids — chained, so the region is + // single-entry / single-exit and only the ids differ — is accepted end to end. + const unique = FlowSchema.safeParse(roundTripped(33, [step('u1'), step('u2')], [{ id: 'ue', source: 'u1', target: 'u2' }])); expect(unique.success).toBe(true); if (!unique.success) return; expect(() => validateControlFlow(unique.data)).not.toThrow();