From a6822beff13355f15c1d33adab55752d15bf39d6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 13:55:02 +0000 Subject: [PATCH 1/6] wip(spec): refuse a pause-capable node and an end node inside a structured region Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude --- packages/spec/src/automation/flow.zod.ts | 114 +++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 553694a19a0..df1f33fec46 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -24,6 +24,7 @@ import { retryPolicyShape } from '../shared/retry-policy.zod'; import { strictObject } from '../shared/strict-object'; import { collectFlowGraphs, parseFlowNodeRegions } from './control-flow.zod'; import { EndConfigSchema } from './builtin-node-config.zod'; +import { APPROVAL_NODE_TYPE, APPROVAL_REVISE_NODE_TYPE } from './approval.zod'; export const FlowNodeAction = z.enum([ 'start', // Trigger 'end', // Return/Stop @@ -67,6 +68,44 @@ export const FLOW_BUILTIN_NODE_TYPES: readonly string[] = FlowNodeAction.options */ export const FLOW_STRUCTURAL_NODE_TYPES: readonly string[] = ['start', 'end']; +/** + * Built-in node types that can park a run on a **durable pause** (ADR-0019) — + * the vocabulary a structured region body may not contain. + * + * A capability list, not a behaviour prediction. Each entry is a node type + * whose shipped executor declares `supportsPause: true`; whether a given node + * of that type pauses on a given run is decided elsewhere and, for two of + * them, in ANOTHER metadata record: `map` and `subflow` pause exactly when the + * child flow they name pauses, which this flow's own text cannot answer. That + * is why the region rule keys on the TYPE — a contract whose verdict depends + * on a record the author is not editing is not a contract, and "legal until + * somebody adds a `wait` to the child flow" is the authoring trap this refusal + * exists to remove. + * + * Derived by reading the descriptors, not by recall: the shipped + * `defineActionDescriptor({... supportsPause: true ...})` literals are + * `screen` / `wait` / `subflow` / `map` in `service-automation`'s builtins and + * `approval` / `approval_revise` in `plugin-approvals`, which is the same set + * of six the ADR-0044 `resumeAuthority` default-flip migration entry names. + * `check:resume-authority-declared` is the gate that already scans exactly + * that population. + * + * ⚠️ **Not closed, and cannot be.** ADR-0018 made the node-type namespace open + * — `FlowNodeSchema.type` is a validated `string` and a plugin registers new + * types, pause-capable ones included, at run time. A parse has no registry, so + * a plugin-contributed pausing type inside a region is NOT refused here; the + * engine's own run-time refusal is what meets it. Extending this list is how a + * first-party type joins the rule. + */ +export const FLOW_PAUSE_CAPABLE_NODE_TYPES: readonly string[] = [ + 'screen', + 'wait', + 'subflow', + 'map', + APPROVAL_NODE_TYPE, + APPROVAL_REVISE_NODE_TYPE, +]; + /* * ── Unknown-key strictness (#4001, ADR-0078) ──────────────────────────────── * @@ -1154,6 +1193,81 @@ export const FlowSchema = lazySchema(() => strictObject( }); } + // What a structured region body may NOT contain (#15646, absorbing #18112). + // + // Two refusals, one rule family, because they are one limit read twice: an + // ADR-0031 region body (`loop` / `try_catch` / `parallel`, at every depth the + // walk above reaches) runs SYNCHRONOUSLY inside the enclosing run — it can + // neither park that run on a durable pause nor end it. #3267 ruled that limit + // 禁 rather than a gap to be filled, so this is its authoring-time + // enforcement, ⛔ not an interim. + // + // PAUSE — the engine converts a suspension raised inside a region into an + // error at the region boundary, but the executor has already written its + // progress state into the ENCLOSING scope by then. Contain that error in a + // `try_catch` and the residue is read back as progress by the next entry to + // the same node: measured on `loop { try_catch { map(pausing child) } }`, + // iteration 3 read `started === collection.length`, ran nothing, and + // returned SUCCESS with `summary.failed = 0`. A sweep that reports green + // having done nothing is the worst available failure, and it is reachable + // only because the shape can be declared at all. + // + // END — an `end` inside a region is a no-op today whatever its `outcome`, + // and a refusing one is converted into a region error at exactly the + // boundary above (#15788). Neither is what the author wrote, so the shape + // has never once been honoured. + // + // Judged on the node TYPE, and the reason is in + // {@link FLOW_PAUSE_CAPABLE_NODE_TYPES}: for `map` / `subflow` whether a pause + // happens is decided by a DIFFERENT metadata record. Two boundaries, declared + // rather than discovered — a plugin-registered pausing type is invisible to a + // parse (ADR-0018's open namespace), and this walk stops at + // `MAX_REGION_DEPTH`, so a region nested past the ceiling is not judged here + // and only the engine's run-time refusal meets it. + for (const graph of collectFlowGraphs(flow)) { + // The flow's own graph is the one place both are legal — that is the whole + // prescription both messages give, so it must stay true. + if (graph.path.length === 0) continue; + graph.nodes.forEach((node, index) => { + const type: unknown = (node as { type?: unknown } | null)?.type; + if (typeof type !== 'string') return; + const id: unknown = (node as { id?: unknown } | null)?.id; + const named = typeof id === 'string' ? `\`${id}\`` : `at index ${index}`; + if (type === 'end') { + ctx.addIssue({ + code: 'custom', + path: [...graph.path, 'nodes', index, 'type'], + message: + `An \`end\` node may not sit inside a structured region — \`${graph.scope}\` is a region ` + + `body and the \`end\` node ${named} is inside it. A region body cannot END the run: it runs ` + + 'synchronously inside the enclosing run, so an `end` here is a no-op whatever its `outcome`, ' + + 'and a refusing one is converted into a region error rather than terminating anything. Put ' + + "the `end` on the top-level graph and route the region's exit to it.", + }); + return; + } + if (FLOW_PAUSE_CAPABLE_NODE_TYPES.includes(type)) { + ctx.addIssue({ + code: 'custom', + path: [...graph.path, 'nodes', index, 'type'], + message: + `A \`${type}\` node may not sit inside a structured region — \`${graph.scope}\` is a region ` + + `body and the \`${type}\` node ${named} is inside it. A region body runs synchronously and ` + + 'cannot durably pause, while `' + type + '` is one of the node types that can park a run ' + + '(`screen` / `wait` / `subflow` / `map` / `approval` / `approval_revise`). The engine ' + + 'refuses such a pause at the region boundary AFTER the node has written its progress state ' + + 'into the enclosing scope, so a contained refusal leaves residue a later entry reads back as ' + + 'progress — the run then reports success having processed nothing. Move the `' + type + '` ' + + "node onto the top-level graph and route the region's exit to it; when the work must repeat " + + 'per item, make the top-level graph the repeating construct rather than nesting the pause. ' + + 'Judged on the node TYPE because `map` / `subflow` pause exactly when the child flow they ' + + 'name pauses, which is a different metadata record and can change without this flow being ' + + 'edited.', + }); + } + }); + } + // 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 From e10b395cee397a84295cc03dab8500414ce50f99 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 17 Sep 2026 14:15:02 +0000 Subject: [PATCH 2/6] feat(spec)!: a structured region body refuses a pause-capable node and an end node An ADR-0031 region body runs synchronously inside the enclosing run, so it can neither park that run on a durable pause nor terminate it. The engine already refused both at run time, silently and after the executor had written its progress state into the enclosing scope. FlowSchema now refuses the shapes at parse, naming the node and the region. Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude --- ...structured-region-pause-and-end-refused.md | 38 +++ packages/spec/api-surface/automation.json | 1 + packages/spec/export-origins/automation.json | 1 + .../src/automation/end-node-outcome.test.ts | 52 ++-- .../flow-region-pause-and-end.test.ts | 263 ++++++++++++++++++ packages/spec/src/automation/flow.test.ts | 54 ++-- packages/spec/src/automation/flow.zod.ts | 19 +- ...tured-region-body-pause-and-end-refused.ts | 66 +++++ packages/spec/src/migrations/registry.ts | 62 +++++ 9 files changed, 514 insertions(+), 42 deletions(-) create mode 100644 .changeset/15646-structured-region-pause-and-end-refused.md create mode 100644 packages/spec/src/automation/flow-region-pause-and-end.test.ts create mode 100644 packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts diff --git a/.changeset/15646-structured-region-pause-and-end-refused.md b/.changeset/15646-structured-region-pause-and-end-refused.md new file mode 100644 index 00000000000..549826a6afa --- /dev/null +++ b/.changeset/15646-structured-region-pause-and-end-refused.md @@ -0,0 +1,38 @@ +--- +'@objectstack/spec': minor +--- + +**BREAKING for authored metadata** — an ADR-0031 structured region body (`loop.config.body`, a `parallel` branch, `try_catch`'s `try` / `catch`) now refuses two node populations at parse: a node whose TYPE can durably pause, and an `end` node (#15646, absorbing #18112). + +`Clause-②: yes (narrowing)` — the flow accept set shrinks. Both refusals are the authoring-time enforcement of a limit the engine already holds at run time and #3267 ruled 禁: **a region body runs synchronously inside the enclosing run, so it can neither park that run nor terminate it.** + +``` +✗ nodes.1.config.body.nodes.0.type: A `map` node may not sit inside a structured region — + `loop 'sweep' body → try_catch 'guard' try` is a region body and the `map` node `per_item` + is inside it. A region body runs synchronously and cannot durably pause … +``` + +**What is refused** + +- **A pause-capable node** — `screen`, `wait`, `subflow`, `map`, `approval`, `approval_revise`, the six built-in types whose shipped executor declares `supportsPause: true`, published as `FLOW_PAUSE_CAPABLE_NODE_TYPES`. +- **An `end` node**, whatever its `outcome`. An `end` in a region was a no-op, and a refusing one was converted into a region error at the boundary; neither is what the author wrote. + +**Why it was silent, measured.** The engine converts a suspension raised inside a region into an error — but the executor has already written its progress state into the ENCLOSING scope by then. Contain that error in a `try_catch` and the residue is read back as progress by the next entry to the same node. On a real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations × 2 items: not one item's subflow completed, only two of three iterations reached the catch, and iteration 3 read `started === collection.length`, ran nothing, and returned `success` with `summary.failed = 0`. A sweep that reports green having processed nothing is the worst available failure, and it is reachable only because the shape can be declared at all. + +### Migration — FROM → TO + +| You wrote | Write instead | +| --- | --- | +| `loop { body: [ …, end ] }` | `loop { body: [ … ] } → end` — give the region a normal exit and put the terminator, with its `outcome` / `message`, on the top-level graph | +| `loop { body: [ try_catch { try: [ map ] } ] }` | a top-level `map` — its per-item subflow already iterates, so the enclosing `loop` is usually redundant, and the `try_catch` that existed only to contain the region's refusal goes with it | +| `parallel { branches: [ [ approval ] , … ] }` | put the `approval` on the top-level graph and fan out around it, or split the branch's pausing half into a `subflow` the top-level graph calls | + +The one-line fix is always the same: **move the node onto the top-level graph and route the region's exit to it.** ⛔ Not mechanically convertible — hoisting a node out of a region is a graph rewrite (new edges, a changed exit, sometimes a deleted container) and which shape the author meant is an intent no artifact records, so this ships as an ADR-0087 D3 structured TODO rather than a D2 conversion. + + + +**⚠️ Wider than the runs that actually broke, deliberately.** The rule judges the node TYPE. A `map` or `subflow` pauses exactly when the child flow it names pauses, so a region-nested `map` over a synchronous child ran green before and is refused now. That shape's legality lived in a DIFFERENT metadata record and could be revoked by editing that record — "legal until somebody adds a `wait` to the child flow" is not a contract an author can rely on, and a parse of one flow cannot answer it. + +**⚠️ Two boundaries this refusal does not reach, stated rather than discovered.** A pausing node type contributed by a **plugin** is not refused: ADR-0018 left the node-type namespace open and a parse has no registry. A region nested past **`MAX_REGION_DEPTH` (32)** is not judged: the parse walk stops there, and unlike a duplicate node id there is no second spec refusal behind it. For both, the engine's run-time refusal is the only one — unchanged by this change, and not fixed by it. + +⛔ `packages/services` is untouched. The engine's run-time refusal stays exactly as it was; this moves the refusal to where the author is standing, it does not replace it. diff --git a/packages/spec/api-surface/automation.json b/packages/spec/api-surface/automation.json index 9dc1216ccf7..6189e5214c2 100644 --- a/packages/spec/api-surface/automation.json +++ b/packages/spec/api-surface/automation.json @@ -111,6 +111,7 @@ "ExecutionStepSkipReasonSchema (const)", "FLOW_BUILTIN_NODE_TYPES (const)", "FLOW_NODE_EXPRESSION_PATHS (const)", + "FLOW_PAUSE_CAPABLE_NODE_TYPES (const)", "FLOW_REGION_CONFIG_KEYS (const)", "FLOW_REGION_SLOTS (const)", "FLOW_REGION_SLOTS_BY_TYPE (const)", diff --git a/packages/spec/export-origins/automation.json b/packages/spec/export-origins/automation.json index 26082654c07..1eaea1c7be5 100644 --- a/packages/spec/export-origins/automation.json +++ b/packages/spec/export-origins/automation.json @@ -107,6 +107,7 @@ "ExecutionStepSkipReasonSchema": "src/automation/execution.zod.ts#ExecutionStepSkipReasonSchema (const)", "FLOW_BUILTIN_NODE_TYPES": "src/automation/flow.zod.ts#FLOW_BUILTIN_NODE_TYPES (const)", "FLOW_NODE_EXPRESSION_PATHS": "src/automation/flow-node-expression-paths.ts#FLOW_NODE_EXPRESSION_PATHS (const)", + "FLOW_PAUSE_CAPABLE_NODE_TYPES": "src/automation/flow.zod.ts#FLOW_PAUSE_CAPABLE_NODE_TYPES (const)", "FLOW_REGION_CONFIG_KEYS": "src/automation/region-slots.ts#FLOW_REGION_CONFIG_KEYS (const)", "FLOW_REGION_SLOTS": "src/automation/region-slots.ts#FLOW_REGION_SLOTS (const)", "FLOW_REGION_SLOTS_BY_TYPE": "src/automation/region-slots.ts#FLOW_REGION_SLOTS_BY_TYPE (const)", diff --git a/packages/spec/src/automation/end-node-outcome.test.ts b/packages/spec/src/automation/end-node-outcome.test.ts index 086edab1070..a98ec3e026d 100644 --- a/packages/spec/src/automation/end-node-outcome.test.ts +++ b/packages/spec/src/automation/end-node-outcome.test.ts @@ -21,7 +21,6 @@ import { describe, it, expect } from 'vitest'; import { EndConfigSchema } from './builtin-node-config.zod'; import { FlowSchema, FlowNodeSchema, defineFlow, type Flow } from './flow.zod'; -import { validateControlFlow } from './control-flow.zod'; import { ExecutionLogSchema, ExecutionStatus } from './execution.zod'; import { formatZodError } from '../shared/error-map.zod'; @@ -218,13 +217,15 @@ describe('FlowSchema applies the `end` contract — the structural node\'s only expect(issues?.map((i) => [i.code, i.path])).toEqual([['custom', ['nodes', 1, 'config', 'message']]]); }); - it('a region-nested `end` is checked at the region door: the flow parse leaves the region raw, validateControlFlow refuses it by name', () => { - // `parseFlowNodeRegions` deliberately leaves a region it cannot parse - // untouched (the registration walk owns nested diagnostics, #4389), so the - // FLOW parse alone does not surface a nested refusal — the same boundary - // every other nested node key has. `validateControlFlow` re-parses the - // region through `FlowNodeSchema`, where this contract now lives, and - // throws with the same sentence. + it('a region-nested `end` is refused by the FLOW parse itself (#15646/#18112) — the region-door reading this test used to pin is unreachable, because the shape is gone', () => { + // ⚠️ REPLACED, not re-spelled. This case used to assert that the flow parse + // was GREEN here and that `validateControlFlow` was the door — a true + // reading of `parseFlowNodeRegions` leaving a refused region raw (#4389). + // #15646 removes its subject: an `end` node inside a structured region body + // is refused at parse, whatever its `config`, so there is no longer a + // region-nested `end` whose CONFIG can be judged one door later. Keeping the + // old assertion by weakening it would have pinned an `end`-in-region shape + // that is now undeclarable. const nested: Flow = { name: 'nested_refusal', label: 'Nested refusal', @@ -249,16 +250,31 @@ describe('FlowSchema applies the `end` contract — the structural node\'s only ], }; const parsed = FlowSchema.safeParse(nested); - expect(parsed.success).toBe(true); - if (!parsed.success) return; - let message = ''; - try { - validateControlFlow(parsed.data); - } catch (error) { - message = (error as Error).message; - } - expect(message).toContain("loop 'each' body"); - expect(message).toContain("`outcome: 'refused'` requires a `message`"); + expect(parsed.success).toBe(false); + if (parsed.success) return; + expect(parsed.error.issues.map((i) => [i.code, i.path])).toEqual([ + ['custom', ['nodes', 1, 'config', 'body', 'nodes', 0, 'type']], + ]); + expect(parsed.error.issues[0].message).toContain( + "An `end` node may not sit inside a structured region — `loop 'each' body` is a region body and the `end` node `inner_end` is inside it", + ); + + // CONTROL — the identical `end` node, identical malformed config, on the + // TOP-LEVEL graph: refused by the `end` CONFIG contract instead, at + // `config.message`. So the reading above is the region rule firing, not this + // fixture being malformed in some way that would fail anywhere. + const topLevel = FlowSchema.safeParse({ + ...nested, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'inner_end', type: 'end', label: 'Inner end', config: { outcome: 'refused' } }, + ], + edges: [{ id: 'e1', source: 'start', target: 'inner_end' }], + } as Flow); + expect(topLevel.success).toBe(false); + if (topLevel.success) return; + expect(topLevel.error.issues.map((i) => i.path)).toEqual([['nodes', 1, 'config', 'message']]); + expect(topLevel.error.issues[0].message).toContain("`outcome: 'refused'` requires a `message`"); }); }); diff --git a/packages/spec/src/automation/flow-region-pause-and-end.test.ts b/packages/spec/src/automation/flow-region-pause-and-end.test.ts new file mode 100644 index 00000000000..691e41e8701 --- /dev/null +++ b/packages/spec/src/automation/flow-region-pause-and-end.test.ts @@ -0,0 +1,263 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * What a structured region body may NOT contain — #15646, absorbing #18112. + * + * Director seat ruling 5714239196 (maintainer 「同意,其他也同意」) and its scope + * addition 5714981966 (maintainer 「146 同意」), quoted in the PR: an ADR-0031 + * region body (`loop` / `try_catch` / `parallel`) refuses a node that can + * durably pause, and refuses an `end` node — one rule family, one PR. + * + * Both halves are authoring-time enforcement of the limit #3267 ruled 禁: a + * region body runs synchronously inside the enclosing run, so it can neither + * park that run nor terminate it. The engine already refuses both AT RUN TIME + * (`durable pause inside a structured region … is not supported`, and the + * #15788 refusing-`end` conversion) — this moves the refusal to where the + * author is standing. + * + * Every case here fails without the rule: the shapes below all parsed green + * before it. + */ +import { describe, it, expect } from 'vitest'; +import { + FlowSchema, + FLOW_PAUSE_CAPABLE_NODE_TYPES, + defineFlow, + type Flow, + type FlowNode, + type FlowEdge, +} from './flow.zod'; +import { APPROVAL_NODE_TYPE, APPROVAL_REVISE_NODE_TYPE } from './approval.zod'; +import { validateControlFlow } from './control-flow.zod'; +import { formatZodError } from '../shared/error-map.zod'; + +/** A `wait` node owes a `waitEventConfig` block, so every fixture carries one. */ +const pausingNode = (type: string, id = 'pauser'): FlowNode => ({ + id, + type, + label: `A ${type}`, + ...(type === 'wait' ? { waitEventConfig: { eventType: 'timer' as const, timerDuration: 'PT1H' } } : {}), + ...(type === 'map' ? { config: { collection: '{items}', flowName: 'per_item' } } : {}), + ...(type === 'subflow' ? { config: { flowName: 'child' } } : {}), +}); + +const step = (id: string): FlowNode => ({ id, type: 'assignment', label: id }); + +const flowWith = (nodes: FlowNode[], edges: FlowEdge[] = []): Flow => ({ + name: 'region_contents', + label: 'Region contents', + type: 'autolaunched', + nodes: [{ id: 'start', type: 'start', label: 'Start' }, ...nodes], + edges, +}); + +const loopOver = (bodyNodes: FlowNode[], id = 'sweep'): FlowNode => ({ + id, type: 'loop', label: 'Sweep', + config: { collection: '{items}', body: { nodes: bodyNodes, edges: [] } }, +}); + +const tryCatchOver = (tryNodes: FlowNode[], catchNodes: FlowNode[], id = 'guard'): FlowNode => ({ + id, type: 'try_catch', label: 'Guard', + config: { try: { nodes: tryNodes, edges: [] }, catch: { nodes: catchNodes, edges: [] } }, +}); + +const parallelOver = (branches: FlowNode[][], id = 'fan'): FlowNode => ({ + id, type: 'parallel', label: 'Fan out', + config: { branches: branches.map((nodes) => ({ nodes, edges: [] })) }, +}); + +/** Every issue as `[path, message]`, for the assertions below. */ +const issuesOf = (flow: Flow): Array<[string, string]> => { + const result = FlowSchema.safeParse(flow); + if (result.success) return []; + return result.error.issues.map((i) => [i.path.join('.'), i.message]); +}; + +describe('FLOW_PAUSE_CAPABLE_NODE_TYPES — the declared set, and how it was derived', () => { + it('names the six built-in types whose shipped executor declares `supportsPause: true`', () => { + // Read off the descriptors, not recalled: `screen` / `wait` / `subflow` / + // `map` in `service-automation`'s builtins, `approval` / `approval_revise` + // in `plugin-approvals` — the same six the ADR-0044 `resumeAuthority` + // default-flip migration entry names in its own prose. + expect([...FLOW_PAUSE_CAPABLE_NODE_TYPES]).toEqual([ + 'screen', 'wait', 'subflow', 'map', 'approval', 'approval_revise', + ]); + }); + + it('carries the approval node types by their declared constants, so a rename cannot desynchronise the two', () => { + expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).toContain(APPROVAL_NODE_TYPE); + expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).toContain(APPROVAL_REVISE_NODE_TYPE); + }); +}); + +describe('a region body refuses a pause-capable node (#15646)', () => { + it.each(FLOW_PAUSE_CAPABLE_NODE_TYPES)('refuses a `%s` node in a loop body, anchored on its `type`', (type) => { + expect(issuesOf(flowWith([loopOver([pausingNode(type)])]))).toEqual([[ + 'nodes.1.config.body.nodes.0.type', + expect.stringContaining( + `A \`${type}\` node may not sit inside a structured region — \`loop 'sweep' body\` is a region body ` + + `and the \`${type}\` node \`pauser\` is inside it`, + ) as unknown as string, + ]]); + }); + + it('refuses it in a try_catch TRY region', () => { + expect(issuesOf(flowWith([tryCatchOver([pausingNode('map')], [step('recover')])]))).toEqual([[ + 'nodes.1.config.try.nodes.0.type', + expect.stringContaining("`try_catch 'guard' try` is a region body and the `map` node `pauser` is inside it") as unknown as string, + ]]); + }); + + it('refuses it in a try_catch CATCH region — the arm route B could never see', () => { + expect(issuesOf(flowWith([tryCatchOver([step('attempt')], [pausingNode('approval')])]))).toEqual([[ + 'nodes.1.config.catch.nodes.0.type', + expect.stringContaining("`try_catch 'guard' catch` is a region body and the `approval` node `pauser` is inside it") as unknown as string, + ]]); + }); + + it('refuses it in a parallel BRANCH — the other arm route B could never see', () => { + expect(issuesOf(flowWith([parallelOver([[step('left')], [pausingNode('screen')]])]))).toEqual([[ + 'nodes.1.config.branches.1.nodes.0.type', + expect.stringContaining("`parallel 'fan' branch 1` is a region body and the `screen` node `pauser` is inside it") as unknown as string, + ]]); + }); + + it("refuses this card's own reproduction — `loop { try_catch { map } }` — with the chained region path", () => { + const issues = issuesOf(flowWith([loopOver([tryCatchOver([pausingNode('map')], [step('recover')])])])); + expect(issues.map(([path]) => path)).toEqual([ + 'nodes.1.config.body.nodes.0.config.try.nodes.0.type', + ]); + expect(issues[0][1]).toContain("`loop 'sweep' body → try_catch 'guard' try` is a region body"); + }); + + it('says WHY on the node type rather than on the child flow — the sentence an author acts on', () => { + const [[, message]] = issuesOf(flowWith([loopOver([pausingNode('map')])])); + expect(message).toContain('A region body runs synchronously and cannot durably pause'); + expect(message).toContain('reads back as progress'); + expect(message).toContain("Move the `map` node onto the top-level graph and route the region's exit to it"); + expect(message).toContain('`map` / `subflow` pause exactly when the child flow they name pauses'); + }); + + it('renders through formatZodError pointing INTO the region', () => { + const result = FlowSchema.safeParse(flowWith([loopOver([pausingNode('wait')])])); + expect(result.success).toBe(false); + if (result.success) return; + expect(formatZodError(result.error)).toContain( + 'nodes.1.config.body.nodes.0.type: A `wait` node may not sit inside a structured region', + ); + }); + + it('defineFlow refuses it with the same anchored issue', () => { + let caught: unknown; + try { + defineFlow(flowWith([loopOver([pausingNode('subflow')])])); + } 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, 'type']], + ]); + }); +}); + +describe('a region body refuses an `end` node (#18112, absorbed into #15646)', () => { + it.each([ + ['loop body', loopOver([{ id: 'stop', type: 'end', label: 'Stop' }]), 'nodes.1.config.body.nodes.0.type', "loop 'sweep' body"], + ['try region', tryCatchOver([{ id: 'stop', type: 'end', label: 'Stop' }], [step('recover')]), 'nodes.1.config.try.nodes.0.type', "try_catch 'guard' try"], + ['catch region', tryCatchOver([step('attempt')], [{ id: 'stop', type: 'end', label: 'Stop' }]), 'nodes.1.config.catch.nodes.0.type', "try_catch 'guard' catch"], + ['parallel branch', parallelOver([[step('left')], [{ id: 'stop', type: 'end', label: 'Stop' }]]), 'nodes.1.config.branches.1.nodes.0.type', "parallel 'fan' branch 1"], + ])('refuses an `end` in a %s', (_kind, container, path, scope) => { + const issues = issuesOf(flowWith([container])); + expect(issues.map(([p]) => p)).toEqual([path]); + expect(issues[0][1]).toContain( + `An \`end\` node may not sit inside a structured region — \`${scope}\` is a region body and the \`end\` node \`stop\` is inside it`, + ); + }); + + it('gives the ruled prescription verbatim — a region body cannot end the run; put the `end` on the top-level graph', () => { + const [[, message]] = issuesOf(flowWith([loopOver([{ id: 'stop', type: 'end', label: 'Stop' }])])); + expect(message).toContain('A region body cannot END the run'); + expect(message).toContain("Put the `end` on the top-level graph and route the region's exit to it"); + }); + + it('refuses it whatever the `outcome` — a plain terminal is a no-op there, a refusing one is converted to a region error', () => { + for (const config of [undefined, { outcome: 'completed' as const }, { outcome: 'refused' as const, message: 'Refused: {record.name}' }]) { + const issues = issuesOf(flowWith([loopOver([{ id: 'stop', type: 'end', label: 'Stop', config }])])); + expect(issues.map(([p]) => p), JSON.stringify(config)).toEqual(['nodes.1.config.body.nodes.0.type']); + } + }); +}); + +describe('the rule does NOT over-reach', () => { + it('accepts every pause-capable type on the TOP-LEVEL graph — a refusal that over-reaches is worse than the silence it replaces', () => { + for (const type of FLOW_PAUSE_CAPABLE_NODE_TYPES) { + const flow = flowWith([pausingNode(type)], [{ id: 'e1', source: 'start', target: 'pauser' }]); + expect(FlowSchema.safeParse(flow).success, type).toBe(true); + } + }); + + it('accepts an `end` on the top-level graph, including one inside a flow that also has regions', () => { + const flow = flowWith( + [loopOver([step('work')]), { id: 'stop', type: 'end', label: 'Stop' }], + [{ id: 'e1', source: 'start', target: 'sweep' }, { id: 'e2', source: 'sweep', target: 'stop' }], + ); + const parsed = FlowSchema.safeParse(flow); + expect(parsed.success).toBe(true); + if (!parsed.success) return; + expect(() => validateControlFlow(parsed.data)).not.toThrow(); + }); + + it('leaves every non-pausing node type alone inside a region — the rule is a list, not a mood', () => { + for (const type of ['assignment', 'decision', 'create_record', 'update_record', 'get_record', 'http', 'notify', 'loop']) { + const node: FlowNode = type === 'loop' + ? loopOver([step('inner')], 'inner_loop') + : { id: 'work', type, label: 'Work' }; + expect(FlowSchema.safeParse(flowWith([loopOver([node])])).success, type).toBe(true); + } + }); + + it('judges the node TYPE, not the node id — a node merely NAMED `end` or `wait` in a region still parses', () => { + expect(FlowSchema.safeParse(flowWith([loopOver([step('end'), step('wait')])])).success).toBe(true); + }); +}); + +describe('the two declared boundaries — measured, so they move deliberately', () => { + it('a PLUGIN-contributed pausing type is not refused: a parse has no registry (ADR-0018 open namespace)', () => { + // ⚠️ Not an oversight and not a gap to quietly close: `FlowNodeSchema.type` + // is a validated `string`, and a plugin registers pause-capable types at run + // time. The engine's own run-time refusal is what meets this one. Extending + // `FLOW_PAUSE_CAPABLE_NODE_TYPES` is how a first-party type joins the rule — + // and this pin is what makes that an edit somebody makes on purpose. + const flow = flowWith([loopOver([{ id: 'vendor', type: 'vendor_signature_pause', label: 'Sign' }])]); + expect(FlowSchema.safeParse(flow).success).toBe(true); + }); + + it('the seam at MAX_REGION_DEPTH: a pause-capable node at nesting 32 is refused; at nesting 33 the parse does not judge it', () => { + // The walk this rule rides (`collectFlowGraphs`) stops at 32, the ceiling + // `parseFlowNodeRegions` shares — the same measured boundary #16134's + // one-id-space rule hands off at. Past it there is no second spec refusal + // for THIS rule (unlike a duplicate id, which `analyzeRegion` still names), + // so the engine's run-time refusal is the only one left. Stated in the + // docblock and in the changeset rather than discovered by an author. + const nestedTo = (nesting: number): Flow => { + let body: { nodes: FlowNode[]; edges: FlowEdge[] } = { nodes: [pausingNode('map')], 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 JSON.parse(JSON.stringify(flowWith([ + { id: 'sweep', type: 'loop', label: 'Sweep', config: { collection: '{items}', body } }, + ]))) as Flow; + }; + + const atCeiling = FlowSchema.safeParse(nestedTo(32)); + expect(atCeiling.success).toBe(false); + if (atCeiling.success) return; + expect(atCeiling.error.issues).toHaveLength(1); + expect(atCeiling.error.issues[0].message).toContain('A `map` node may not sit inside a structured region'); + expect(atCeiling.error.issues[0].path.slice(-2)).toEqual([0, 'type']); + + expect(FlowSchema.safeParse(nestedTo(33)).success).toBe(true); + }); +}); diff --git a/packages/spec/src/automation/flow.test.ts b/packages/spec/src/automation/flow.test.ts index e82da586a05..379e7d7df8d 100644 --- a/packages/spec/src/automation/flow.test.ts +++ b/packages/spec/src/automation/flow.test.ts @@ -1464,22 +1464,26 @@ describe('BPMN — Wait Event Configuration', () => { }); /** - * Nested in an ADR-0031 region, the refusal is reached through the REGION - * CONTRACT, not through `FlowSchema` — measured here rather than assumed, - * because the two doors answer differently and only one of them is the door - * a run actually passes through. + * Nested in an ADR-0031 region, `waitEventConfig`'s refusal is reached through + * the REGION CONTRACT (`LoopConfigSchema`) — measured here rather than + * assumed, because the two doors answer differently and only one of them is + * the door a run actually passes through. * * `parseFlowNodeRegions` parses each region slot with `safeParse` and, on a * refusal, leaves the region RAW and continues (its own comment says so: a - * refused region is left for `validateControlFlow` to name). That policy - * predates this change and is not specific to `waitEventConfig` — it is why the - * flow-level parse below is GREEN with a block-less wait sitting in the loop - * body. The contract that refuses it is `LoopConfigSchema`, which is what - * `loop`'s executor parses its config through at execute time - * (`parseNodeConfig` → a guard refusal), so the nested shape still cannot - * run; it is refused one door later and by node id. + * refused region is left for `validateControlFlow` to name). That policy is + * not specific to `waitEventConfig`, and it is why the flow parse never + * carried this key's refusal. + * + * ⚠️ What #15646 changed, and what it did not: the FLOW parse is no longer + * green on the nested fixture, because a `wait` node may not sit inside a + * region body AT ALL now — a region body cannot durably pause, and `wait` + * always does. So the flow parse refuses the fixture for a reason that has + * nothing to do with `waitEventConfig`, and the block's own refusal is still + * the region contract's. Both are asserted below so the two cannot be + * confused for each other again. */ - it('nested in a region: the flow parse leaves it raw, and the REGION contract refuses it by path', () => { + it('nested in a region: the flow parse refuses the `wait` node itself, and the REGION contract still refuses the missing block by path', () => { const loopConfig = (bodyNode: Record) => ({ collection: '{rows}', iteratorVariable: 'row', body: { nodes: [bodyNode], edges: [] }, @@ -1498,20 +1502,34 @@ describe('BPMN — Wait Event Configuration', () => { edges: [{ id: 'e1', source: 'start', target: 'loop' }], }); - // Door 1 — the flow parse: green, and the body node comes back UNPARSED. + // Door 1 — the flow parse: refused on the node TYPE, not on the block. const flowParse = FlowSchema.safeParse(flowWith(bare)); - expect(flowParse.success).toBe(true); + expect(flowParse.success).toBe(false); + if (flowParse.success) return; + expect(flowParse.error.issues.map(i => i.path.join('.'))).toEqual(['nodes.1.config.body.nodes.0.type']); + expect(flowParse.error.issues[0].message).toContain('A `wait` node may not sit inside a structured region'); // Door 2 — the region contract the `loop` executor parses through: refused, - // anchored on the block, at the node's own index inside the body. + // anchored on the block, at the node's own index inside the body. Reached + // directly, because the flow that would carry it no longer parses. const refused = LoopConfigSchema.safeParse(loopConfig(bare)); expect(refused.success).toBe(false); expect(refused.error!.issues.map(i => i.path.join('.'))).toContain('body.nodes.0.waitEventConfig'); - // CONTROL — the declared body node passes both doors, so the refusal above - // is the missing block and not the region fixture. - expect(FlowSchema.safeParse(flowWith(declared)).success).toBe(true); + // CONTROL — the DECLARED body node still passes door 2, so door 2's refusal + // above is the missing block and not the region fixture. It does not pass + // door 1: the region rule judges the type, and a declared block does not + // make a `wait` pausable-in-a-region. expect(LoopConfigSchema.safeParse(loopConfig(declared)).success).toBe(true); + expect(FlowSchema.safeParse(flowWith(declared)).success).toBe(false); + + // CONTROL — the same declared `wait` on the TOP-LEVEL graph passes the flow + // parse, so door 1's refusal is the region and not the node. + expect(FlowSchema.safeParse({ + name: 'top_level', label: 'Top level', type: 'autolaunched', + nodes: [{ id: 'start', type: 'start', label: 'Start' }, declared], + edges: [{ id: 'e1', source: 'start', target: 'pause' }], + }).success).toBe(true); }); }); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index df1f33fec46..044bbbba129 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -329,16 +329,23 @@ export const FlowNodeSchema = lazySchema(() => flowNodeObject().transform( * parses each region slot with `safeParse` and, on a refusal, leaves that region * RAW and continues (its own comment says so: a refused region is left for * `validateControlFlow` to name). That policy predates this change and is not - * specific to `waitEventConfig`, and the consequence is measurable: - * `FlowSchema.safeParse` of a flow whose `loop` body holds a block-less `wait` - * answers `success: true`. What refuses the nested node is the REGION contract — + * specific to `waitEventConfig`. What refuses the nested node is the REGION contract — * `LoopConfigSchema` / `ParallelConfigSchema` / `TryCatchConfigSchema` — at * `body.nodes[i].waitEventConfig`, which is the same contract the container * node's executor parses its config through at execute time, so the nested shape * still cannot RUN; it is refused one door later and by node id. Both halves are - * pinned in `flow.test.ts` ("nested in a region: the flow parse leaves it raw, - * and the REGION contract refuses it by path"), and the ADR-0087 entry's - * `acceptanceCriteria` states the same thing for whoever migrates a stack. + * pinned in `flow.test.ts` ("nested in a region: the flow parse refuses the + * `wait` node itself, and the REGION contract still refuses the missing block by + * path"), and the ADR-0087 entry's `acceptanceCriteria` states the same thing + * for whoever migrates a stack. + * + * ⚠️ Since #15646 a `wait` nested in a region body meets an EARLIER refusal than + * either of those, and it is not about this block: a region body cannot durably + * pause, so {@link FLOW_PAUSE_CAPABLE_NODE_TYPES} may not appear in one at all + * and the flow parse says so on the node's `type`. ⛔ Do not read the paragraph + * above as "a nested block-less `wait` parses" — it no longer does, for a + * different reason. The two-door reading it describes still governs every node + * type the region rule leaves alone. * * ⚠️ `boundary_event` gets the contract half ONLY: the platform registers no * executor for that type at all (`NO_EXECUTOR` plus a startup `warn`, measured diff --git a/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts b/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts new file mode 100644 index 00000000000..6a3c17bc98b --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts @@ -0,0 +1,66 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +export const entry: SemanticMigration = { + id: 'structured-region-body-pause-and-end-refused', + surface: + 'The BODY of every ADR-0031 structured region — `loop.config.body`, each ' + + '`parallel.config.branches[]`, and `try_catch.config.try` / `.catch` — at every depth the ' + + 'flow parse walks. Two node populations become undeclarable there: a node whose TYPE can ' + + 'durably pause (`screen`, `wait`, `subflow`, `map`, `approval`, `approval_revise`) and an ' + + '`end` node, whatever its `outcome`. ⚠️ Judged on the node TYPE, which is WIDER than the ' + + 'runs that actually broke: a `map` or `subflow` pauses exactly when the child flow it ' + + 'names pauses, so a region-nested `map` over a synchronous child ran green and is refused ' + + 'now. That is deliberate — the legality of the old shape lived in a DIFFERENT metadata ' + + 'record and could be revoked by editing that record, which is not a contract an author ' + + 'can rely on.', + replacement: + 'Move the node onto the TOP-LEVEL graph and route the region\'s exit to it. For an `end`: ' + + 'delete it from the body, give the region a normal exit, and put the terminator (with its ' + + '`outcome` / `message`) on the top-level graph — `loop { body: [ …, end ] }` becomes ' + + '`loop { body: [ … ] } → end`. For a pausing node: hoist it out of the container — ' + + '`loop { body: [ try_catch { try: [ map ] } ] }` becomes a top-level `map` (its own ' + + 'per-item subflow already iterates, so the enclosing `loop` is usually redundant), and ' + + 'where the repetition is genuinely needed, make the TOP-LEVEL graph the repeating ' + + 'construct with the pause on it rather than nesting the pause inside a region. A ' + + '`try_catch` whose only purpose was to contain the region\'s refusal has nothing left to ' + + 'contain and is deleted with it.', + reason: + 'Maintainer ruling, decision batch #145 item 5, verbatim and untranslated: 「同意,其他也同' + + '意」, carrying the presented option C; extended by batch #146 「146 同意」, which attached ' + + 'the `end` half (the absorbed #18112) and recorded that #3267 is ruled 禁 — structured ' + + 'regions do not support durable pause and a region body cannot terminate the run, so this ' + + 'is that limit\'s authoring-time enforcement rather than an interim. The refusal already ' + + 'existed AT RUN TIME and said nothing an author could act on: the engine converts a ' + + 'suspension raised inside a region into an error at the region boundary, AFTER the ' + + 'executor has written its progress state into the enclosing scope, so a `try_catch` that ' + + 'contains that error leaves residue the next entry reads back as progress. Measured on a ' + + 'real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations x ' + + '2 items: not one item\'s subflow ever completed, only two of three iterations reached ' + + 'the catch, and iteration 3 read `started === collection.length`, ran nothing, and ' + + 'returned SUCCESS with `summary.failed = 0`. ⛔ NOT losslessly convertible: hoisting a ' + + 'node out of a region is a GRAPH REWRITE — new edges, a changed exit, sometimes a deleted ' + + 'container — and which of several shapes the author meant is an intent no artifact ' + + 'records, so a transform that picked one would be inventing the design. That leaves D3, a ' + + 'structured TODO naming each node to edit. ⚠️ Two boundaries this refusal deliberately ' + + 'does NOT reach, because a parse cannot: a pausing node type contributed by a PLUGIN ' + + '(ADR-0018 left the node-type namespace open and a parse has no registry), and a region ' + + 'nested past `MAX_REGION_DEPTH` (32), where the walk stops. For both, the engine\'s ' + + 'run-time refusal is still the only one — unchanged by this step, not fixed by it.', + acceptanceCriteria: + 'No `screen` / `wait` / `subflow` / `map` / `approval` / `approval_revise` node and no ' + + '`end` node sits inside any `loop` body, `parallel` branch or `try_catch` try/catch ' + + 'region in the stack, at any depth. `FlowSchema.parse` (and therefore `defineFlow`, ' + + '`registerFlow`, `os validate` and a Studio publish) accepts the stack: a node still ' + + 'nested is refused with the node AND the region named in one message ' + + "(`A \\`map\\` node may not sit inside a structured region — \\`loop 'sweep' body → " + + "try_catch 'guard' try\\` is a region body …`), anchored at " + + '`nodes[i].config.body.nodes[j].type` so a designer can jump to it. Behaviour to re-check ' + + 'after editing, because the fix CHANGES IT deliberately: a sweep that used to report ' + + '`success` having processed nothing now processes its items, so anything downstream that ' + + 'had quietly stopped receiving work starts receiving it again, and any alerting tuned to ' + + 'the empty-but-green runs will see real volume. A region-nested `end` was a no-op, so ' + + 'moving it to the top level makes the run actually TERMINATE there — check that the nodes ' + + 'after the container were not relying on continuing past it.', +}; diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 3d54818d5d9..04c10e48690 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -10891,6 +10891,68 @@ const step18: MigrationStep = { + 'parse-and-refuse accepts and rejects exactly the same sets before and after, ' + 'and no stored metadata or document needs editing.', }, + { + id: 'structured-region-body-pause-and-end-refused', + surface: + 'The BODY of every ADR-0031 structured region — `loop.config.body`, each ' + + '`parallel.config.branches[]`, and `try_catch.config.try` / `.catch` — at every depth the ' + + 'flow parse walks. Two node populations become undeclarable there: a node whose TYPE can ' + + 'durably pause (`screen`, `wait`, `subflow`, `map`, `approval`, `approval_revise`) and an ' + + '`end` node, whatever its `outcome`. ⚠️ Judged on the node TYPE, which is WIDER than the ' + + 'runs that actually broke: a `map` or `subflow` pauses exactly when the child flow it ' + + 'names pauses, so a region-nested `map` over a synchronous child ran green and is refused ' + + 'now. That is deliberate — the legality of the old shape lived in a DIFFERENT metadata ' + + 'record and could be revoked by editing that record, which is not a contract an author ' + + 'can rely on.', + replacement: + 'Move the node onto the TOP-LEVEL graph and route the region\'s exit to it. For an `end`: ' + + 'delete it from the body, give the region a normal exit, and put the terminator (with its ' + + '`outcome` / `message`) on the top-level graph — `loop { body: [ …, end ] }` becomes ' + + '`loop { body: [ … ] } → end`. For a pausing node: hoist it out of the container — ' + + '`loop { body: [ try_catch { try: [ map ] } ] }` becomes a top-level `map` (its own ' + + 'per-item subflow already iterates, so the enclosing `loop` is usually redundant), and ' + + 'where the repetition is genuinely needed, make the TOP-LEVEL graph the repeating ' + + 'construct with the pause on it rather than nesting the pause inside a region. A ' + + '`try_catch` whose only purpose was to contain the region\'s refusal has nothing left to ' + + 'contain and is deleted with it.', + reason: + 'Maintainer ruling, decision batch #145 item 5, verbatim and untranslated: 「同意,其他也同' + + '意」, carrying the presented option C; extended by batch #146 「146 同意」, which attached ' + + 'the `end` half (the absorbed #18112) and recorded that #3267 is ruled 禁 — structured ' + + 'regions do not support durable pause and a region body cannot terminate the run, so this ' + + 'is that limit\'s authoring-time enforcement rather than an interim. The refusal already ' + + 'existed AT RUN TIME and said nothing an author could act on: the engine converts a ' + + 'suspension raised inside a region into an error at the region boundary, AFTER the ' + + 'executor has written its progress state into the enclosing scope, so a `try_catch` that ' + + 'contains that error leaves residue the next entry reads back as progress. Measured on a ' + + 'real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations x ' + + '2 items: not one item\'s subflow ever completed, only two of three iterations reached ' + + 'the catch, and iteration 3 read `started === collection.length`, ran nothing, and ' + + 'returned SUCCESS with `summary.failed = 0`. ⛔ NOT losslessly convertible: hoisting a ' + + 'node out of a region is a GRAPH REWRITE — new edges, a changed exit, sometimes a deleted ' + + 'container — and which of several shapes the author meant is an intent no artifact ' + + 'records, so a transform that picked one would be inventing the design. That leaves D3, a ' + + 'structured TODO naming each node to edit. ⚠️ Two boundaries this refusal deliberately ' + + 'does NOT reach, because a parse cannot: a pausing node type contributed by a PLUGIN ' + + '(ADR-0018 left the node-type namespace open and a parse has no registry), and a region ' + + 'nested past `MAX_REGION_DEPTH` (32), where the walk stops. For both, the engine\'s ' + + 'run-time refusal is still the only one — unchanged by this step, not fixed by it.', + acceptanceCriteria: + 'No `screen` / `wait` / `subflow` / `map` / `approval` / `approval_revise` node and no ' + + '`end` node sits inside any `loop` body, `parallel` branch or `try_catch` try/catch ' + + 'region in the stack, at any depth. `FlowSchema.parse` (and therefore `defineFlow`, ' + + '`registerFlow`, `os validate` and a Studio publish) accepts the stack: a node still ' + + 'nested is refused with the node AND the region named in one message ' + + "(`A \\`map\\` node may not sit inside a structured region — \\`loop 'sweep' body → " + + "try_catch 'guard' try\\` is a region body …`), anchored at " + + '`nodes[i].config.body.nodes[j].type` so a designer can jump to it. Behaviour to re-check ' + + 'after editing, because the fix CHANGES IT deliberately: a sweep that used to report ' + + '`success` having processed nothing now processes its items, so anything downstream that ' + + 'had quietly stopped receiving work starts receiving it again, and any alerting tuned to ' + + 'the empty-but-green runs will see real volume. A region-nested `end` was a no-op, so ' + + 'moving it to the top level makes the run actually TERMINATE there — check that the nodes ' + + 'after the container were not relying on continuing past it.', + }, { id: 'sys-account-issuer-retired', surface: '`sys_account.issuer` — the column, its `{ fields: [\'issuer\', \'account_id\'], unique: ' From 87973cab8d16e058d07cfb08d05d49f3efbe2d6f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 18 Sep 2026 06:19:46 +0000 Subject: [PATCH 3/6] test(service-automation): re-home #15788's region `end` case to the parse refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerFlow` parses through `FlowSchema` (`canonicalizeStoredFlow`), so the region-nested refusing `end` this case registered can no longer be registered at all — the refusal it asserted at the region boundary is now met one door earlier, at load. The fixture is unchanged and the case still fails the day the shape becomes declarable again; what it no longer covers (`runRegion`'s `isRefusalSignal` arm, now reachable only past `MAX_REGION_DEPTH`) is stated in the docblock rather than left to be discovered. The changeset's scope line said `packages/services` is untouched and the run-time refusal stays exactly as it was. Measured: registration and the ADR-0087 stored-row rehydration seam both parse, so a stored row carrying a refused shape stops loading, and the `end` arm's run-time refusal has no other caller. Corrected in place. Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude --- ...structured-region-pause-and-end-refused.md | 2 +- .../src/end-node-refused-outcome.test.ts | 92 ++++++++++++------- 2 files changed, 58 insertions(+), 36 deletions(-) diff --git a/.changeset/15646-structured-region-pause-and-end-refused.md b/.changeset/15646-structured-region-pause-and-end-refused.md index 549826a6afa..67315644cf9 100644 --- a/.changeset/15646-structured-region-pause-and-end-refused.md +++ b/.changeset/15646-structured-region-pause-and-end-refused.md @@ -35,4 +35,4 @@ The one-line fix is always the same: **move the node onto the top-level graph an **⚠️ Two boundaries this refusal does not reach, stated rather than discovered.** A pausing node type contributed by a **plugin** is not refused: ADR-0018 left the node-type namespace open and a parse has no registry. A region nested past **`MAX_REGION_DEPTH` (32)** is not judged: the parse walk stops there, and unlike a duplicate node id there is no second spec refusal behind it. For both, the engine's run-time refusal is the only one — unchanged by this change, and not fixed by it. -⛔ `packages/services` is untouched. The engine's run-time refusal stays exactly as it was; this moves the refusal to where the author is standing, it does not replace it. +⛔ No engine source is edited. What the refusal does to the run time is stated rather than left to be discovered: `AutomationEngine.registerFlow` and the ADR-0087 stored-row rehydration seam both go through `FlowSchema.parse` (`canonicalizeStoredFlow`), so a flow carrying a refused shape no longer registers or rehydrates — it is met at LOAD, not at the region boundary, and a stored row that carries one stops loading until it is rewritten. The engine's own run-time refusals for these shapes stay in place but are reachable only through the two boundaries above; for the `end` arm those are the only remaining path, because the refusal signal it answers is raised at exactly one site — an `end` node whose `outcome` is `refused`. diff --git a/packages/services/service-automation/src/end-node-refused-outcome.test.ts b/packages/services/service-automation/src/end-node-refused-outcome.test.ts index 6928d4bf661..56935aec6d5 100644 --- a/packages/services/service-automation/src/end-node-refused-outcome.test.ts +++ b/packages/services/service-automation/src/end-node-refused-outcome.test.ts @@ -384,48 +384,70 @@ describe('#15788 — the region boundary, made loud', () => { * throw as the try region FAILING: the author's refusal would run the catch * handler and the run would still record `completed`. * - * ⛔ Nothing an author had is narrowed. Before #15788 an `end` inside a - * region was a no-op whatever its `outcome`, so this shape has never once - * been honoured; whether a refusal should instead propagate out of a region - * is a real question the #14945 ruling does not answer. + * ⚠️ #15646 moved that refusal one door EARLIER, and this case moved with + * it. `FlowSchema` now refuses an `end` node inside any ADR-0031 region + * body at parse, and `registerFlow` parses (`canonicalizeStoredFlow` → + * `FlowSchema.parse`), so the fixture below no longer registers at all and + * the run this case used to drive is unreachable. The fixture is + * deliberately unchanged, so this case still fails the day the shape + * becomes declarable again — a re-home, ⛔ not a deletion. + * + * ⚠️ What it no longer covers, stated rather than left to be discovered: + * `runRegion`'s own `isRefusalSignal` arm. `FlowRefusalSignal` is raised at + * exactly one site — an `end` node whose parsed `outcome` is `refused` — + * and the class is not exported, so with the parse refusal in place no + * authored flow reaches that arm except past `MAX_REGION_DEPTH` (32), + * where the spec's region walk stops. The engine code stays as the refusal + * for that one remaining seam, and nothing in this package exercises it. + * + * ⛔ Nothing an author had is narrowed by either door. Before #15788 an + * `end` inside a region was a no-op whatever its `outcome`, so this shape + * has never once been honoured; whether a refusal should instead propagate + * out of a region is a real question the #14945 ruling does not answer. */ - it('a refusing `end` inside a `loop` body fails the run loudly instead of vanishing', async () => { + it('a refusing `end` inside a `loop` body is refused at REGISTRATION, before any run', () => { const { engine } = engineWithStore(); installBuiltinNodes(engine, { logger: createTestLogger(), getService: () => undefined } as never); - engine.registerFlow('in_region', { - name: 'in_region', label: 'in_region', type: 'autolaunched', - successMessage: SUCCESS_TEXT, - nodes: [ - { id: 'start', type: 'start', label: 'Start' }, - { - id: 'sweep', type: 'loop', label: 'Sweep', - config: { - collection: '{items}', - iteratorVariable: 'item', - body: { - nodes: [{ id: 'nope', type: 'end', label: 'Nope', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }], - edges: [], + + let caught: unknown; + try { + engine.registerFlow('in_region', { + name: 'in_region', label: 'in_region', type: 'autolaunched', + successMessage: SUCCESS_TEXT, + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { + id: 'sweep', type: 'loop', label: 'Sweep', + config: { + collection: '{items}', + iteratorVariable: 'item', + body: { + nodes: [{ id: 'nope', type: 'end', label: 'Nope', config: { outcome: 'refused', message: REFUSAL_TEMPLATE } }], + edges: [], + }, }, }, - }, - ], - edges: [{ id: 'e0', source: 'start', target: 'sweep' }], - } as never); - - // `items` rides on the trigger record, which the engine flattens into - // the variable map — so `{items}` resolves without declaring an input. - const result = await engine.execute('in_region', { - event: 'manual', object: 'account', record: { ...ACME, items: [1] }, - } as unknown as AutomationContext); + ], + edges: [{ id: 'e0', source: 'start', target: 'sweep' }], + } as never); + } catch (err) { + caught = err; + } - expect(result.success).toBe(false); - expect(result.status).toBe('failed'); + // Refused, and refused by the REGION rule specifically — a bare "it + // threw" would be satisfied by any unrelated parse error. + const issues = (caught as { issues?: Array<{ path: Array; message: string }> } | undefined)?.issues ?? []; + const refusal = issues.find((i) => i.message.includes('may not sit inside a structured region')); + expect(refusal, `no region refusal among ${JSON.stringify(issues)}`).toBeDefined(); + // Anchored where the author wrote the node, not on the container. + expect(refusal!.path).toEqual(['nodes', 1, 'config', 'body', 'nodes', 0, 'type']); // The message names the shape and the one-line fix, per "absence must // be loud" — ⛔ not a bare stringified sentinel. - expect(result.error).toContain('structured region'); - expect(result.error).toContain("outcome: 'refused'"); - // ⛔ And it is NOT recorded as a refusal: the run did not refuse, the - // engine declined to honour a shape it cannot express. - expect(result.refusalMessage).toBeUndefined(); + expect(refusal!.message).toContain('An `end` node may not sit inside a structured region'); + expect(refusal!.message).toContain('Put the `end` on the top-level graph'); + + // …and the refusal is terminal: nothing was registered, so no later + // call can reach the shape by another door. + expect(engine.getFlowVersionHistory('in_region')).toEqual([]); }); }); From 247845c11ae923466d043790b2c956f8fab990e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 18 Sep 2026 10:01:07 +0000 Subject: [PATCH 4/6] fix(spec)!: narrow the region-body pause refusal to the unconditional types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ruling batch #153 item 1, letter D: inside `loop` / `parallel` branch / `try_catch` bodies at any depth the refused population is `screen`, `wait`, `approval`, `approval_revise` and `end`. `map` and `subflow` are not refused by type — they pause exactly when the child flow `config.flowName` names pauses, a record this parse does not hold, so a type-keyed refusal would also refuse `loop { map(synchronous child) }`, a shape that runs correctly. `FLOW_PAUSE_CAPABLE_NODE_TYPES` (unreleased, added on this branch) becomes `FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES` so the exported name states the population the rule keys on rather than a capability list two of whose members it does not judge. Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude --- ...structured-region-pause-and-end-refused.md | 22 ++-- .../flow-region-pause-and-end.test.ts | 120 +++++++++++++----- packages/spec/src/automation/flow.zod.ts | 85 +++++++------ ...tured-region-body-pause-and-end-refused.ts | 100 ++++++++------- 4 files changed, 200 insertions(+), 127 deletions(-) diff --git a/.changeset/15646-structured-region-pause-and-end-refused.md b/.changeset/15646-structured-region-pause-and-end-refused.md index 67315644cf9..3ebe59c7683 100644 --- a/.changeset/15646-structured-region-pause-and-end-refused.md +++ b/.changeset/15646-structured-region-pause-and-end-refused.md @@ -2,37 +2,41 @@ '@objectstack/spec': minor --- -**BREAKING for authored metadata** — an ADR-0031 structured region body (`loop.config.body`, a `parallel` branch, `try_catch`'s `try` / `catch`) now refuses two node populations at parse: a node whose TYPE can durably pause, and an `end` node (#15646, absorbing #18112). +**BREAKING for authored metadata** — an ADR-0031 structured region body (`loop.config.body`, a `parallel` branch, `try_catch`'s `try` / `catch`) now refuses two node populations at parse: a node whose TYPE parks the run on every execution, and an `end` node (#15646, absorbing #18112). -`Clause-②: yes (narrowing)` — the flow accept set shrinks. Both refusals are the authoring-time enforcement of a limit the engine already holds at run time and #3267 ruled 禁: **a region body runs synchronously inside the enclosing run, so it can neither park that run nor terminate it.** +Clause-②: yes + +The flow accept set shrinks for five node types inside region bodies — shapes the runtime never honoured. Both refusals are the authoring-time enforcement of a limit the engine already holds at run time and #3267 ruled 禁: **a region body runs synchronously inside the enclosing run, so it can neither park that run nor terminate it.** ``` -✗ nodes.1.config.body.nodes.0.type: A `map` node may not sit inside a structured region — - `loop 'sweep' body → try_catch 'guard' try` is a region body and the `map` node `per_item` +✗ nodes.1.config.body.nodes.0.type: A `approval` node may not sit inside a structured region — + `loop 'sweep' body → try_catch 'guard' try` is a region body and the `approval` node `sign_off` is inside it. A region body runs synchronously and cannot durably pause … ``` **What is refused** -- **A pause-capable node** — `screen`, `wait`, `subflow`, `map`, `approval`, `approval_revise`, the six built-in types whose shipped executor declares `supportsPause: true`, published as `FLOW_PAUSE_CAPABLE_NODE_TYPES`. +- **A node that pauses on EVERY execution** — `screen`, `wait`, `approval`, `approval_revise`. - **An `end` node**, whatever its `outcome`. An `end` in a region was a no-op, and a refusing one was converted into a region error at the boundary; neither is what the author wrote. -**Why it was silent, measured.** The engine converts a suspension raised inside a region into an error — but the executor has already written its progress state into the ENCLOSING scope by then. Contain that error in a `try_catch` and the residue is read back as progress by the next entry to the same node. On a real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations × 2 items: not one item's subflow completed, only two of three iterations reached the catch, and iteration 3 read `started === collection.length`, ran nothing, and returned `success` with `summary.failed = 0`. A sweep that reports green having processed nothing is the worst available failure, and it is reachable only because the shape can be declared at all. +**⛔ What is deliberately NOT refused: `subflow` and `map`.** Their shipped executors also declare `supportsPause: true`, but they pause exactly when the child flow their `config.flowName` names pauses — a **different metadata record**, not in hand while this flow is parsed. Refusing them by type would also refuse `loop { map(synchronous child) }`, a shape that runs correctly today and is covered by an existing regression suite. A parse-time rule refuses what is statically wrong; a region-contained node that actually suspends is a fact only the run holds. **Nothing an author wrote with a region-nested `map` or `subflow` needs editing for this release.** + +**Why it was silent, measured.** The engine converts a suspension raised inside a region into an error — but the executor has already written its progress state into the ENCLOSING scope by then. Contain that error in a `try_catch` and the residue is read back as progress by the next entry to the same node. On a real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations × 2 items: not one item's subflow completed, only two of three iterations reached the catch, and iteration 3 read `started === collection.length`, ran nothing, and returned `success` with `summary.failed = 0`. ⚠️ Read that for the MECHANISM, not for this change's reach — the shape it was measured on is a `map`, and making that run's refusal loud is a separate change to the automation engine, not this one. ### Migration — FROM → TO | You wrote | Write instead | | --- | --- | | `loop { body: [ …, end ] }` | `loop { body: [ … ] } → end` — give the region a normal exit and put the terminator, with its `outcome` / `message`, on the top-level graph | -| `loop { body: [ try_catch { try: [ map ] } ] }` | a top-level `map` — its per-item subflow already iterates, so the enclosing `loop` is usually redundant, and the `try_catch` that existed only to contain the region's refusal goes with it | +| `loop { body: [ wait ] }` | a top-level `wait`, with the top-level graph as the repeating construct — a region body cannot park the run, so the nested form never waited | | `parallel { branches: [ [ approval ] , … ] }` | put the `approval` on the top-level graph and fan out around it, or split the branch's pausing half into a `subflow` the top-level graph calls | The one-line fix is always the same: **move the node onto the top-level graph and route the region's exit to it.** ⛔ Not mechanically convertible — hoisting a node out of a region is a graph rewrite (new edges, a changed exit, sometimes a deleted container) and which shape the author meant is an intent no artifact records, so this ships as an ADR-0087 D3 structured TODO rather than a D2 conversion. -**⚠️ Wider than the runs that actually broke, deliberately.** The rule judges the node TYPE. A `map` or `subflow` pauses exactly when the child flow it names pauses, so a region-nested `map` over a synchronous child ran green before and is refused now. That shape's legality lived in a DIFFERENT metadata record and could be revoked by editing that record — "legal until somebody adds a `wait` to the child flow" is not a contract an author can rely on, and a parse of one flow cannot answer it. - **⚠️ Two boundaries this refusal does not reach, stated rather than discovered.** A pausing node type contributed by a **plugin** is not refused: ADR-0018 left the node-type namespace open and a parse has no registry. A region nested past **`MAX_REGION_DEPTH` (32)** is not judged: the parse walk stops there, and unlike a duplicate node id there is no second spec refusal behind it. For both, the engine's run-time refusal is the only one — unchanged by this change, and not fixed by it. ⛔ No engine source is edited. What the refusal does to the run time is stated rather than left to be discovered: `AutomationEngine.registerFlow` and the ADR-0087 stored-row rehydration seam both go through `FlowSchema.parse` (`canonicalizeStoredFlow`), so a flow carrying a refused shape no longer registers or rehydrates — it is met at LOAD, not at the region boundary, and a stored row that carries one stops loading until it is rewritten. The engine's own run-time refusals for these shapes stay in place but are reachable only through the two boundaries above; for the `end` arm those are the only remaining path, because the refusal signal it answers is raised at exactly one site — an `end` node whose `outcome` is `refused`. + +**Published surface.** `FLOW_PAUSE_CAPABLE_NODE_TYPES` was never released; this change publishes `FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES` instead — the four types above — so the exported name states the population the rule actually keys on rather than a capability list two of whose members it does not judge. diff --git a/packages/spec/src/automation/flow-region-pause-and-end.test.ts b/packages/spec/src/automation/flow-region-pause-and-end.test.ts index 691e41e8701..3f851d4555c 100644 --- a/packages/spec/src/automation/flow-region-pause-and-end.test.ts +++ b/packages/spec/src/automation/flow-region-pause-and-end.test.ts @@ -4,24 +4,35 @@ * What a structured region body may NOT contain — #15646, absorbing #18112. * * Director seat ruling 5714239196 (maintainer 「同意,其他也同意」) and its scope - * addition 5714981966 (maintainer 「146 同意」), quoted in the PR: an ADR-0031 - * region body (`loop` / `try_catch` / `parallel`) refuses a node that can - * durably pause, and refuses an `end` node — one rule family, one PR. + * addition 5714981966 (maintainer 「146 同意」) put the rule here; ruling + * 5724940095, decision batch #153 item 1 letter **D** (maintainer 「其他同意」), + * fixed its POPULATION and is quoted verbatim in the PR: + * + * 「inside `loop` / `parallel` branch / `try_catch` (try and catch) bodies at + * any depth, the node types `screen`, `wait`, `approval`, `approval_revise` + * and `end` are refused by `FlowSchema.superRefine`, the message naming the + * node and the region. `map` and `subflow` are ⛔ not refused by type.」 * * Both halves are authoring-time enforcement of the limit #3267 ruled 禁: a * region body runs synchronously inside the enclosing run, so it can neither * park that run nor terminate it. The engine already refuses both AT RUN TIME * (`durable pause inside a structured region … is not supported`, and the * #15788 refusing-`end` conversion) — this moves the refusal to where the - * author is standing. + * author is standing, for the half a parse can actually judge. + * + * ⭐ The `map` / `subflow` exclusion is load-bearing and has its own pins in + * "the declared boundaries" below: those two pause exactly when the child flow + * they name pauses, so a type-keyed refusal would also refuse + * `loop { map(synchronous child) }` — a shape that runs correctly today. The + * run-time half of ruling D is what meets them. * - * Every case here fails without the rule: the shapes below all parsed green - * before it. + * Every refusal case here fails without the rule: the shapes below all parsed + * green before it. */ import { describe, it, expect } from 'vitest'; import { FlowSchema, - FLOW_PAUSE_CAPABLE_NODE_TYPES, + FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES, defineFlow, type Flow, type FlowNode, @@ -73,25 +84,37 @@ const issuesOf = (flow: Flow): Array<[string, string]> => { return result.error.issues.map((i) => [i.path.join('.'), i.message]); }; -describe('FLOW_PAUSE_CAPABLE_NODE_TYPES — the declared set, and how it was derived', () => { - it('names the six built-in types whose shipped executor declares `supportsPause: true`', () => { - // Read off the descriptors, not recalled: `screen` / `wait` / `subflow` / - // `map` in `service-automation`'s builtins, `approval` / `approval_revise` - // in `plugin-approvals` — the same six the ADR-0044 `resumeAuthority` - // default-flip migration entry names in its own prose. - expect([...FLOW_PAUSE_CAPABLE_NODE_TYPES]).toEqual([ - 'screen', 'wait', 'subflow', 'map', 'approval', 'approval_revise', +describe('FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES — the declared set, and how it was derived', () => { + it('names the four built-in types that park a run on EVERY execution', () => { + // Read off the descriptors, not recalled. SIX shipped executors declare + // `supportsPause: true` — `screen` / `wait` / `subflow` / `map` in + // `service-automation`'s builtins, `approval` / `approval_revise` in + // `plugin-approvals`, the same six the ADR-0044 `resumeAuthority` + // default-flip migration entry names. Four of them pause from this flow's + // own text; those four are the region rule's population. + expect([...FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES]).toEqual([ + 'screen', 'wait', 'approval', 'approval_revise', ]); }); + it('⛔ excludes `subflow` and `map`, which are pause-capable but not unconditionally so', () => { + // Ruling D, verbatim: 「`map` and `subflow` are ⛔ not refused by type.」 + // They pause exactly when the child flow `config.flowName` names pauses — + // a different metadata record, unreadable from here. Pinned as a DECISION + // so re-adding either is an edit somebody makes on purpose, against the + // ruling, rather than a tidy-up that looks like completing a list. + expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).not.toContain('subflow'); + expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).not.toContain('map'); + }); + it('carries the approval node types by their declared constants, so a rename cannot desynchronise the two', () => { - expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).toContain(APPROVAL_NODE_TYPE); - expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).toContain(APPROVAL_REVISE_NODE_TYPE); + expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).toContain(APPROVAL_NODE_TYPE); + expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).toContain(APPROVAL_REVISE_NODE_TYPE); }); }); describe('a region body refuses a pause-capable node (#15646)', () => { - it.each(FLOW_PAUSE_CAPABLE_NODE_TYPES)('refuses a `%s` node in a loop body, anchored on its `type`', (type) => { + it.each(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES)('refuses a `%s` node in a loop body, anchored on its `type`', (type) => { expect(issuesOf(flowWith([loopOver([pausingNode(type)])]))).toEqual([[ 'nodes.1.config.body.nodes.0.type', expect.stringContaining( @@ -102,9 +125,9 @@ describe('a region body refuses a pause-capable node (#15646)', () => { }); it('refuses it in a try_catch TRY region', () => { - expect(issuesOf(flowWith([tryCatchOver([pausingNode('map')], [step('recover')])]))).toEqual([[ + expect(issuesOf(flowWith([tryCatchOver([pausingNode('wait')], [step('recover')])]))).toEqual([[ 'nodes.1.config.try.nodes.0.type', - expect.stringContaining("`try_catch 'guard' try` is a region body and the `map` node `pauser` is inside it") as unknown as string, + expect.stringContaining("`try_catch 'guard' try` is a region body and the `wait` node `pauser` is inside it") as unknown as string, ]]); }); @@ -122,20 +145,29 @@ describe('a region body refuses a pause-capable node (#15646)', () => { ]]); }); - it("refuses this card's own reproduction — `loop { try_catch { map } }` — with the chained region path", () => { - const issues = issuesOf(flowWith([loopOver([tryCatchOver([pausingNode('map')], [step('recover')])])])); + it('names the CHAINED region path when the regions nest — `loop { try_catch { approval } }`', () => { + // The nesting shape of this card's own reproduction, with a node type the + // parse can judge. The reproduction's own `map` is pinned as still + // declarable in "the declared boundaries" below — that is ruling D, not a + // hole in this assertion. + const issues = issuesOf(flowWith([loopOver([tryCatchOver([pausingNode('approval')], [step('recover')])])])); expect(issues.map(([path]) => path)).toEqual([ 'nodes.1.config.body.nodes.0.config.try.nodes.0.type', ]); expect(issues[0][1]).toContain("`loop 'sweep' body → try_catch 'guard' try` is a region body"); }); - it('says WHY on the node type rather than on the child flow — the sentence an author acts on', () => { - const [[, message]] = issuesOf(flowWith([loopOver([pausingNode('map')])])); + it('says WHY, and names the node and the region — the sentence an author acts on', () => { + const [[, message]] = issuesOf(flowWith([loopOver([pausingNode('wait')])])); expect(message).toContain('A region body runs synchronously and cannot durably pause'); + expect(message).toContain('parks the run on EVERY execution'); expect(message).toContain('reads back as progress'); - expect(message).toContain("Move the `map` node onto the top-level graph and route the region's exit to it"); - expect(message).toContain('`map` / `subflow` pause exactly when the child flow they name pauses'); + expect(message).toContain("Move the `wait` node onto the top-level graph and route the region's exit to it"); + // ⛔ The message must not advertise a population the rule does not refuse: + // naming `map` / `subflow` here would send an author hunting for a refusal + // that never fires. + expect(message).not.toContain('subflow'); + expect(message).not.toContain('`map`'); }); it('renders through formatZodError pointing INTO the region', () => { @@ -150,7 +182,7 @@ describe('a region body refuses a pause-capable node (#15646)', () => { it('defineFlow refuses it with the same anchored issue', () => { let caught: unknown; try { - defineFlow(flowWith([loopOver([pausingNode('subflow')])])); + defineFlow(flowWith([loopOver([pausingNode('screen')])])); } catch (error) { caught = error; } @@ -192,7 +224,7 @@ describe('a region body refuses an `end` node (#18112, absorbed into #15646)', ( describe('the rule does NOT over-reach', () => { it('accepts every pause-capable type on the TOP-LEVEL graph — a refusal that over-reaches is worse than the silence it replaces', () => { - for (const type of FLOW_PAUSE_CAPABLE_NODE_TYPES) { + for (const type of [...FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES, 'subflow', 'map']) { const flow = flowWith([pausingNode(type)], [{ id: 'e1', source: 'start', target: 'pauser' }]); expect(FlowSchema.safeParse(flow).success, type).toBe(true); } @@ -223,18 +255,38 @@ describe('the rule does NOT over-reach', () => { }); }); -describe('the two declared boundaries — measured, so they move deliberately', () => { +describe('the declared boundaries — measured, so they move deliberately', () => { + it.each(['map', 'subflow'])('⛔ does NOT refuse a `%s` in a region body — ruling D, letter for letter', (type) => { + // 「`map` and `subflow` are ⛔ not refused by type.」 Their pause lives in + // the child flow `config.flowName` names — a metadata record this parse + // does not hold — so a type-keyed refusal would also refuse + // `loop { map(synchronous child) }`, which runs correctly today and is + // covered by #15616's regression suite in `packages/services`. + expect(FlowSchema.safeParse(flowWith([loopOver([pausingNode(type)])])).success, type).toBe(true); + }); + + it("⛔ leaves this card's own reproduction declarable — `loop { try_catch { map } }` — and that is the ruling, not a hole", () => { + // The exact shape #15646 was filed on. It parses, deliberately: what makes + // it wrong is that the child flow pauses, which only the RUN knows. Ruling + // D's second half is a `domain:services` card that fails such a run with a + // named error instead of reporting `success` with `summary.failed = 0`. + // ⛔ Do not "fix" this pin by widening the parse — that is route A/C, both + // explicitly refused. + const flow = flowWith([loopOver([tryCatchOver([pausingNode('map')], [step('recover')])])]); + expect(FlowSchema.safeParse(flow).success).toBe(true); + }); + it('a PLUGIN-contributed pausing type is not refused: a parse has no registry (ADR-0018 open namespace)', () => { // ⚠️ Not an oversight and not a gap to quietly close: `FlowNodeSchema.type` // is a validated `string`, and a plugin registers pause-capable types at run // time. The engine's own run-time refusal is what meets this one. Extending - // `FLOW_PAUSE_CAPABLE_NODE_TYPES` is how a first-party type joins the rule — - // and this pin is what makes that an edit somebody makes on purpose. + // `FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES` is how a first-party type joins the + // rule — and this pin is what makes that an edit somebody makes on purpose. const flow = flowWith([loopOver([{ id: 'vendor', type: 'vendor_signature_pause', label: 'Sign' }])]); expect(FlowSchema.safeParse(flow).success).toBe(true); }); - it('the seam at MAX_REGION_DEPTH: a pause-capable node at nesting 32 is refused; at nesting 33 the parse does not judge it', () => { + it('the seam at MAX_REGION_DEPTH: an unconditionally pausing node at nesting 32 is refused; at nesting 33 the parse does not judge it', () => { // The walk this rule rides (`collectFlowGraphs`) stops at 32, the ceiling // `parseFlowNodeRegions` shares — the same measured boundary #16134's // one-id-space rule hands off at. Past it there is no second spec refusal @@ -242,7 +294,7 @@ describe('the two declared boundaries — measured, so they move deliberately', // so the engine's run-time refusal is the only one left. Stated in the // docblock and in the changeset rather than discovered by an author. const nestedTo = (nesting: number): Flow => { - let body: { nodes: FlowNode[]; edges: FlowEdge[] } = { nodes: [pausingNode('map')], edges: [] }; + let body: { nodes: FlowNode[]; edges: FlowEdge[] } = { nodes: [pausingNode('wait')], edges: [] }; for (let k = nesting - 1; k >= 1; k--) { body = { nodes: [{ id: `l${k}`, type: 'loop', label: `L${k}`, config: { collection: '{items}', body } }], edges: [] }; } @@ -255,7 +307,7 @@ describe('the two declared boundaries — measured, so they move deliberately', expect(atCeiling.success).toBe(false); if (atCeiling.success) return; expect(atCeiling.error.issues).toHaveLength(1); - expect(atCeiling.error.issues[0].message).toContain('A `map` node may not sit inside a structured region'); + expect(atCeiling.error.issues[0].message).toContain('A `wait` node may not sit inside a structured region'); expect(atCeiling.error.issues[0].path.slice(-2)).toEqual([0, 'type']); expect(FlowSchema.safeParse(nestedTo(33)).success).toBe(true); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 044bbbba129..cde5ee95b8b 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -69,39 +69,38 @@ export const FLOW_BUILTIN_NODE_TYPES: readonly string[] = FlowNodeAction.options export const FLOW_STRUCTURAL_NODE_TYPES: readonly string[] = ['start', 'end']; /** - * Built-in node types that can park a run on a **durable pause** (ADR-0019) — - * the vocabulary a structured region body may not contain. + * Built-in node types that park a run on a **durable pause** (ADR-0019) on + * EVERY execution — the vocabulary a structured region body may not contain. * - * A capability list, not a behaviour prediction. Each entry is a node type - * whose shipped executor declares `supportsPause: true`; whether a given node - * of that type pauses on a given run is decided elsewhere and, for two of - * them, in ANOTHER metadata record: `map` and `subflow` pause exactly when the - * child flow they name pauses, which this flow's own text cannot answer. That - * is why the region rule keys on the TYPE — a contract whose verdict depends - * on a record the author is not editing is not a contract, and "legal until - * somebody adds a `wait` to the child flow" is the authoring trap this refusal - * exists to remove. + * ⭐ The unconditional half of the pause-capable population, and that + * distinction is the whole rule. Six shipped executors declare + * `supportsPause: true` — read off the + * `defineActionDescriptor({... supportsPause: true ...})` literals rather than + * recalled: `screen` / `wait` / `subflow` / `map` in `service-automation`'s + * builtins and `approval` / `approval_revise` in `plugin-approvals`, the same + * six the ADR-0044 `resumeAuthority` default-flip migration entry names and + * `check:resume-authority-declared` scans. Four of them pause from THIS flow's + * own text, and those four are listed here. * - * Derived by reading the descriptors, not by recall: the shipped - * `defineActionDescriptor({... supportsPause: true ...})` literals are - * `screen` / `wait` / `subflow` / `map` in `service-automation`'s builtins and - * `approval` / `approval_revise` in `plugin-approvals`, which is the same set - * of six the ADR-0044 `resumeAuthority` default-flip migration entry names. - * `check:resume-authority-declared` is the gate that already scans exactly - * that population. + * ⛔ `subflow` and `map` are deliberately NOT listed. They pause exactly when + * the child flow their `config.flowName` names pauses — a DIFFERENT metadata + * record, not in hand while this flow is parsed — so refusing them by type also + * refuses `loop { map(synchronous child) }`, a shape that runs correctly today. + * A parse-time rule refuses what is STATICALLY wrong; a region-contained node + * that actually suspends is a fact only the run holds, and the engine's own + * refusal is what meets it. Adding either type back here is a ruled decision, + * not a fix — maintainer ruling, decision batch #153 item 1, letter D. * * ⚠️ **Not closed, and cannot be.** ADR-0018 made the node-type namespace open * — `FlowNodeSchema.type` is a validated `string` and a plugin registers new - * types, pause-capable ones included, at run time. A parse has no registry, so - * a plugin-contributed pausing type inside a region is NOT refused here; the + * types, pausing ones included, at run time. A parse has no registry, so a + * plugin-contributed pausing type inside a region is NOT refused here; the * engine's own run-time refusal is what meets it. Extending this list is how a * first-party type joins the rule. */ -export const FLOW_PAUSE_CAPABLE_NODE_TYPES: readonly string[] = [ +export const FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES: readonly string[] = [ 'screen', 'wait', - 'subflow', - 'map', APPROVAL_NODE_TYPE, APPROVAL_REVISE_NODE_TYPE, ]; @@ -341,7 +340,7 @@ export const FlowNodeSchema = lazySchema(() => flowNodeObject().transform( * * ⚠️ Since #15646 a `wait` nested in a region body meets an EARLIER refusal than * either of those, and it is not about this block: a region body cannot durably - * pause, so {@link FLOW_PAUSE_CAPABLE_NODE_TYPES} may not appear in one at all + * pause, so {@link FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES} may not appear in one at all * and the flow parse says so on the node's `type`. ⛔ Do not read the paragraph * above as "a nested block-less `wait` parses" — it no longer does, for a * different reason. The two-door reading it describes still governs every node @@ -1216,21 +1215,30 @@ export const FlowSchema = lazySchema(() => strictObject( // the same node: measured on `loop { try_catch { map(pausing child) } }`, // iteration 3 read `started === collection.length`, ran nothing, and // returned SUCCESS with `summary.failed = 0`. A sweep that reports green - // having done nothing is the worst available failure, and it is reachable - // only because the shape can be declared at all. + // having done nothing is the worst available failure. + // + // ⛔ Read that measurement for the MECHANISM, not for this rule's reach: + // the shape it was taken on is a `map`, and a `map` is not judged here (see + // below). What the parse removes is the half it can see — the node types + // that pause whatever any other record says. The measured shape itself is + // still declarable, and is met at RUN time; making that run-time refusal + // loud instead of silent is the `domain:services` half of the same ruling. // // END — an `end` inside a region is a no-op today whatever its `outcome`, // and a refusing one is converted into a region error at exactly the // boundary above (#15788). Neither is what the author wrote, so the shape // has never once been honoured. // - // Judged on the node TYPE, and the reason is in - // {@link FLOW_PAUSE_CAPABLE_NODE_TYPES}: for `map` / `subflow` whether a pause - // happens is decided by a DIFFERENT metadata record. Two boundaries, declared - // rather than discovered — a plugin-registered pausing type is invisible to a - // parse (ADR-0018's open namespace), and this walk stops at - // `MAX_REGION_DEPTH`, so a region nested past the ceiling is not judged here - // and only the engine's run-time refusal meets it. + // Judged on the node TYPE, and the population is the UNCONDITIONAL one — + // {@link FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES}. `map` and `subflow` are + // pause-capable but are ⛔ NOT judged here: whether they pause is decided by + // a DIFFERENT metadata record, so refusing them by type would also refuse + // `loop { map(synchronous child) }`, a shape that runs correctly. Three + // boundaries, declared rather than discovered — a region-contained node that + // durably suspends at RUN time (the `map` / `subflow` case, met by the + // engine at the region boundary), a plugin-registered pausing type (invisible to + // a parse, ADR-0018's open namespace), and a region nested past + // `MAX_REGION_DEPTH`, where this walk stops. for (const graph of collectFlowGraphs(flow)) { // The flow's own graph is the one place both are legal — that is the whole // prescription both messages give, so it must stay true. @@ -1253,23 +1261,20 @@ export const FlowSchema = lazySchema(() => strictObject( }); return; } - if (FLOW_PAUSE_CAPABLE_NODE_TYPES.includes(type)) { + if (FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES.includes(type)) { ctx.addIssue({ code: 'custom', path: [...graph.path, 'nodes', index, 'type'], message: `A \`${type}\` node may not sit inside a structured region — \`${graph.scope}\` is a region ` + `body and the \`${type}\` node ${named} is inside it. A region body runs synchronously and ` + - 'cannot durably pause, while `' + type + '` is one of the node types that can park a run ' + - '(`screen` / `wait` / `subflow` / `map` / `approval` / `approval_revise`). The engine ' + + 'cannot durably pause, while `' + type + '` parks the run on EVERY execution ' + + '(`screen` / `wait` / `approval` / `approval_revise`). The engine ' + 'refuses such a pause at the region boundary AFTER the node has written its progress state ' + 'into the enclosing scope, so a contained refusal leaves residue a later entry reads back as ' + 'progress — the run then reports success having processed nothing. Move the `' + type + '` ' + "node onto the top-level graph and route the region's exit to it; when the work must repeat " + - 'per item, make the top-level graph the repeating construct rather than nesting the pause. ' + - 'Judged on the node TYPE because `map` / `subflow` pause exactly when the child flow they ' + - 'name pauses, which is a different metadata record and can change without this flow being ' + - 'edited.', + 'per item, make the top-level graph the repeating construct rather than nesting the pause.', }); } }); diff --git a/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts b/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts index 6a3c17bc98b..ca15d6afb61 100644 --- a/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts +++ b/packages/spec/src/migrations/entries/semantic/18.structured-region-body-pause-and-end-refused.ts @@ -7,60 +7,72 @@ export const entry: SemanticMigration = { surface: 'The BODY of every ADR-0031 structured region — `loop.config.body`, each ' + '`parallel.config.branches[]`, and `try_catch.config.try` / `.catch` — at every depth the ' - + 'flow parse walks. Two node populations become undeclarable there: a node whose TYPE can ' - + 'durably pause (`screen`, `wait`, `subflow`, `map`, `approval`, `approval_revise`) and an ' - + '`end` node, whatever its `outcome`. ⚠️ Judged on the node TYPE, which is WIDER than the ' - + 'runs that actually broke: a `map` or `subflow` pauses exactly when the child flow it ' - + 'names pauses, so a region-nested `map` over a synchronous child ran green and is refused ' - + 'now. That is deliberate — the legality of the old shape lived in a DIFFERENT metadata ' - + 'record and could be revoked by editing that record, which is not a contract an author ' - + 'can rely on.', + + 'flow parse walks. Two node populations become undeclarable there: a node whose TYPE parks ' + + 'the run on EVERY execution (`screen`, `wait`, `approval`, `approval_revise`) and an ' + + '`end` node, whatever its `outcome`. ⛔ `subflow` and `map` are NOT in the population, ' + + 'although their executors also declare `supportsPause: true`: they pause exactly when the ' + + 'child flow their `config.flowName` names pauses, which is a DIFFERENT metadata record and ' + + 'is not in hand while this flow is parsed. Refusing them by type would also refuse ' + + '`loop { map(synchronous child) }`, a shape that runs correctly today, so a region-nested ' + + '`map` or `subflow` still parses and is met at RUN time instead.', replacement: 'Move the node onto the TOP-LEVEL graph and route the region\'s exit to it. For an `end`: ' + 'delete it from the body, give the region a normal exit, and put the terminator (with its ' + '`outcome` / `message`) on the top-level graph — `loop { body: [ …, end ] }` becomes ' + '`loop { body: [ … ] } → end`. For a pausing node: hoist it out of the container — ' - + '`loop { body: [ try_catch { try: [ map ] } ] }` becomes a top-level `map` (its own ' - + 'per-item subflow already iterates, so the enclosing `loop` is usually redundant), and ' - + 'where the repetition is genuinely needed, make the TOP-LEVEL graph the repeating ' - + 'construct with the pause on it rather than nesting the pause inside a region. A ' - + '`try_catch` whose only purpose was to contain the region\'s refusal has nothing left to ' - + 'contain and is deleted with it.', + + '`loop { body: [ try_catch { try: [ approval ] } ] }` becomes a top-level `approval` with ' + + 'the loop fanning out around it, or the pausing half of the branch is split into a ' + + '`subflow` the top-level graph calls; where the repetition is genuinely needed, make the ' + + 'TOP-LEVEL graph the repeating construct with the pause on it rather than nesting the ' + + 'pause inside a region. A `try_catch` whose only purpose was to contain the region\'s ' + + 'refusal has nothing left to contain and is deleted with it.', reason: 'Maintainer ruling, decision batch #145 item 5, verbatim and untranslated: 「同意,其他也同' + '意」, carrying the presented option C; extended by batch #146 「146 同意」, which attached ' + 'the `end` half (the absorbed #18112) and recorded that #3267 is ruled 禁 — structured ' + 'regions do not support durable pause and a region body cannot terminate the run, so this ' - + 'is that limit\'s authoring-time enforcement rather than an interim. The refusal already ' - + 'existed AT RUN TIME and said nothing an author could act on: the engine converts a ' - + 'suspension raised inside a region into an error at the region boundary, AFTER the ' - + 'executor has written its progress state into the enclosing scope, so a `try_catch` that ' - + 'contains that error leaves residue the next entry reads back as progress. Measured on a ' - + 'real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations x ' - + '2 items: not one item\'s subflow ever completed, only two of three iterations reached ' - + 'the catch, and iteration 3 read `started === collection.length`, ran nothing, and ' - + 'returned SUCCESS with `summary.failed = 0`. ⛔ NOT losslessly convertible: hoisting a ' - + 'node out of a region is a GRAPH REWRITE — new edges, a changed exit, sometimes a deleted ' - + 'container — and which of several shapes the author meant is an intent no artifact ' - + 'records, so a transform that picked one would be inventing the design. That leaves D3, a ' - + 'structured TODO naming each node to edit. ⚠️ Two boundaries this refusal deliberately ' - + 'does NOT reach, because a parse cannot: a pausing node type contributed by a PLUGIN ' - + '(ADR-0018 left the node-type namespace open and a parse has no registry), and a region ' - + 'nested past `MAX_REGION_DEPTH` (32), where the walk stops. For both, the engine\'s ' - + 'run-time refusal is still the only one — unchanged by this step, not fixed by it.', + + 'is that limit\'s authoring-time enforcement rather than an interim. The POPULATION was ' + + 'then fixed by decision batch #153 item 1, letter D (maintainer 「其他同意」): 「inside ' + + '`loop` / `parallel` branch / `try_catch` (try and catch) bodies at any depth, the node ' + + 'types `screen`, `wait`, `approval`, `approval_revise` and `end` are refused by ' + + '`FlowSchema.superRefine` … `map` and `subflow` are ⛔ not refused by type.」 A parse-time ' + + 'rule refuses what is STATICALLY wrong; refusing `map` / `subflow` by type would refuse a ' + + 'correct working shape on a guess about another record. The refusal already existed AT RUN ' + + 'TIME and said nothing an author could act on: the engine converts a suspension raised ' + + 'inside a region into an error at the region boundary, AFTER the executor has written its ' + + 'progress state into the enclosing scope, so a `try_catch` that contains that error leaves ' + + 'residue the next entry reads back as progress. Measured on a real `AutomationEngine`, ' + + '`loop { try_catch { map(pausing child) } }` over 3 iterations x 2 items: not one item\'s ' + + 'subflow ever completed, only two of three iterations reached the catch, and iteration 3 ' + + 'read `started === collection.length`, ran nothing, and returned SUCCESS with ' + + '`summary.failed = 0`. ⚠️ Read that measurement for the MECHANISM: the shape it was taken ' + + 'on is a `map`, which this parse rule deliberately does not reach — making the run-time ' + + 'refusal of a region-contained node that durably suspends LOUD is the second half of ' + + 'ruling D and ships as its own `domain:services` change. ⛔ NOT losslessly convertible: ' + + 'hoisting a node out of a region is a GRAPH REWRITE — new edges, a changed exit, sometimes ' + + 'a deleted container — and which of several shapes the author meant is an intent no ' + + 'artifact records, so a transform that picked one would be inventing the design. That ' + + 'leaves D3, a structured TODO naming each node to edit. ⚠️ Two further boundaries this ' + + 'refusal deliberately does NOT reach, because a parse cannot: a pausing node type ' + + 'contributed by a PLUGIN (ADR-0018 left the node-type namespace open and a parse has no ' + + 'registry), and a region nested past `MAX_REGION_DEPTH` (32), where the walk stops. For ' + + 'both, the engine\'s run-time refusal is still the only one — unchanged by this step, not ' + + 'fixed by it.', acceptanceCriteria: - 'No `screen` / `wait` / `subflow` / `map` / `approval` / `approval_revise` node and no ' - + '`end` node sits inside any `loop` body, `parallel` branch or `try_catch` try/catch ' - + 'region in the stack, at any depth. `FlowSchema.parse` (and therefore `defineFlow`, ' - + '`registerFlow`, `os validate` and a Studio publish) accepts the stack: a node still ' - + 'nested is refused with the node AND the region named in one message ' - + "(`A \\`map\\` node may not sit inside a structured region — \\`loop 'sweep' body → " + 'No `screen` / `wait` / `approval` / `approval_revise` node and no `end` node sits inside ' + + 'any `loop` body, `parallel` branch or `try_catch` try/catch region in the stack, at any ' + + 'depth. `FlowSchema.parse` (and therefore `defineFlow`, `registerFlow`, `os validate` and ' + + 'a Studio publish) accepts the stack: a node still nested is refused with the node AND the ' + + 'region named in one message ' + + "(`A \\`approval\\` node may not sit inside a structured region — \\`loop 'sweep' body → " + "try_catch 'guard' try\\` is a region body …`), anchored at " - + '`nodes[i].config.body.nodes[j].type` so a designer can jump to it. Behaviour to re-check ' - + 'after editing, because the fix CHANGES IT deliberately: a sweep that used to report ' - + '`success` having processed nothing now processes its items, so anything downstream that ' - + 'had quietly stopped receiving work starts receiving it again, and any alerting tuned to ' - + 'the empty-but-green runs will see real volume. A region-nested `end` was a no-op, so ' - + 'moving it to the top level makes the run actually TERMINATE there — check that the nodes ' - + 'after the container were not relying on continuing past it.', + + '`nodes[i].config.body.nodes[j].type` so a designer can jump to it. ⛔ A region-nested ' + + '`map` or `subflow` is NOT part of this migration and needs no edit to load — if such a ' + + 'flow reports `success` having processed nothing, that is the run-time half of the same ' + + 'ruling and not a stack edit. Behaviour to re-check after editing, because the fix CHANGES ' + + 'IT deliberately: a region-nested `end` was a no-op, so moving it to the top level makes ' + + 'the run actually TERMINATE there — check that the nodes after the container were not ' + + 'relying on continuing past it. A hoisted `approval` / `wait` / `screen` now parks the run ' + + 'where the enclosing graph can see it, so anything that polled for the sweep to finish ' + + 'sees a suspended run instead of a green-but-empty one.', }; From 48f2065f6ea989f5408d4f2da8bac510cca78bc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 18 Sep 2026 10:21:56 +0000 Subject: [PATCH 5/6] fix(spec): keep the published constant name, narrow only its contents Seat decision on the conflict the rename surfaced: ruling D orders a `minor` for `@objectstack/spec`, and `check:api-surface` grades a removed export breaking, so the rename and the ruling cannot both stand. The ruling asks for a change to the refused POPULATION, not to the export's name. `FLOW_PAUSE_CAPABLE_NODE_TYPES` keeps its identifier and its place in `api-surface/automation.json`; only its contents narrow to the four types that pause unconditionally. The docblock now leads with "read the contents, not the name" and states why the name is kept, so the mismatch is declared rather than discovered. `src/migrations/registry.ts` is regenerated from the edited ADR-0087 entry. Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude --- ...structured-region-pause-and-end-refused.md | 2 +- .../flow-region-pause-and-end.test.ts | 20 ++-- packages/spec/src/automation/flow.zod.ts | 20 ++-- packages/spec/src/migrations/registry.ts | 100 ++++++++++-------- 4 files changed, 81 insertions(+), 61 deletions(-) diff --git a/.changeset/15646-structured-region-pause-and-end-refused.md b/.changeset/15646-structured-region-pause-and-end-refused.md index 3ebe59c7683..7b62c00f4ab 100644 --- a/.changeset/15646-structured-region-pause-and-end-refused.md +++ b/.changeset/15646-structured-region-pause-and-end-refused.md @@ -39,4 +39,4 @@ The one-line fix is always the same: **move the node onto the top-level graph an ⛔ No engine source is edited. What the refusal does to the run time is stated rather than left to be discovered: `AutomationEngine.registerFlow` and the ADR-0087 stored-row rehydration seam both go through `FlowSchema.parse` (`canonicalizeStoredFlow`), so a flow carrying a refused shape no longer registers or rehydrates — it is met at LOAD, not at the region boundary, and a stored row that carries one stops loading until it is rewritten. The engine's own run-time refusals for these shapes stay in place but are reachable only through the two boundaries above; for the `end` arm those are the only remaining path, because the refusal signal it answers is raised at exactly one site — an `end` node whose `outcome` is `refused`. -**Published surface.** `FLOW_PAUSE_CAPABLE_NODE_TYPES` was never released; this change publishes `FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES` instead — the four types above — so the exported name states the population the rule actually keys on rather than a capability list two of whose members it does not judge. +**Published surface.** `FLOW_PAUSE_CAPABLE_NODE_TYPES` is published with the four types above. ⚠️ Read its contents, not its name: it is the UNCONDITIONALLY pausing set, not every type that can pause — `subflow` and `map` declare `supportsPause: true` and are deliberately absent, for the reason above. The identifier is unchanged, so this release removes no export. diff --git a/packages/spec/src/automation/flow-region-pause-and-end.test.ts b/packages/spec/src/automation/flow-region-pause-and-end.test.ts index 3f851d4555c..8ed2314ff5e 100644 --- a/packages/spec/src/automation/flow-region-pause-and-end.test.ts +++ b/packages/spec/src/automation/flow-region-pause-and-end.test.ts @@ -32,7 +32,7 @@ import { describe, it, expect } from 'vitest'; import { FlowSchema, - FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES, + FLOW_PAUSE_CAPABLE_NODE_TYPES, defineFlow, type Flow, type FlowNode, @@ -84,7 +84,7 @@ const issuesOf = (flow: Flow): Array<[string, string]> => { return result.error.issues.map((i) => [i.path.join('.'), i.message]); }; -describe('FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES — the declared set, and how it was derived', () => { +describe('FLOW_PAUSE_CAPABLE_NODE_TYPES — the declared set, and how it was derived', () => { it('names the four built-in types that park a run on EVERY execution', () => { // Read off the descriptors, not recalled. SIX shipped executors declare // `supportsPause: true` — `screen` / `wait` / `subflow` / `map` in @@ -92,7 +92,7 @@ describe('FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES — the declared set, and how it w // `plugin-approvals`, the same six the ADR-0044 `resumeAuthority` // default-flip migration entry names. Four of them pause from this flow's // own text; those four are the region rule's population. - expect([...FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES]).toEqual([ + expect([...FLOW_PAUSE_CAPABLE_NODE_TYPES]).toEqual([ 'screen', 'wait', 'approval', 'approval_revise', ]); }); @@ -103,18 +103,18 @@ describe('FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES — the declared set, and how it w // a different metadata record, unreadable from here. Pinned as a DECISION // so re-adding either is an edit somebody makes on purpose, against the // ruling, rather than a tidy-up that looks like completing a list. - expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).not.toContain('subflow'); - expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).not.toContain('map'); + expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).not.toContain('subflow'); + expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).not.toContain('map'); }); it('carries the approval node types by their declared constants, so a rename cannot desynchronise the two', () => { - expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).toContain(APPROVAL_NODE_TYPE); - expect(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES).toContain(APPROVAL_REVISE_NODE_TYPE); + expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).toContain(APPROVAL_NODE_TYPE); + expect(FLOW_PAUSE_CAPABLE_NODE_TYPES).toContain(APPROVAL_REVISE_NODE_TYPE); }); }); describe('a region body refuses a pause-capable node (#15646)', () => { - it.each(FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES)('refuses a `%s` node in a loop body, anchored on its `type`', (type) => { + it.each(FLOW_PAUSE_CAPABLE_NODE_TYPES)('refuses a `%s` node in a loop body, anchored on its `type`', (type) => { expect(issuesOf(flowWith([loopOver([pausingNode(type)])]))).toEqual([[ 'nodes.1.config.body.nodes.0.type', expect.stringContaining( @@ -224,7 +224,7 @@ describe('a region body refuses an `end` node (#18112, absorbed into #15646)', ( describe('the rule does NOT over-reach', () => { it('accepts every pause-capable type on the TOP-LEVEL graph — a refusal that over-reaches is worse than the silence it replaces', () => { - for (const type of [...FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES, 'subflow', 'map']) { + for (const type of [...FLOW_PAUSE_CAPABLE_NODE_TYPES, 'subflow', 'map']) { const flow = flowWith([pausingNode(type)], [{ id: 'e1', source: 'start', target: 'pauser' }]); expect(FlowSchema.safeParse(flow).success, type).toBe(true); } @@ -280,7 +280,7 @@ describe('the declared boundaries — measured, so they move deliberately', () = // ⚠️ Not an oversight and not a gap to quietly close: `FlowNodeSchema.type` // is a validated `string`, and a plugin registers pause-capable types at run // time. The engine's own run-time refusal is what meets this one. Extending - // `FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES` is how a first-party type joins the + // `FLOW_PAUSE_CAPABLE_NODE_TYPES` is how a first-party type joins the // rule — and this pin is what makes that an edit somebody makes on purpose. const flow = flowWith([loopOver([{ id: 'vendor', type: 'vendor_signature_pause', label: 'Sign' }])]); expect(FlowSchema.safeParse(flow).success).toBe(true); diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index cde5ee95b8b..12360e7a511 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -69,8 +69,16 @@ export const FLOW_BUILTIN_NODE_TYPES: readonly string[] = FlowNodeAction.options export const FLOW_STRUCTURAL_NODE_TYPES: readonly string[] = ['start', 'end']; /** - * Built-in node types that park a run on a **durable pause** (ADR-0019) on - * EVERY execution — the vocabulary a structured region body may not contain. + * The node types a structured region body may not contain — the population the + * `FlowSchema.superRefine` region rule below keys on. + * + * ⚠️ **Read the contents, not the name.** This is NOT every type that can + * pause; it is the four that pause UNCONDITIONALLY, on every execution, decided + * by THIS flow's own text. The name predates that narrowing and is kept + * deliberately: the identifier is in `api-surface/automation.json`, and + * `check:api-surface` grades a removed export breaking — trading a whole-stack + * major for a better name is a ruled decision of its own, ⛔ not a tidy-up to + * make in passing. * * ⭐ The unconditional half of the pause-capable population, and that * distinction is the whole rule. Six shipped executors declare @@ -98,7 +106,7 @@ export const FLOW_STRUCTURAL_NODE_TYPES: readonly string[] = ['start', 'end']; * engine's own run-time refusal is what meets it. Extending this list is how a * first-party type joins the rule. */ -export const FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES: readonly string[] = [ +export const FLOW_PAUSE_CAPABLE_NODE_TYPES: readonly string[] = [ 'screen', 'wait', APPROVAL_NODE_TYPE, @@ -340,7 +348,7 @@ export const FlowNodeSchema = lazySchema(() => flowNodeObject().transform( * * ⚠️ Since #15646 a `wait` nested in a region body meets an EARLIER refusal than * either of those, and it is not about this block: a region body cannot durably - * pause, so {@link FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES} may not appear in one at all + * pause, so {@link FLOW_PAUSE_CAPABLE_NODE_TYPES} may not appear in one at all * and the flow parse says so on the node's `type`. ⛔ Do not read the paragraph * above as "a nested block-less `wait` parses" — it no longer does, for a * different reason. The two-door reading it describes still governs every node @@ -1230,7 +1238,7 @@ export const FlowSchema = lazySchema(() => strictObject( // has never once been honoured. // // Judged on the node TYPE, and the population is the UNCONDITIONAL one — - // {@link FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES}. `map` and `subflow` are + // {@link FLOW_PAUSE_CAPABLE_NODE_TYPES}. `map` and `subflow` are // pause-capable but are ⛔ NOT judged here: whether they pause is decided by // a DIFFERENT metadata record, so refusing them by type would also refuse // `loop { map(synchronous child) }`, a shape that runs correctly. Three @@ -1261,7 +1269,7 @@ export const FlowSchema = lazySchema(() => strictObject( }); return; } - if (FLOW_UNCONDITIONAL_PAUSE_NODE_TYPES.includes(type)) { + if (FLOW_PAUSE_CAPABLE_NODE_TYPES.includes(type)) { ctx.addIssue({ code: 'custom', path: [...graph.path, 'nodes', index, 'type'], diff --git a/packages/spec/src/migrations/registry.ts b/packages/spec/src/migrations/registry.ts index 04c10e48690..a483f7ce627 100644 --- a/packages/spec/src/migrations/registry.ts +++ b/packages/spec/src/migrations/registry.ts @@ -10896,62 +10896,74 @@ const step18: MigrationStep = { surface: 'The BODY of every ADR-0031 structured region — `loop.config.body`, each ' + '`parallel.config.branches[]`, and `try_catch.config.try` / `.catch` — at every depth the ' - + 'flow parse walks. Two node populations become undeclarable there: a node whose TYPE can ' - + 'durably pause (`screen`, `wait`, `subflow`, `map`, `approval`, `approval_revise`) and an ' - + '`end` node, whatever its `outcome`. ⚠️ Judged on the node TYPE, which is WIDER than the ' - + 'runs that actually broke: a `map` or `subflow` pauses exactly when the child flow it ' - + 'names pauses, so a region-nested `map` over a synchronous child ran green and is refused ' - + 'now. That is deliberate — the legality of the old shape lived in a DIFFERENT metadata ' - + 'record and could be revoked by editing that record, which is not a contract an author ' - + 'can rely on.', + + 'flow parse walks. Two node populations become undeclarable there: a node whose TYPE parks ' + + 'the run on EVERY execution (`screen`, `wait`, `approval`, `approval_revise`) and an ' + + '`end` node, whatever its `outcome`. ⛔ `subflow` and `map` are NOT in the population, ' + + 'although their executors also declare `supportsPause: true`: they pause exactly when the ' + + 'child flow their `config.flowName` names pauses, which is a DIFFERENT metadata record and ' + + 'is not in hand while this flow is parsed. Refusing them by type would also refuse ' + + '`loop { map(synchronous child) }`, a shape that runs correctly today, so a region-nested ' + + '`map` or `subflow` still parses and is met at RUN time instead.', replacement: 'Move the node onto the TOP-LEVEL graph and route the region\'s exit to it. For an `end`: ' + 'delete it from the body, give the region a normal exit, and put the terminator (with its ' + '`outcome` / `message`) on the top-level graph — `loop { body: [ …, end ] }` becomes ' + '`loop { body: [ … ] } → end`. For a pausing node: hoist it out of the container — ' - + '`loop { body: [ try_catch { try: [ map ] } ] }` becomes a top-level `map` (its own ' - + 'per-item subflow already iterates, so the enclosing `loop` is usually redundant), and ' - + 'where the repetition is genuinely needed, make the TOP-LEVEL graph the repeating ' - + 'construct with the pause on it rather than nesting the pause inside a region. A ' - + '`try_catch` whose only purpose was to contain the region\'s refusal has nothing left to ' - + 'contain and is deleted with it.', + + '`loop { body: [ try_catch { try: [ approval ] } ] }` becomes a top-level `approval` with ' + + 'the loop fanning out around it, or the pausing half of the branch is split into a ' + + '`subflow` the top-level graph calls; where the repetition is genuinely needed, make the ' + + 'TOP-LEVEL graph the repeating construct with the pause on it rather than nesting the ' + + 'pause inside a region. A `try_catch` whose only purpose was to contain the region\'s ' + + 'refusal has nothing left to contain and is deleted with it.', reason: 'Maintainer ruling, decision batch #145 item 5, verbatim and untranslated: 「同意,其他也同' + '意」, carrying the presented option C; extended by batch #146 「146 同意」, which attached ' + 'the `end` half (the absorbed #18112) and recorded that #3267 is ruled 禁 — structured ' + 'regions do not support durable pause and a region body cannot terminate the run, so this ' - + 'is that limit\'s authoring-time enforcement rather than an interim. The refusal already ' - + 'existed AT RUN TIME and said nothing an author could act on: the engine converts a ' - + 'suspension raised inside a region into an error at the region boundary, AFTER the ' - + 'executor has written its progress state into the enclosing scope, so a `try_catch` that ' - + 'contains that error leaves residue the next entry reads back as progress. Measured on a ' - + 'real `AutomationEngine`, `loop { try_catch { map(pausing child) } }` over 3 iterations x ' - + '2 items: not one item\'s subflow ever completed, only two of three iterations reached ' - + 'the catch, and iteration 3 read `started === collection.length`, ran nothing, and ' - + 'returned SUCCESS with `summary.failed = 0`. ⛔ NOT losslessly convertible: hoisting a ' - + 'node out of a region is a GRAPH REWRITE — new edges, a changed exit, sometimes a deleted ' - + 'container — and which of several shapes the author meant is an intent no artifact ' - + 'records, so a transform that picked one would be inventing the design. That leaves D3, a ' - + 'structured TODO naming each node to edit. ⚠️ Two boundaries this refusal deliberately ' - + 'does NOT reach, because a parse cannot: a pausing node type contributed by a PLUGIN ' - + '(ADR-0018 left the node-type namespace open and a parse has no registry), and a region ' - + 'nested past `MAX_REGION_DEPTH` (32), where the walk stops. For both, the engine\'s ' - + 'run-time refusal is still the only one — unchanged by this step, not fixed by it.', + + 'is that limit\'s authoring-time enforcement rather than an interim. The POPULATION was ' + + 'then fixed by decision batch #153 item 1, letter D (maintainer 「其他同意」): 「inside ' + + '`loop` / `parallel` branch / `try_catch` (try and catch) bodies at any depth, the node ' + + 'types `screen`, `wait`, `approval`, `approval_revise` and `end` are refused by ' + + '`FlowSchema.superRefine` … `map` and `subflow` are ⛔ not refused by type.」 A parse-time ' + + 'rule refuses what is STATICALLY wrong; refusing `map` / `subflow` by type would refuse a ' + + 'correct working shape on a guess about another record. The refusal already existed AT RUN ' + + 'TIME and said nothing an author could act on: the engine converts a suspension raised ' + + 'inside a region into an error at the region boundary, AFTER the executor has written its ' + + 'progress state into the enclosing scope, so a `try_catch` that contains that error leaves ' + + 'residue the next entry reads back as progress. Measured on a real `AutomationEngine`, ' + + '`loop { try_catch { map(pausing child) } }` over 3 iterations x 2 items: not one item\'s ' + + 'subflow ever completed, only two of three iterations reached the catch, and iteration 3 ' + + 'read `started === collection.length`, ran nothing, and returned SUCCESS with ' + + '`summary.failed = 0`. ⚠️ Read that measurement for the MECHANISM: the shape it was taken ' + + 'on is a `map`, which this parse rule deliberately does not reach — making the run-time ' + + 'refusal of a region-contained node that durably suspends LOUD is the second half of ' + + 'ruling D and ships as its own `domain:services` change. ⛔ NOT losslessly convertible: ' + + 'hoisting a node out of a region is a GRAPH REWRITE — new edges, a changed exit, sometimes ' + + 'a deleted container — and which of several shapes the author meant is an intent no ' + + 'artifact records, so a transform that picked one would be inventing the design. That ' + + 'leaves D3, a structured TODO naming each node to edit. ⚠️ Two further boundaries this ' + + 'refusal deliberately does NOT reach, because a parse cannot: a pausing node type ' + + 'contributed by a PLUGIN (ADR-0018 left the node-type namespace open and a parse has no ' + + 'registry), and a region nested past `MAX_REGION_DEPTH` (32), where the walk stops. For ' + + 'both, the engine\'s run-time refusal is still the only one — unchanged by this step, not ' + + 'fixed by it.', acceptanceCriteria: - 'No `screen` / `wait` / `subflow` / `map` / `approval` / `approval_revise` node and no ' - + '`end` node sits inside any `loop` body, `parallel` branch or `try_catch` try/catch ' - + 'region in the stack, at any depth. `FlowSchema.parse` (and therefore `defineFlow`, ' - + '`registerFlow`, `os validate` and a Studio publish) accepts the stack: a node still ' - + 'nested is refused with the node AND the region named in one message ' - + "(`A \\`map\\` node may not sit inside a structured region — \\`loop 'sweep' body → " + 'No `screen` / `wait` / `approval` / `approval_revise` node and no `end` node sits inside ' + + 'any `loop` body, `parallel` branch or `try_catch` try/catch region in the stack, at any ' + + 'depth. `FlowSchema.parse` (and therefore `defineFlow`, `registerFlow`, `os validate` and ' + + 'a Studio publish) accepts the stack: a node still nested is refused with the node AND the ' + + 'region named in one message ' + + "(`A \\`approval\\` node may not sit inside a structured region — \\`loop 'sweep' body → " + "try_catch 'guard' try\\` is a region body …`), anchored at " - + '`nodes[i].config.body.nodes[j].type` so a designer can jump to it. Behaviour to re-check ' - + 'after editing, because the fix CHANGES IT deliberately: a sweep that used to report ' - + '`success` having processed nothing now processes its items, so anything downstream that ' - + 'had quietly stopped receiving work starts receiving it again, and any alerting tuned to ' - + 'the empty-but-green runs will see real volume. A region-nested `end` was a no-op, so ' - + 'moving it to the top level makes the run actually TERMINATE there — check that the nodes ' - + 'after the container were not relying on continuing past it.', + + '`nodes[i].config.body.nodes[j].type` so a designer can jump to it. ⛔ A region-nested ' + + '`map` or `subflow` is NOT part of this migration and needs no edit to load — if such a ' + + 'flow reports `success` having processed nothing, that is the run-time half of the same ' + + 'ruling and not a stack edit. Behaviour to re-check after editing, because the fix CHANGES ' + + 'IT deliberately: a region-nested `end` was a no-op, so moving it to the top level makes ' + + 'the run actually TERMINATE there — check that the nodes after the container were not ' + + 'relying on continuing past it. A hoisted `approval` / `wait` / `screen` now parks the run ' + + 'where the enclosing graph can see it, so anything that polled for the sweep to finish ' + + 'sees a suspended run instead of a green-but-empty one.', }, { id: 'sys-account-issuer-retired', From 6de9d662f6df5e38be9303647845704a357e6b50 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 18 Sep 2026 10:53:49 +0000 Subject: [PATCH 6/6] chore(spec): regenerate the declaration-text shard for the added export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check:api-surface-declarations` landed on main after this branch point; the merge brings it in. Its delta on this branch is 0 removed / 1 added / 2 reshaped, and all three are non-narrowing: + FLOW_PAUSE_CAPABLE_NODE_TYPES — introduced by this PR; it is on neither main's api-surface nor main's declaration shard, so "added" is accurate. ~ ApprovalDecision, ApprovalNodeConfigSchema — property ORDER inside their `z.ZodEnum<{...}>` type literals, same members, same literal values. Object type members are order-insensitive in TypeScript, so old and new are mutually assignable; proved with a two-direction assignability probe plus a `@ts-expect-error` negative control, tsc exit 0. Claude-Session: https://claude.ai/code/session_01LvwGppdonww4zGLWZo5rho Co-authored-by: Claude --- packages/spec/api-surface-declarations/automation.txt | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/spec/api-surface-declarations/automation.txt b/packages/spec/api-surface-declarations/automation.txt index 23d9c94895b..283e0f069ad 100644 --- a/packages/spec/api-surface-declarations/automation.txt +++ b/packages/spec/api-surface-declarations/automation.txt @@ -12,8 +12,8 @@ # excluded: documentation drift is `check:docs`'s axis, not this one. # # entry: ./automation -# exported names: 273 -# declarations: 279 +# exported names: 274 +# declarations: 280 # # GENERATED — ⛔ never hand-edited. Regenerate after a real build: # pnpm --filter @objectstack/spec build && pnpm --filter @objectstack/spec gen:api-surface-declarations @@ -215,8 +215,8 @@ declare const ActionRefSchema: z.ZodUnion; // ── ApprovalDecision (type) ── @@ -311,8 +311,8 @@ declare const ApprovalNodeConfigSchema: z.ZodObject<{ lockRecord: z.ZodDefault; approvalStatusField: z.ZodOptional; onEmptyApprovers: z.ZodDefault>; @@ -988,6 +988,9 @@ declare const FLOW_BUILTIN_NODE_TYPES: readonly string[]; // ── FLOW_NODE_EXPRESSION_PATHS (const) ── declare const FLOW_NODE_EXPRESSION_PATHS: readonly FlowNodeExpressionPath[]; +// ── FLOW_PAUSE_CAPABLE_NODE_TYPES (const) ── +declare const FLOW_PAUSE_CAPABLE_NODE_TYPES: readonly string[]; + // ── FLOW_REGION_CONFIG_KEYS (const) ── declare const FLOW_REGION_CONFIG_KEYS: ReadonlySet;