From d8f9861dc9d271ddfd94f83c739fabed1b9d565a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 02:47:42 +0000 Subject: [PATCH 1/2] feat(spec)!: FlowSchema refuses a flow whose top-level nodes[] declares the same id twice The existing superRefine on FlowSchema gains a pass over the flow's own top-level nodes[], the same shape as the edges[] pass: a later occurrence of an already-declared id raises one custom issue anchored on the LATER node, naming the id and both positions, so the formatted error points at the node to rename. Region bodies stay analyzeRegion's to judge; whether the two id spaces are one is not decided here. Pins: the duplicate refused (issue shape, formatZodError line, one issue per later occurrence naming the first declaration), both card controls (missing label, unknown key) still refused with no duplicate issue beside them, a unique-id flow accepted in authored order, defineFlow refuses, node+edge duplicates in one flow raise one issue each in the same shape, and the scope boundary (a region node reusing a top-level id is outside this rule). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T6HeZvT9wdSJD1ZxJb5Eno --- .changeset/flow-node-id-uniqueness.md | 75 ++++++++++ packages/spec/src/automation/flow.test.ts | 165 ++++++++++++++++++++++ packages/spec/src/automation/flow.zod.ts | 47 ++++-- 3 files changed, 278 insertions(+), 9 deletions(-) create mode 100644 .changeset/flow-node-id-uniqueness.md diff --git a/.changeset/flow-node-id-uniqueness.md b/.changeset/flow-node-id-uniqueness.md new file mode 100644 index 0000000000..d932b00398 --- /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 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..743e884730 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -1981,3 +1981,168 @@ 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 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..920d8210cb 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, 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); From e9d66a1ce3a39ce2276a92248e052de67a9f1358 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 02:57:14 +0000 Subject: [PATCH 2/2] docs(spec): name the deferred id-space decision (#16134) at the rule's scope boundary Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T6HeZvT9wdSJD1ZxJb5Eno --- .changeset/flow-node-id-uniqueness.md | 2 +- packages/spec/src/automation/flow.test.ts | 7 ++++--- packages/spec/src/automation/flow.zod.ts | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.changeset/flow-node-id-uniqueness.md b/.changeset/flow-node-id-uniqueness.md index d932b00398..16d7a003db 100644 --- a/.changeset/flow-node-id-uniqueness.md +++ b/.changeset/flow-node-id-uniqueness.md @@ -43,7 +43,7 @@ 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 this change neither takes nor pre-empts. +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: diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 743e884730..05cd06b1c5 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -2124,9 +2124,10 @@ describe('FlowSchema — top-level node ids are unique (#15713)', () => { // 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 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. + // 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' }, diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 920d8210cb..174bff4182 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -934,7 +934,7 @@ export const FlowSchema = lazySchema(() => strictObject( // 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, not taken here. + // 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);