diff --git a/.changeset/flow-node-id-uniqueness.md b/.changeset/flow-node-id-uniqueness.md new file mode 100644 index 0000000000..16d7a003db --- /dev/null +++ b/.changeset/flow-node-id-uniqueness.md @@ -0,0 +1,75 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec)!: `FlowSchema` refuses a flow whose top-level `nodes[]` declares the same id twice (#15713) + + + +**BREAKING** accept-set narrowing on `FlowSchema` — a flow whose top-level +`nodes[]` carries two nodes with the same `id` 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. The +exact parallel of #14964 (edge ids, maintainer ruling 2026-09-05, option A — +an `error`, not a `warning`; no opt-out, no transition window), applied to the +other hand-authored id space in the same schema. + +Every edge's `source` / `target` names a node by id, and the engine's traversal +picks out-edges by `source` — with two nodes sharing an id, every edge from +that id is ambiguous and whichever node wins is decided by array order, +silently. A designer, a BPMN export and a flow diff key on node ids the same +way they key on edge ids. Only region bodies (`loop` / `try_catch` / `parallel` +sub-graphs) were checked, by `analyzeRegion` at `registerFlow()`; the flow's +own top-level `nodes[]` parsed with the collision intact — measured on +`origin/main` `1f2a02ba` with two lit controls on the same schema instance (a +node missing its `label` → refused at `nodes.1.label`; an unknown key on a node +→ `unrecognized_keys`). + +**What changes** (`packages/spec/src/automation/flow.zod.ts`): the existing +`superRefine` on `FlowSchema` gains a pass over the top-level `nodes[]`, the +same shape as the `edges[]` pass. Each later occurrence of an already-declared +id raises one `custom` issue, anchored at `nodes[N].id` of the *later* node and +naming both positions, so the formatted error points at the node to rename: + +```text +✗ nodes.2.id: Duplicate node id `n` — `nodes[2]` reuses the id already declared by `nodes[1]`; every node id in a flow must be unique. Rename one of them: … +``` + +**What does NOT change:** `nodes[].id` keeps its name, type and describe; the +open node-type vocabulary (ADR-0018), the region rules (`analyzeRegion`) and +every other refusal are untouched; a flow with unique top-level node ids +parses exactly as before. The rule judges the flow's **own top-level** +`nodes[]` only — a region body's nodes remain `analyzeRegion`'s to judge, and +whether a region node may reuse a top-level node id (one id space or two) is a +separate decision (#16134) this change neither takes nor pre-empts. + +The shape that is refused, and what the author does about it — a four-node +excerpt, the later node renamed and its edge re-pointed: + +```ts +// before — parsed on green, two nodes keyed 'n' +nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'n', type: 'assignment', label: 'Assign A' }, + { id: 'n', type: 'assignment', label: 'Assign B' }, + { id: 'end', type: 'end', label: 'End' }, +] + +// after — refused at parse (nodes.2.id: Duplicate node id `n` …); rename the later one +// and point the edges that meant it at the new id: +nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'n', type: 'assignment', label: 'Assign A' }, + { id: 'n2', type: 'assignment', label: 'Assign B' }, + { id: 'end', type: 'end', label: 'End' }, +] +``` + +**Remedy.** Rename the later node to an id no other top-level node in that +flow carries, then re-point at the new id the edges whose `source` / `target` +meant that node; 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 duplicate top-level node id. diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 03a4e9b19d..05cd06b1c5 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -1981,3 +1981,169 @@ describe('FlowSchema — edge ids are unique (#14964)', () => { expect(issues![0].message).toContain('Duplicate edge id `dup`'); }); }); + +describe('FlowSchema — top-level node ids are unique (#15713)', () => { + // The card's probe, reproduced: a four-node flow with `nodes[1].id === + // nodes[2].id === 'n'`, parsed on green (`origin/main` 1f2a02ba re-measured + // before this rule landed). The two controls beside it — a node missing its + // required `label`, and an unknown key on a node, on the SAME schema instance + // — are what proved the acceptance was a missing rule rather than a disabled + // validator, so both are pinned here too. An invalid node `type` is NOT a + // control on this schema: the node-type vocabulary is open by contract + // (ADR-0018), so that lever never fires — recorded so nobody reuses it. + const edges: Flow['edges'] = [ + { id: 'e1', source: 'start', target: 'n' }, + { id: 'e2', source: 'n', target: 'end' }, + ]; + const flowWith = (nodes: Flow['nodes']): Flow => ({ + name: 'node_id_flow', + label: 'Node id flow', + type: 'autolaunched', + nodes, + edges, + }); + const duplicate = flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'n', type: 'assignment', label: 'Assign A' }, + { id: 'n', type: 'assignment', label: 'Assign B' }, + { id: 'end', type: 'end', label: 'End' }, + ]); + + it('refuses a flow whose top-level nodes[] declares one id twice — the issue names the id and BOTH positions, anchored on the later node', () => { + const result = FlowSchema.safeParse(duplicate); + 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', 2, 'id']); + expect(issue.message).toContain('Duplicate node id `n`'); + expect(issue.message).toContain('`nodes[2]` reuses the id already declared by `nodes[1]`'); + }); + + it('renders through formatZodError as a line that points at the node to rename', () => { + const result = FlowSchema.safeParse(duplicate); + 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.2.id: Duplicate node id `n` — `nodes[2]` reuses the id already declared by `nodes[1]`', + ); + }); + + it('raises one issue per later occurrence, each naming the FIRST declaration of that id', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'a', type: 'start', label: 'Start' }, + { id: 'b', type: 'assignment', label: 'B' }, + { id: 'a', type: 'assignment', label: 'A again' }, + { id: 'b', type: 'assignment', label: 'B again' }, + { id: 'a', type: 'end', label: 'A once more' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => i.path)).toEqual([ + ['nodes', 2, 'id'], + ['nodes', 3, 'id'], + ['nodes', 4, 'id'], + ]); + expect(result.error.issues[0].message).toContain('`nodes[2]` reuses the id already declared by `nodes[0]`'); + expect(result.error.issues[1].message).toContain('`nodes[3]` reuses the id already declared by `nodes[1]`'); + expect(result.error.issues[2].message).toContain('`nodes[4]` reuses the id already declared by `nodes[0]`'); + }); + + it('a duplicate node id and a duplicate edge id in one flow raise one issue each, nodes first, same shape', () => { + const result = FlowSchema.safeParse({ + ...duplicate, + edges: [ + { id: 'dup', source: 'start', target: 'n' }, + { id: 'dup', source: 'n', target: 'end' }, + ], + }); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 2, 'id']], + ['custom', ['edges', 1, 'id']], + ]); + expect(result.error.issues[0].message).toMatch(/^Duplicate node id `n` — `nodes\[2\]` reuses the id already declared by `nodes\[1\]`; every node id in a flow must be unique\. /); + expect(result.error.issues[1].message).toMatch(/^Duplicate edge id `dup` — `edges\[1\]` reuses the id already declared by `edges\[0\]`; every edge id in a flow must be unique\. /); + }); + + it('still refuses card control A — a node missing its required `label` — so the refusal above is not a vacuous pass', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'n', 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([['invalid_type', ['nodes', 1, 'label']]]); + expect(result.error.issues.some((i) => i.message.includes('Duplicate node id'))).toBe(false); + }); + + it('still refuses card control B — an unknown key on a node — with `unrecognized_keys`', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'n', type: 'assignment', label: 'Assign', bogusKey: 1 } 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)).toEqual(['unrecognized_keys']); + expect(result.error.issues.some((i) => i.message.includes('Duplicate node id'))).toBe(false); + }); + + it('accepts the same flow once the ids are unique, keeping the nodes in authored order', () => { + const unique = flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'n', type: 'assignment', label: 'Assign A' }, + { id: 'n2', type: 'assignment', label: 'Assign B' }, + { id: 'end', type: 'end', label: 'End' }, + ]); + const result = FlowSchema.safeParse(unique); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.data.nodes.map((n) => n.id)).toEqual(['start', 'n', 'n2', 'end']); + expect(defineFlow(unique).nodes.map((n) => n.id)).toEqual(['start', 'n', 'n2', 'end']); + }); + + it('defineFlow refuses the duplicate with the same anchored issue', () => { + let caught: unknown; + try { + defineFlow(duplicate); + } catch (error) { + caught = error; + } + const issues = (caught as { issues?: Array<{ code: string; path: PropertyKey[]; message: string }> })?.issues; + expect(issues).toBeDefined(); + expect(issues!.map((i) => [i.code, i.path])).toEqual([['custom', ['nodes', 2, 'id']]]); + 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', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'n', type: 'loop', label: 'Loop', + config: { + collection: '{items}', + body: { + nodes: [{ id: 'start', type: 'assignment', label: 'Body step (reuses a top-level id)' }], + }, + }, + }, + { id: 'end', type: 'end', label: 'End' }, + ])); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.data.nodes.map((n) => n.id)).toEqual(['start', 'n', 'end']); + }); +}); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index f508660909..174bff4182 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -922,15 +922,44 @@ export const FlowSchema = lazySchema(() => strictObject( ...MetadataProtectionFields, }).superRefine((flow, ctx) => { - // Every reader of `edges[].id` assumes the ids are unique — a designer, a - // BPMN export, a flow diff, any traversal that dedupes by id — while nothing - // enforced it: two edges carrying one id parsed, shipped through green CI - // twice, and were inert only because the engine keys out-edges by `source` - // (#14964). The id space is hand-authored, so the next author picking a - // "free" id from the sequence cannot tell it is taken. Refuse the collision - // here, at parse time, naming the id and BOTH positions; the issue is - // anchored on the later occurrence so the formatted error points at the - // edge to renumber. + // Two hand-authored id spaces, one rule each, one shape — a later occurrence + // of an already-declared id raises one `custom` issue anchored on the LATER + // 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.', + }); + }); + + // 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 — + // while nothing enforced it: two edges carrying one id parsed, shipped + // through green CI twice, and were inert only because the engine keys + // out-edges by `source`. The id space is hand-authored, so the next author + // picking a "free" id from the sequence cannot tell it is taken. const firstIndexById = new Map(); flow.edges.forEach((edge, index) => { const first = firstIndexById.get(edge.id);