diff --git a/.changeset/flow-edge-id-uniqueness.md b/.changeset/flow-edge-id-uniqueness.md new file mode 100644 index 0000000000..a69e7ad3c7 --- /dev/null +++ b/.changeset/flow-edge-id-uniqueness.md @@ -0,0 +1,64 @@ +--- +"@objectstack/spec": minor +--- + +feat(spec)!: `FlowSchema` refuses a flow whose `edges[]` declares the same id twice (#14964) + + + +**BREAKING** accept-set narrowing on `FlowSchema` — a flow whose `edges[]` +carries two edges 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. Maintainer ruling +2026-09-05 on #14964 (director decision batch #40, verbatim 「同意」): option +A — an `error`, not a `warning`; no opt-out, no transition window. + +Every reader of an edge id assumes the ids in a flow are unique — a designer, +a BPMN export, a flow diff, any traversal that dedupes by id — and nothing +enforced it. A real duplicate (`id: 'e20'` on two edges of one flow) shipped +through two releases of green CI in a downstream app and was inert only +because the engine keys out-edges by `source`, never by `id`: the collision is +invisible until something keys on ids, and then silently wrong rather than +loudly broken. The id space is hand-authored, so the next author picking a +"free" id from the sequence had no way to know it was taken. + +**What changes** (`packages/spec/src/automation/flow.zod.ts`): a `superRefine` +on the flow's `edges[]`. Each later occurrence of an already-declared id raises +one `custom` issue, anchored at `edges[N].id` of the *later* edge and naming +both positions, so the formatted error points at the edge to renumber: + +```text +✗ edges.7.id: Duplicate edge id `e20` — `edges[7]` reuses the id already declared by `edges[3]`; every edge id in a flow must be unique. Renumber one of them: … +``` + +**What does NOT change:** `edges[].id` keeps its name, type and describe; the +node vocabulary, the edge `type` enum and every other refusal are untouched; +a flow with unique edge ids (or no edges) parses exactly as before. Node ids +are not covered by this change. + +The shape that is refused, and what the author does about it — a two-edge +excerpt, the later edge renumbered: + +```ts +// before — parsed on green, both edges keyed 'e20' +edges: [ + { id: 'e20', source: 'qualify', target: 'convert' }, + { id: 'e20', source: 'convert', target: 'end' }, +] + +// after — refused at parse (edges.1.id: Duplicate edge id `e20` …); renumber the later one: +edges: [ + { id: 'e20', source: 'qualify', target: 'convert' }, + { id: 'e21', source: 'convert', target: 'end' }, +] +``` + +**Remedy.** Renumber the later edge to an id no other edge in that flow +carries; 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 edge id, and the pinned objectui tree +carries none in its authored flows. The one known downstream instance was +renumbered before this change (hotcrm PR #1571). diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index 485bd0a60f..03a4e9b19d 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -28,6 +28,7 @@ import { // 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 { formatZodError } from '../shared/error-map.zod'; describe('FlowNodeAction', () => { it('should accept all node action types', () => { @@ -1876,3 +1877,107 @@ describe('unknown keys are rejected, not stripped (#4001)', () => { }); }); }); + +describe('FlowSchema — edge ids are unique (#14964)', () => { + // The card's probe, reproduced: two edges differing only in source/target, + // both `id: 'dup'`, parsed on green through 17.2.0. The control beside it — + // an invalid edge `type` on the SAME schema instance — is what proves the + // acceptance was a missing rule rather than a disabled validator, so it is + // pinned here too: a refactor that silences the whole edge branch would + // otherwise read as "the duplicate is still refused". + const nodes: Flow['nodes'] = [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'assign', type: 'assignment', label: 'Assign' }, + { id: 'end', type: 'end', label: 'End' }, + ]; + const flowWith = (edges: Flow['edges']): Flow => ({ + name: 'edge_id_flow', + label: 'Edge id flow', + type: 'autolaunched', + nodes, + edges, + }); + const duplicate = flowWith([ + { id: 'dup', source: 'start', target: 'assign' }, + { id: 'dup', source: 'assign', target: 'end' }, + ]); + + it('refuses a flow whose edges[] declares one id twice — the issue names the id and BOTH positions, anchored on the later edge', () => { + 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(['edges', 1, 'id']); + expect(issue.message).toContain('Duplicate edge id `dup`'); + expect(issue.message).toContain('`edges[1]` reuses the id already declared by `edges[0]`'); + }); + + it('renders through formatZodError as a line that points at the edge to renumber', () => { + 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( + '✗ edges.1.id: Duplicate edge id `dup` — `edges[1]` reuses the id already declared by `edges[0]`', + ); + }); + + it('raises one issue per later occurrence, each naming the FIRST declaration of that id', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'a', source: 'start', target: 'assign' }, + { id: 'b', source: 'start', target: 'end' }, + { id: 'a', source: 'assign', target: 'end' }, + { id: 'b', source: 'assign', target: 'end', type: 'fault' }, + { id: 'a', source: 'start', target: 'end', type: 'back' }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => i.path)).toEqual([ + ['edges', 2, 'id'], + ['edges', 3, 'id'], + ['edges', 4, 'id'], + ]); + expect(result.error.issues[0].message).toContain('`edges[2]` reuses the id already declared by `edges[0]`'); + expect(result.error.issues[1].message).toContain('`edges[3]` reuses the id already declared by `edges[1]`'); + expect(result.error.issues[2].message).toContain('`edges[4]` reuses the id already declared by `edges[0]`'); + }); + + it('still refuses the card control — an invalid edge `type` — so the refusal above is not a vacuous pass', () => { + const result = FlowSchema.safeParse(flowWith([ + { id: 'e1', source: 'start', target: 'assign' }, + { id: 'e2', source: 'assign', target: 'end', type: 'bogus' as never }, + ])); + expect(result.success).toBe(false); + if (result.success) return; + expect(result.error.issues.map((i) => [i.code, i.path])).toEqual([['invalid_value', ['edges', 1, 'type']]]); + expect(result.error.issues.some((i) => i.message.includes('Duplicate edge id'))).toBe(false); + }); + + it('accepts the same flow once the ids are unique, keeping the ids in authored order', () => { + const unique = flowWith([ + { id: 'e1', source: 'start', target: 'assign' }, + { id: 'e2', source: 'assign', target: 'end' }, + ]); + const result = FlowSchema.safeParse(unique); + expect(result.success).toBe(true); + if (!result.success) return; + expect(result.data.edges.map((e) => e.id)).toEqual(['e1', 'e2']); + expect(defineFlow(unique).edges.map((e) => e.id)).toEqual(['e1', 'e2']); + }); + + 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', ['edges', 1, 'id']]]); + expect(issues![0].message).toContain('Duplicate edge id `dup`'); + }); +}); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index abc5dc708f..a0d5cf2688 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -880,6 +880,33 @@ export const FlowSchema = lazySchema(() => strictObject( // ADR-0010 — runtime protection envelope (internal — set by loader). ...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. + const firstIndexById = new Map(); + flow.edges.forEach((edge, index) => { + const first = firstIndexById.get(edge.id); + if (first === undefined) { + firstIndexById.set(edge.id, index); + return; + } + ctx.addIssue({ + code: 'custom', + path: ['edges', index, 'id'], + message: + `Duplicate edge id \`${edge.id}\` — \`edges[${index}]\` reuses the id already declared by ` + + `\`edges[${first}]\`; every edge id in a flow must be unique. Renumber one of them: an ` + + 'edge id is the handle a designer, a BPMN export or a flow diff keys on, so a collision ' + + 'is silently wrong there rather than loudly broken.', + }); + }); })); /**