feat(spec)!: one node-id space — FlowSchema refuses a region node whose id is declared elsewhere in the flow (#16134)

.changeset/flow-one-node-id-space.md

@@ -0,0 +1,96 @@
+---
+"@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)
+
+<!-- adr-0087: not-required (no-migration-prescription) No authorable key is renamed, retired or re-typed: `nodes[].id` keeps its name, its type and its describe at every depth, and every flow whose node ids are unique across the whole flow parses byte-identically. The only newly refused shape is a region node (`loop.config.body`, `try_catch.config.try` / `.catch`, `parallel.config.branches[]`, at any depth the parse walks — nesting up to `MAX_REGION_DEPTH` = 32) carrying an id that a top-level node or a node in another region already declares — a collision, not a spelling — and its remedy is to rename one of the two (and re-point the edges that meant it), which is authoring intent no `objectstack migrate meta` rewrite can choose for the author. The census over this repository at `83863b2df` (AST scan of `packages/**` and `examples/**`: 972 outermost literal `nodes[]` arrays including tests, 66 excluding; 102 region arrays / 86 region nodes with a literal id, 15 / 11 excluding tests; a planted region-reuses-top-level-id control reads 1 at its planted line) found zero cross-region or region-vs-top-level collisions, so there is no in-repo file to name. -->
+
+**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 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
+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 the parse reaches via
+`collectFlowGraphs` — the top-level graph first, then each region in document
+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
+(`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, 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
+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.

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
 

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

packages/spec/src/automation/control-flow.zod.ts

@@ -406,7 +406,16 @@ 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), 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<string>();
   for (const n of nodes) {
     if (ids.has(n.id)) errors.push(`duplicate node id '${n.id}'`);
@@ -681,6 +690,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 +724,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;
 }