diff --git a/.changeset/decision-branch-routing-enforced.md b/.changeset/decision-branch-routing-enforced.md new file mode 100644 index 0000000000..623e48e9eb --- /dev/null +++ b/.changeset/decision-branch-routing-enforced.md @@ -0,0 +1,86 @@ +--- +"@objectstack/service-automation": minor +"@objectstack/spec": minor +"@objectstack/cli": minor +"@objectstack/example-crm": patch +--- + +fix(automation): a decision's three declared ways to route a branch are now one working model (#4414) + +A `decision` node advertised three mechanisms for splitting a path and only one +of them did anything. The other two were the ADR-0049 `declared ≠ enforced` +shape, and the pair of them shipped a guard that does not guard in +`examples/app-crm`. + +| mechanism | before | now | +|:---|:---|:---| +| `edge.condition` | ✅ the only one that worked | unchanged | +| `edge.isDefault` | **zero readers** anywhere but the schema declaration | BPMN default flow, enforced in `traverseNext` | +| `decision.config.conditions[].label` → `branchLabel` | matched **0** out-edge labels across every example app, then fell back to the full edge set in silence | routes; an unclaimable label is logged, not swallowed | + +## What was broken, end to end + +`crm_convert_lead_wizard` means "already converted → abort screen; otherwise → +the wizard". It ran **both**: an already-converted lead got +"This lead has already been converted" and then walked straight into the +conversion wizard behind it. Four independent silences stacked up: + +1. the decision's first condition was authored `{lead_record.status} == + 'converted'` — braces in a slot declared bare CEL, so it was string-compared + and never true; +2. the second (`'true'`) therefore won, yielding `branchLabel: 'No — proceed'`; +3. no out-edge carried that label (they were `'Yes'` / `'No'`), so traversal + discarded the branch and considered every out-edge; +4. `e3b` was unconditional, so it ran regardless — and the natural fix, marking + it `isDefault: true`, was a dead key. + +## The model + +`branchLabel` narrows the edge set → `condition` gates each edge → `isDefault` +catches whatever is left. Concretely: + +- **`isDefault` is enforced.** A default edge is traversed only when no + conditional sibling of the same source node matched, and it is no longer part + of the unconditional parallel fan-out — that distinction is the whole point of + the marker. Passed over because a real branch won, its target records the same + `skipped` step a closed gate does (#4354). +- **An unclaimable branch label warns.** Traversal still falls back to the full + edge set (a run mid-flight must not die on a metadata error) but says so, + naming the computed branch and the out-edge labels that exist. +- **A decision that declares no `conditions` reports no branch.** It used to + report `'default'` unconditionally — a label no out-edge in the repo ever + carried — which is why every decision node fell back to the full edge set. + The `'default'` sentinel survives for the case it actually describes (declared + conditions, none matched) and is now claimed by the `isDefault` edge as well + as by an edge literally labelled `'default'`. +- **`conditions[].expression` is evaluated as the bare CEL it is declared to + be.** The raw string went to the legacy `{var}` template path, where + `lead.status == 'converted'` cannot resolve and the branch is decided by + string comparison. Unlike `edge.condition` this slot carries no + `ExpressionInput` envelope — the decision descriptor is deliberately + schemaless — so the executor supplies the dialect. A brace-in-CEL predicate + now fails loudly (ADR-0032 §1c) instead of deciding `false`. + +## Caught at authoring time too + +Four new `os build` / `os validate` warnings, because a wrong route is silent at +run time by nature (Prime Directive #12): + +`flow-branch-label-unmatched` (the shipped shape), +`flow-decision-unconditional-branch` (a guarded decision with an unconditional +sibling — the actual hole), `flow-default-edge-with-condition` and +`flow-multiple-default-edges`. + +Both of the first two fire on the pre-fix `convert-lead.flow.ts` and are silent +after it. + +## The example app + +`crm_convert_lead_wizard`'s guard is now a plain exclusive gateway: the +redundant `config.conditions` is gone and `e3b` carries `isDefault: true`. One +mechanism per decision, and exactly one branch runs. + +Verified: 11 new engine/executor tests (including the reported repro in both +directions), 12 new linter tests; `@objectstack/service-automation` 577 tests +and `@objectstack/cli` 652 tests green, all three example apps build with no new +findings. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 0db5623b40..7d344b7f5a 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -731,7 +731,61 @@ Edges connect nodes and define the execution path: | `type` | `enum` | optional | `'default'` (success), `'fault'` (error), `'conditional'` (expression-guarded), or `'back'` (declared back-edge, ADR-0044); defaults to `'default'` | | `condition` | `string` | optional | Boolean CEL predicate for branching (a bare string is stored as `{ dialect: 'cel', source }`) | | `label` | `string` | optional | Label displayed on the connector — cosmetic only. It does **not** select a path except on a branching node (`decision` / `approval`), which picks its out-edge by label. | -| `isDefault` | `boolean` | optional | BPMN default-flow marker (interop). Accepted by the schema, but **the engine does not read it** — traversal selects by `condition` and by `label`. On a `decision` node, the fallback is the out-edge labelled `default`: when no `conditions[]` entry matches, the node emits `branchLabel: 'default'` | +| `isDefault` | `boolean` | optional | BPMN default flow — the **"otherwise" branch**. Traversed only when no sibling `condition` on the same source node matched; never part of the unconditional parallel fan-out. Mutually exclusive with `condition`, at most one per node | + +### Branching — pick one mechanism per node + +A node has exactly two ways to split its path, and mixing them is what makes a +guard stop guarding (#4414). + +**Branch on the edges** (BPMN exclusive gateway — the default choice): + +```typescript +{ id: 'check', type: 'decision', label: 'Already converted?' }, // no config +// … +edges: [ + { id: 'e_yes', source: 'check', target: 'abort', condition: "lead.status == 'converted'", label: 'Yes' }, + { id: 'e_no', source: 'check', target: 'proceed', isDefault: true, label: 'No' }, +] +``` + +`e_no` runs **only** when `e_yes`'s condition was false. Drop `isDefault` and +`e_no` becomes an ordinary unconditional out-edge that runs on **every** pass — +in parallel with `abort` — so an already-converted lead sees the abort screen +*and* walks into the wizard behind it. Writing the negation of every sibling +condition by hand is the only other correct spelling; `isDefault` is the one +that stays correct when a third branch is added. + +**Branch on the node** (Salesforce-style decision outcomes): the node declares +`config.conditions[]` and traversal restricts itself to the out-edge whose +`label` matches the first matching entry. + +```typescript +{ id: 'check', type: 'decision', config: { conditions: [ + { label: 'Yes', expression: "lead.status == 'converted'" }, +] } }, +edges: [ + { id: 'e_yes', source: 'check', target: 'abort', label: 'Yes' }, + { id: 'e_no', source: 'check', target: 'proceed', isDefault: true }, +] +``` + +The labels must match **exactly**. When no declared condition matches, the node +reports the branch `default`, claimed by an out-edge labelled `'default'` or by +the `isDefault` edge. A branch label that no out-edge carries cannot route: the +engine logs a warning and falls back to considering every out-edge. That +fallback used to be silent, and a decision declaring `'Yes — already converted'` +against an out-edge labelled `'Yes'` is how #4414 shipped. `os validate` reports +the shape as `flow-branch-label-unmatched` at build time, along with +`flow-decision-unconditional-branch` (a guarded decision with an unconditional +sibling), `flow-default-edge-with-condition` and `flow-multiple-default-edges`. + + +A decision node that declares **no** `conditions` reports no branch at all — it +is a plain gateway and its out-edges do the routing. Do not declare both: +`config.conditions` *and* per-edge `condition`s on the same node means the node +picks a branch, and then that branch's edge re-decides. + ### Fault edges — handling a failed node diff --git a/content/docs/references/automation/flow.mdx b/content/docs/references/automation/flow.mdx index 344b10731e..78d1d46dfb 100644 --- a/content/docs/references/automation/flow.mdx +++ b/content/docs/references/automation/flow.mdx @@ -84,7 +84,7 @@ const result = Flow.parse(data); | **condition** | `string \| { dialect: Enum<'cel' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Predicate (CEL) returning boolean used for branching. | | **type** | `Enum<'default' \| 'fault' \| 'conditional' \| 'back'>` | optional | Connection type: default (normal flow), fault (error path), conditional (expression-guarded), or back (ADR-0044 declared back-edge — traversed normally at run time, but excluded from DAG cycle validation so a revise/rework loop can re-enter an earlier node) | | **label** | `string` | optional | Label on the connector | -| **isDefault** | `boolean` | optional | Marks this edge as the default path when no other conditions match | +| **isDefault** | `boolean` | optional | BPMN default flow: traverse this edge only when no sibling conditional edge of the same source node matched. Mutually exclusive with `condition`; at most one per source node. | --- diff --git a/content/docs/references/automation/schemaless-node-config.mdx b/content/docs/references/automation/schemaless-node-config.mdx index ab971fe805..1bf30f2936 100644 --- a/content/docs/references/automation/schemaless-node-config.mdx +++ b/content/docs/references/automation/schemaless-node-config.mdx @@ -121,7 +121,7 @@ const result = DecisionCondition.parse(data); | Property | Type | Required | Description | | :--- | :--- | :--- | :--- | -| **label** | `string` | ✅ | Branch label; the winning branch resumes down the out-edge with this label ('true' expression = default/else path) | +| **label** | `string` | ✅ | Branch label; the winning branch resumes down the out-edge with this label (no match → the out-edge marked isDefault, or one labelled 'default') | | **expression** | `string` | ✅ | Bare CEL predicate deciding this branch | diff --git a/examples/app-crm/src/flows/convert-lead.flow.ts b/examples/app-crm/src/flows/convert-lead.flow.ts index bc90e584cb..f8106daa3c 100644 --- a/examples/app-crm/src/flows/convert-lead.flow.ts +++ b/examples/app-crm/src/flows/convert-lead.flow.ts @@ -13,8 +13,8 @@ import { defineFlow } from '@objectstack/spec'; * children, atomically), and resumes the run with the new record's id bound to * `config.idVariable`. * - * start → get_lead → decision (already converted?) - * → screen_already_converted (abort path) + * start → get_lead → decision (already converted?) — exclusive, exactly one: + * → screen_already_converted (abort path, when status == 'converted') * → screen_account (Step 1 — full Customer form → account_id) * → screen_opportunity (Step 2 — full Opportunity form WITH product * line-items grid, prefilled account → opportunity_id) @@ -67,16 +67,19 @@ export const ConvertLeadScreenFlow = defineFlow({ }, // ── 3. Guard: already converted? ────────────────────────────────────── + // A plain exclusive gateway: the branching lives on the OUT-EDGES (`e3a`'s + // CEL condition, `e3b`'s `isDefault`) — ONE mechanism, not two. + // + // It used to ALSO declare `config.conditions` with its own labels, and that + // guard did not guard (#4414). Those labels (`'Yes — already converted'` / + // `'No — proceed'`) matched no out-edge label (`'Yes'` / `'No'`), so the + // branch the node computed was dropped and every out-edge was considered + // instead — and `e3b` was unconditional, so an already-converted lead got + // the abort screen AND walked straight into the wizard behind it. { id: 'check_converted', type: 'decision', label: 'Already Converted?', - config: { - conditions: [ - { label: 'Yes — already converted', expression: "{lead_record.status} == 'converted'" }, - { label: 'No — proceed', expression: 'true' }, - ], - }, }, // ── 3a. Already-converted abort screen ──────────────────────────────── @@ -155,9 +158,13 @@ export const ConvertLeadScreenFlow = defineFlow({ edges: [ { id: 'e1', source: 'start', target: 'get_lead', type: 'default' }, { id: 'e2', source: 'get_lead', target: 'check_converted', type: 'default' }, - // guard branches + // Guard branches — mutually exclusive. `e3a` carries the CEL predicate; + // `e3b` is the BPMN default flow (`isDefault`), traversed ONLY when no + // conditional sibling matched. Without that marker `e3b` is an ordinary + // unconditional out-edge and runs on every pass, wizard and abort screen + // together (#4414). { id: 'e3a', source: 'check_converted', target: 'screen_already_converted', type: 'default', condition: "lead_record.status == 'converted'", label: 'Yes' }, - { id: 'e3b', source: 'check_converted', target: 'screen_account', type: 'default', label: 'No' }, + { id: 'e3b', source: 'check_converted', target: 'screen_account', type: 'default', isDefault: true, label: 'No' }, { id: 'e3c', source: 'screen_already_converted', target: 'end', type: 'default' }, // main path — full Customer form → full Opportunity form → link { id: 'e4', source: 'screen_account', target: 'screen_opportunity', type: 'default' }, diff --git a/packages/cli/src/utils/lint-flow-patterns.test.ts b/packages/cli/src/utils/lint-flow-patterns.test.ts index f7ac8c10fa..510bffd287 100644 --- a/packages/cli/src/utils/lint-flow-patterns.test.ts +++ b/packages/cli/src/utils/lint-flow-patterns.test.ts @@ -13,6 +13,10 @@ import { FLOW_APPROVAL_REVISE_DISABLED, FLOW_RUNAS_UNSCOPED, FLOW_ERROR_LABEL_NOT_FAULT, + FLOW_BRANCH_LABEL_UNMATCHED, + FLOW_DECISION_UNCONDITIONAL_BRANCH, + FLOW_DEFAULT_EDGE_WITH_CONDITION, + FLOW_MULTIPLE_DEFAULT_EDGES, } from './lint-flow-patterns.js'; const CEL = (source: string) => ({ dialect: 'cel', source }); @@ -448,3 +452,156 @@ describe('flow-error-label-not-fault (#3863)', () => { }, ); }); + +/** + * #4414 — a decision that declares a branch it cannot route. + * + * `guardFlow` is `examples/app-crm/src/flows/convert-lead.flow.ts`'s guard, + * reduced: one CEL-guarded abort branch and one fallback branch. + */ +function guardFlow(opts: { + conditions?: Array<{ label: string; expression: string }>; + proceed?: Record; + extra?: Array>; +} = {}) { + return { + flows: [{ + name: 'convert_lead', + type: 'screen', + nodes: [ + { id: 'start', type: 'start', config: {} }, + { + id: 'check', type: 'decision', + ...(opts.conditions ? { config: { conditions: opts.conditions } } : {}), + }, + { id: 'abort', type: 'screen', config: {} }, + { id: 'proceed', type: 'screen', config: {} }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e_yes', source: 'check', target: 'abort', label: 'Yes', condition: "lead.status == 'converted'" }, + { id: 'e_no', source: 'check', target: 'proceed', label: 'No', ...(opts.proceed ?? {}) }, + ...(opts.extra ?? []), + ], + }], + }; +} + +describe('flow-branch-label-unmatched (#4414)', () => { + // The shipped shape: labels `'Yes — already converted'` / `'No — proceed'` + // against out-edges labelled `'Yes'` / `'No'` — zero matches. + it('flags decision branch labels no out-edge carries', () => { + const fnds = lintFlowPatterns(guardFlow({ + proceed: { isDefault: true }, + conditions: [ + { label: 'Yes — already converted', expression: "lead.status == 'converted'" }, + { label: 'No — proceed', expression: 'true' }, + ], + })).filter((f) => f.rule === FLOW_BRANCH_LABEL_UNMATCHED); + + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toContain("decision 'check'"); + expect(fnds[0].message).toContain("'yes — already converted'"); + expect(fnds[0].message).toContain("'no — proceed'"); + // The consequence, not just the mismatch. + expect(fnds[0].message).toContain('EVERY'); + }); + + it('flags a partial mismatch — one claimed label does not excuse the other', () => { + const fnds = lintFlowPatterns(guardFlow({ + proceed: { isDefault: true }, + conditions: [ + { label: 'Yes', expression: "lead.status == 'converted'" }, + { label: 'No — proceed', expression: 'true' }, + ], + })).filter((f) => f.rule === FLOW_BRANCH_LABEL_UNMATCHED); + expect(fnds).toHaveLength(1); + // Only the unclaimed label is reported as unclaimed; `'yes'` still shows up + // later in the message as one of the out-edge labels that DO exist. + expect(fnds[0].message).toMatch(/declares branch label\(s\) 'no — proceed' that no out-edge/); + }); + + it('does NOT flag labels that match (case/whitespace-insensitively)', () => { + expect(lintFlowPatterns(guardFlow({ + proceed: { isDefault: true }, + conditions: [{ label: ' yes ', expression: 'true' }], + })).filter((f) => f.rule === FLOW_BRANCH_LABEL_UNMATCHED)).toHaveLength(0); + }); + + it('does NOT flag a decision that declares no conditions at all', () => { + expect(lintFlowPatterns(guardFlow({ proceed: { isDefault: true } }))).toHaveLength(0); + }); +}); + +describe('flow-decision-unconditional-branch (#4414)', () => { + // The actual hole: `e_no` has no condition and no `isDefault`, so it is + // traversed on every pass — the abort screen AND the wizard behind it. + it('flags an unconditional out-edge alongside a guarded one', () => { + const fnds = lintFlowPatterns(guardFlow()).filter( + (f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH, + ); + expect(fnds).toHaveLength(1); + expect(fnds[0].message).toContain("'proceed'"); + expect(fnds[0].message).toContain('EVERY pass'); + expect(fnds[0].hint).toContain('isDefault'); + }); + + it('does NOT flag once the fallback is marked isDefault — the fix', () => { + expect(lintFlowPatterns(guardFlow({ proceed: { isDefault: true } }))).toHaveLength(0); + }); + + it('does NOT flag once the fallback carries its own condition', () => { + expect(lintFlowPatterns(guardFlow({ + proceed: { condition: "lead.status != 'converted'" }, + }))).toHaveLength(0); + }); + + it('does NOT flag an edge the decision CAN select by declared label', () => { + expect(lintFlowPatterns(guardFlow({ + conditions: [{ label: 'No', expression: 'true' }, { label: 'Yes', expression: 'false' }], + })).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0); + }); + + it('does NOT flag a decision with no guarded edge at all — nothing to undercut', () => { + expect(lintFlowPatterns({ + flows: [{ + name: 'plain', + nodes: [{ id: 'start', type: 'start', config: {} }, { id: 'check', type: 'decision' }, { id: 'a', type: 'screen', config: {} }], + edges: [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e2', source: 'check', target: 'a' }, + ], + }], + })).toHaveLength(0); + }); + + it('does NOT flag a fault edge — error routing is not branch selection', () => { + expect(lintFlowPatterns(guardFlow({ + proceed: { isDefault: true }, + extra: [{ id: 'e_err', source: 'check', target: 'abort', type: 'fault' }], + }))).toHaveLength(0); + }); +}); + +describe('flow-default-edge-with-condition / flow-multiple-default-edges (#4414)', () => { + it('flags an edge that is both the default and conditional', () => { + const fnds = lintFlowPatterns(guardFlow({ + proceed: { isDefault: true, condition: "lead.status != 'converted'" }, + })).filter((f) => f.rule === FLOW_DEFAULT_EDGE_WITH_CONDITION); + expect(fnds).toHaveLength(1); + expect(fnds[0].message).toContain('contradictory'); + }); + + it('flags two default edges out of one node', () => { + const fnds = lintFlowPatterns(guardFlow({ + proceed: { isDefault: true }, + extra: [{ id: 'e_also', source: 'check', target: 'abort', isDefault: true }], + })).filter((f) => f.rule === FLOW_MULTIPLE_DEFAULT_EDGES); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toContain("node 'check'"); + }); + + it('does NOT flag one default edge per node', () => { + expect(lintFlowPatterns(guardFlow({ proceed: { isDefault: true } }))).toHaveLength(0); + }); +}); diff --git a/packages/cli/src/utils/lint-flow-patterns.ts b/packages/cli/src/utils/lint-flow-patterns.ts index e3925fc2ce..9bbb3ea789 100644 --- a/packages/cli/src/utils/lint-flow-patterns.ts +++ b/packages/cli/src/utils/lint-flow-patterns.ts @@ -78,6 +78,11 @@ export const FLOW_APPROVAL_REVISE_DISABLED = 'flow-approval-revise-disabled'; */ export const FLOW_RUNAS_UNSCOPED = 'flow-runas-unscoped'; export const FLOW_ERROR_LABEL_NOT_FAULT = 'flow-error-label-not-fault'; +/** #4414 — the four ways a decision's declared branching fails to route. */ +export const FLOW_BRANCH_LABEL_UNMATCHED = 'flow-branch-label-unmatched'; +export const FLOW_DECISION_UNCONDITIONAL_BRANCH = 'flow-decision-unconditional-branch'; +export const FLOW_DEFAULT_EDGE_WITH_CONDITION = 'flow-default-edge-with-condition'; +export const FLOW_MULTIPLE_DEFAULT_EDGES = 'flow-multiple-default-edges'; /** Node types that perform a data operation — the ones `flow.runAs` governs (#1888). */ const DATA_NODE_TYPES = new Set(['get_record', 'create_record', 'update_record', 'delete_record']); @@ -291,6 +296,147 @@ function scanErrorLabelledEdges( } } +/** + * #4414 — a decision node that DECLARES a branch it cannot route. + * + * A decision has three declared ways to pick a branch, and until #4414 only one + * of them worked. They now compose (`branchLabel` narrows the edge set → + * `condition` gates → `isDefault` catches the rest), but composing them still + * leaves four authorable shapes where what the author wrote does not route what + * they meant. All four are silent at run time — the flow completes green, having + * taken the wrong path — so they are caught here, at authoring time: + * + * (1) `flow-branch-label-unmatched` — the decision's `conditions[].label` names + * a branch no out-edge carries. Traversal cannot honour a label nothing + * claims, so it falls back to considering EVERY out-edge. This is the + * shipped defect: app-crm's convert-lead guard computed `'No — proceed'` + * against out-edges labelled `'Yes'` / `'No'`, matched nothing, and ran + * both branches. + * (2) `flow-decision-unconditional-branch` — an out-edge of a decision that has + * no `condition`, no `isDefault`, and no label the decision can select. It + * is traversed on EVERY pass, in parallel with whichever branch did match, + * so the guard next to it does not guard. + * (3) `flow-default-edge-with-condition` — `isDefault` means "when nothing else + * matched"; a condition on the same edge contradicts it (BPMN forbids a + * conditional default flow). The condition wins and the marker is inert. + * (4) `flow-multiple-default-edges` — two fallbacks out of one node. Both are + * traversed when nothing matched, which is a parallel fan-out, not the + * exclusive "otherwise" the marker promises. + * + * Advisory, not build-failing: each shape is a wrong ROUTE, not a guaranteed + * runtime failure, and the engine now also warns when it hits (1) live. + */ +function scanBranchRouting( + flowName: string, + nodes: AnyRec[], + edges: AnyRec[], + findings: FlowLintFinding[], +): void { + const outEdgesBySource = new Map(); + for (const e of edges) { + if (e.type === 'fault') continue; // error routing, not branch selection + const src = typeof e.source === 'string' ? e.source : ''; + if (!src) continue; + if (!outEdgesBySource.has(src)) outEdgesBySource.set(src, []); + outEdgesBySource.get(src)!.push(e); + } + + // (3) + (4) apply to every node's out-edges, not just decisions — `isDefault` + // is meaningful wherever conditional siblings exist. + for (const [src, outs] of outEdgesBySource) { + for (const e of outs) { + if (e.isDefault === true && e.condition) { + findings.push({ + where: `flow '${flowName}' · edge '${src}' → '${String(e.target)}'`, + message: + `edge sets \`isDefault: true\` AND a \`condition\` — contradictory. \`isDefault\` means ` + + `"take this edge when NO sibling condition matched"; a condition makes it an ordinary ` + + `guarded branch. The condition wins and the default marker routes nothing.`, + hint: + `Drop one: keep \`condition\` for a guarded branch, or drop it and keep \`isDefault: true\` ` + + `for the "otherwise" path. (#4414)`, + rule: FLOW_DEFAULT_EDGE_WITH_CONDITION, + }); + } + } + const defaults = outs.filter((e) => e.isDefault === true && !e.condition); + if (defaults.length > 1) { + findings.push({ + where: `flow '${flowName}' · node '${src}'`, + message: + `${defaults.length} out-edges are marked \`isDefault: true\` (${defaults + .map((e) => `'${String(e.target)}'`) + .join(', ')}) — a node has at most ONE default path. All of them are traversed together ` + + `when no condition matches, which is a parallel fan-out, not an "otherwise".`, + hint: + `Keep \`isDefault: true\` on the single fallback edge and give the others a \`condition\` ` + + `(or leave them unconditional if the fan-out really is intended). (#4414)`, + rule: FLOW_MULTIPLE_DEFAULT_EDGES, + }); + } + } + + // (1) + (2) are about a DECISION's own declared branching. + for (const node of nodes) { + if (node.type !== 'decision') continue; + const nid = typeof node.id === 'string' ? node.id : ''; + if (!nid) continue; + const outs = outEdgesBySource.get(nid) ?? []; + if (outs.length === 0) continue; + + const cfg = (node.config ?? {}) as AnyRec; + const declaredLabels = new Set( + (Array.isArray(cfg.conditions) ? (cfg.conditions as AnyRec[]) : []) + .map((c) => (typeof c?.label === 'string' ? c.label.trim().toLowerCase() : '')) + .filter(Boolean), + ); + const edgeLabels = new Set(outs.map(edgeLabelOf).filter(Boolean)); + + // (1) a declared branch label nothing claims. `default` is the engine's own + // sentinel for "no declared condition matched" and is additionally + // claimed by the BPMN default edge, so it is never counted as unclaimed. + const unclaimed = [...declaredLabels].filter((l) => !edgeLabels.has(l)); + if (unclaimed.length > 0) { + findings.push({ + where: `flow '${flowName}' · decision '${nid}'`, + message: + `declares branch label(s) ${unclaimed.map((l) => `'${l}'`).join(', ')} that no out-edge ` + + `carries — out-edge labels are [${[...edgeLabels].map((l) => `'${l}'`).join(', ') || 'none'}]. ` + + `Traversal cannot honour a label nothing claims, so it falls back to considering EVERY ` + + `out-edge and the branch the decision computed is ignored.`, + hint: + `Make an out-edge's \`label\` match the declared branch exactly, or drop \`config.conditions\` ` + + `and branch on the edges instead (\`condition\` per branch + \`isDefault: true\` on the ` + + `fallback) — one mechanism per decision, never both. (#4414)`, + rule: FLOW_BRANCH_LABEL_UNMATCHED, + }); + } + + // (2) an out-edge nothing can gate: no condition, not the default, and not + // selectable by a label the decision declares. + const gated = outs.filter((e) => e.condition || e.isDefault === true); + if (gated.length === 0) continue; // no branching declared at all — nothing to undercut + const ungated = outs.filter( + (e) => !e.condition && e.isDefault !== true && !declaredLabels.has(edgeLabelOf(e)), + ); + if (ungated.length > 0) { + findings.push({ + where: `flow '${flowName}' · decision '${nid}'`, + message: + `has guarded out-edge(s) alongside unconditional one(s) ` + + `(${ungated.map((e) => `'${String(e.target)}'`).join(', ')}) — an unconditional out-edge is ` + + `traversed on EVERY pass, in parallel with whichever guarded branch matched, so the ` + + `decision does not actually exclude it. A \`label\` alone does not select a path unless the ` + + `decision declares a matching \`conditions[].label\`.`, + hint: + `Mark the fallback \`isDefault: true\` so it is taken only when no sibling condition matched ` + + `(BPMN default flow), or give it its own \`condition\`. (#4414)`, + rule: FLOW_DECISION_UNCONDITIONAL_BRANCH, + }); + } + } +} + function scanApprovalReviseLoops( flowName: string, nodes: AnyRec[], @@ -503,6 +649,11 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { // unconditional out-edge: the handler runs on every SUCCESS, in parallel // with the real path, and never on a failure. scanErrorLabelledEdges(flowName, nodes, edges, findings); + + // (e) #4414 — a decision that declares a branch it cannot route: an + // unclaimable branch label, an unconditional sibling that runs anyway, + // or a self-contradictory / duplicated `isDefault` marker. + scanBranchRouting(flowName, nodes, edges, findings); } return findings; } diff --git a/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts b/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts new file mode 100644 index 0000000000..55328439ea --- /dev/null +++ b/packages/services/service-automation/src/builtin/decision-branch-routing.test.ts @@ -0,0 +1,252 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { AutomationEngine } from '../engine.js'; +import { registerLogicNodes } from './logic-nodes.js'; + +/** + * #4414 — a decision node declared three ways to route a branch and two of them + * did nothing. + * + * • `decision.config.conditions[].label` → `branchLabel` matched no out-edge + * label anywhere in the repo and fell back to the full edge set, silently. + * • `FlowEdgeSchema.isDefault` had ZERO readers: it parsed, it was documented + * as "the default path when no other conditions match", and it routed + * nothing. + * • `edge.condition` was the only one that worked. + * + * The consequence shipped in `examples/app-crm` — see the first block below. + */ + +const warnings: string[] = []; + +function createTestLogger(): any { + return { + info: () => {}, + warn: (msg: string) => { warnings.push(String(msg)); }, + error: () => {}, + debug: () => {}, + child: () => createTestLogger(), + }; +} + +function createCtx(): any { + return { logger: createTestLogger(), getService: () => undefined }; +} + +describe('decision branch routing (#4414)', () => { + let engine: AutomationEngine; + let visited: string[]; + + beforeEach(() => { + warnings.length = 0; + visited = []; + engine = new AutomationEngine(createTestLogger()); + registerLogicNodes(engine, createCtx()); + // A do-nothing terminal so a visited branch is observable without + // dragging screen/CRUD executors into the test. + engine.registerNodeExecutor({ + type: 'mark', + async execute(node) { + visited.push(node.id); + return { success: true }; + }, + }); + }); + + /** The guard from `examples/app-crm/src/flows/convert-lead.flow.ts`. */ + function guardFlow(opts: { + conditions?: Array<{ label: string; expression: string }>; + proceedIsDefault?: boolean; + }) { + return { + name: 'guard', + label: 'Guard', + type: 'autolaunched' as const, + variables: [{ name: 'lead', type: 'object', isInput: true }], + nodes: [ + { id: 'start', type: 'start' as const, label: 'Start' }, + { + id: 'check', type: 'decision' as const, label: 'Already converted?', + ...(opts.conditions ? { config: { conditions: opts.conditions } } : {}), + }, + { id: 'abort', type: 'mark' as const, label: 'Abort' }, + { id: 'proceed', type: 'mark' as const, label: 'Proceed' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'check' }, + { + id: 'e_yes', source: 'check', target: 'abort', label: 'Yes', + condition: "lead.status == 'converted'", + }, + { + id: 'e_no', source: 'check', target: 'proceed', label: 'No', + ...(opts.proceedIsDefault ? { isDefault: true } : {}), + }, + ], + }; + } + + const run = (lead: Record) => + engine.execute('guard', { params: { lead } } as any); + + // ── The shipped defect, and its fix ─────────────────────────────────── + + it('runs BOTH branches when the fallback edge is merely unconditional', async () => { + engine.registerFlow('guard', guardFlow({})); + await run({ status: 'converted' }); + // This is the bug as reported: the abort screen AND the wizard behind it. + expect(visited).toEqual(['abort', 'proceed']); + }); + + it('takes exactly one branch once the fallback edge is `isDefault`', async () => { + engine.registerFlow('guard', guardFlow({ proceedIsDefault: true })); + + await run({ status: 'converted' }); + expect(visited).toEqual(['abort']); + + visited.length = 0; + await run({ status: 'open' }); + expect(visited).toEqual(['proceed']); + }); + + // ── `isDefault` — BPMN default flow ─────────────────────────────────── + + it('records a `skipped` step for a default edge passed over by a real branch', async () => { + engine.registerFlow('guard', guardFlow({ proceedIsDefault: true })); + await run({ status: 'converted' }); + const [log] = await engine.listRuns('guard'); + const skipped = log!.steps!.find((s) => s.nodeId === 'proceed'); + expect(skipped?.status).toBe('skipped'); + expect(skipped?.skippedBy).toMatchObject({ nodeId: 'check', edgeId: 'e_no' }); + }); + + it('keeps a default edge out of the unconditional parallel fan-out', async () => { + // Two plain unconditional edges still fan out; the default one does not + // join them when a conditional sibling matched. + engine.registerFlow('fanout', { + name: 'fanout', + label: 'Fanout', + type: 'autolaunched', + variables: [{ name: 'lead', type: 'object', isInput: true }], + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'check', type: 'decision', label: 'Check' }, + { id: 'hit', type: 'mark', label: 'Hit' }, + { id: 'always', type: 'mark', label: 'Always' }, + { id: 'otherwise', type: 'mark', label: 'Otherwise' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'check' }, + { id: 'e2', source: 'check', target: 'hit', condition: "lead.status == 'converted'" }, + { id: 'e3', source: 'check', target: 'always' }, + { id: 'e4', source: 'check', target: 'otherwise', isDefault: true }, + ], + }); + await engine.execute('fanout', { params: { lead: { status: 'converted' } } } as any); + expect(visited).toContain('hit'); + expect(visited).toContain('always'); + expect(visited).not.toContain('otherwise'); + }); + + it('takes the default edge when the node has no conditional siblings at all', async () => { + engine.registerFlow('bare', { + name: 'bare', + label: 'Bare', + type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'only', type: 'mark', label: 'Only' }, + ], + edges: [{ id: 'e1', source: 'start', target: 'only', isDefault: true }], + }); + await engine.execute('bare'); + expect(visited).toEqual(['only']); + }); + + // ── `branchLabel` — no more silent fallback ─────────────────────────── + + it('routes by branch label when an out-edge claims it', async () => { + engine.registerFlow('guard', guardFlow({ + proceedIsDefault: true, + conditions: [{ label: 'No', expression: 'true' }], + })); + await run({ status: 'converted' }); + // The decision selected 'No', which narrows traversal to `e_no` — the + // abort branch is never even evaluated. + expect(visited).toEqual(['proceed']); + expect(warnings).toHaveLength(0); + }); + + it('warns — instead of silently falling back — when no out-edge claims the label', async () => { + engine.registerFlow('guard', guardFlow({ + conditions: [ + { label: 'Yes — already converted', expression: "lead.status == 'converted'" }, + { label: 'No — proceed', expression: 'true' }, + ], + })); + await run({ status: 'converted' }); + + expect(warnings.some((w) => + w.includes("selected branch 'Yes — already converted'") + && w.includes("'Yes'") && w.includes("'No'") + && w.includes('#4414'), + )).toBe(true); + // Behaviour is unchanged (a run mid-flight must not die on it) — but it + // is no longer invisible. + expect(visited).toEqual(['abort', 'proceed']); + }); + + it('reports no branch at all from a decision that declares no conditions', async () => { + // The old executor returned `branchLabel: 'default'` here, a label no + // out-edge in the repo ever carried — so EVERY decision node fell back + // to the full edge set. Nothing to warn about now: there is no branch. + engine.registerFlow('guard', guardFlow({ proceedIsDefault: true })); + await run({ status: 'open' }); + expect(warnings).toHaveLength(0); + expect(visited).toEqual(['proceed']); + }); + + // ── `conditions[].expression` is bare CEL, as declared ──────────────── + + it('decides a branch on a bare-CEL predicate over a nested variable', async () => { + engine.registerFlow('guard', guardFlow({ + proceedIsDefault: true, + conditions: [ + { label: 'Yes', expression: "lead.status == 'converted'" }, + { label: 'No', expression: 'true' }, + ], + })); + // Handed to the legacy `{var}` template path — which is where a raw + // string used to go — `lead.status` never resolves and the first branch + // is decided by string comparison instead of by the record. + await run({ status: 'converted' }); + expect(visited).toEqual(['abort']); + + visited.length = 0; + await run({ status: 'open' }); + expect(visited).toEqual(['proceed']); + }); + + it('fails loudly on a brace-in-CEL decision predicate rather than deciding `false`', async () => { + engine.registerFlow('guard', guardFlow({ + proceedIsDefault: true, + conditions: [{ label: 'Yes', expression: "{lead.status} == 'converted'" }], + })); + const result = await run({ status: 'converted' }); + expect(result.success).toBe(false); + expect(String(result.error)).toMatch(/template braces|bare CEL/); + }); + + it("lets the `default` sentinel claim the `isDefault` edge when no condition matched", async () => { + engine.registerFlow('guard', guardFlow({ + proceedIsDefault: true, + conditions: [{ label: 'Yes', expression: "lead.status == 'converted'" }], + })); + await run({ status: 'open' }); + // No declared condition matched → branch 'default' → the BPMN default + // edge claims it, without the author also labelling that edge 'default'. + expect(visited).toEqual(['proceed']); + expect(warnings).toHaveLength(0); + }); +}); diff --git a/packages/services/service-automation/src/builtin/logic-nodes.ts b/packages/services/service-automation/src/builtin/logic-nodes.ts index 099bf0a360..d69b22dc5e 100644 --- a/packages/services/service-automation/src/builtin/logic-nodes.ts +++ b/packages/services/service-automation/src/builtin/logic-nodes.ts @@ -2,7 +2,7 @@ import type { PluginContext } from '@objectstack/core'; import { defineActionDescriptor } from '@objectstack/spec/automation'; -import type { AutomationEngine } from '../engine.js'; +import { DEFAULT_BRANCH_LABEL, type AutomationEngine } from '../engine.js'; import { interpolate } from './template.js'; /** @@ -26,16 +26,43 @@ export function registerLogicNodes(engine: AutomationEngine, ctx: PluginContext) description: 'Branch execution based on conditions.', icon: 'git-branch', category: 'logic', source: 'builtin', }), + /** + * A decision routes one of two ways, and it must not pretend to the + * other (#4414): + * + * • It DECLARED `config.conditions` → the first matching entry's + * `label` is the branch, and traversal restricts itself to the + * out-edge carrying that label. If none matched, the branch is + * {@link DEFAULT_BRANCH_LABEL} — claimed by an out-edge labelled + * `'default'` or marked `isDefault: true`. + * • It declared NONE → it is a plain gateway: the branching lives on + * the out-edges (`condition` / `isDefault`) and the node reports + * **no** branch. It used to report `'default'` regardless — a + * label no out-edge in this repo ever carried — so every decision + * node silently fell back to "consider every out-edge", which is + * the state #4414 measured (0 label matches across all example + * apps) and, on an unconditional sibling, ran both branches. + */ async execute(node, variables, _context) { const config = node.config as Record | undefined; const conditions = (config?.conditions ?? []) as Array<{ label: string; expression: string }>; + if (conditions.length === 0) return { success: true }; for (const cond of conditions) { - if (engine.evaluateCondition(cond.expression, variables)) { + // `DecisionConditionSchema.expression` is declared BARE CEL + // (ADR-0032) — evaluate it as such. Handing the raw string to + // `evaluateCondition` routed it to the legacy `{var}` template + // path instead, which reads a declared-CEL field in a second + // dialect: `lead.status == 'converted'` never resolves there, + // so the branch it decides is decided by string comparison. + // Unlike `edge.condition` this slot has no `ExpressionInput` + // envelope to carry the dialect — the decision descriptor is + // deliberately schemaless — so the executor supplies it. + if (engine.evaluateCondition({ dialect: 'cel', source: cond.expression }, variables)) { return { success: true, branchLabel: cond.label }; } } - return { success: true, branchLabel: 'default' }; + return { success: true, branchLabel: DEFAULT_BRANCH_LABEL }; }, }); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 7f805c97d8..2855b574fb 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -41,6 +41,19 @@ import { ExpressionEngine, validateExpression, nearestName } from '@objectstack/ */ const UNRESOLVED_CEL_REFERENCE = /^[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)+$/; +/** + * The branch a `decision` node reports when it DECLARED `config.conditions` and + * none of them matched — "fall through to the declared fallback". + * + * Two spellings claim it in {@link AutomationEngine.traverseNext}: an out-edge + * literally `label`led `'default'` (the historical, documented spelling) and an + * out-edge marked `isDefault: true` (the BPMN default flow, canonical since + * #4414). A decision that declares NO conditions reports no branch at all — it + * has nothing to fall through *from*, and inventing a label for it is what made + * every decision node in the repo emit an unclaimable `'default'`. + */ +export const DEFAULT_BRANCH_LABEL = 'default'; + /** * The slice of a descriptor's JSON-Schema `configSchema` that the undeclared-key * walk reads (#4045). Structural only — no validation semantics. @@ -3599,10 +3612,37 @@ export class AutomationEngine implements IAutomationService { * {@link executeNode} so {@link resume} can re-enter traversal from a * suspended node without re-running the node body. * - * @param branchLabel - When set (e.g. from a resume signal), restrict - * traversal to out-edges whose `label` matches — this is how an Approval - * node's `approve`/`reject` decision selects its downstream branch. When - * no edge carries the label, traversal falls back to the normal edge set. + * Three declared mechanisms select a branch here, and #4414 found two of + * them doing nothing. They now compose as ONE model, applied in this order: + * + * 1. **`branchLabel`** (from a `decision`/`approval` executor or a resume + * signal) narrows the edge set to out-edges carrying that `label`. + * {@link DEFAULT_BRANCH_LABEL} is the engine's own sentinel for "the + * node's declared conditions all failed" and is additionally claimed by + * the BPMN default edge. A label NO edge claims is a metadata error — + * traversal still falls back to the full edge set (a run mid-flight must + * not die on it) but it is now **logged**, not silent: the decision had + * computed a branch and nothing routed it, which is how app-crm's + * convert-lead guard ran its abort screen AND its wizard. + * 2. **`edge.condition`** — evaluated per edge; a closed gate records a + * `skipped` step (#4354). + * 3. **`edge.isDefault`** — BPMN default flow. Traversed **only** when no + * conditional sibling in the selected set matched. Before #4414 this key + * had zero readers: it parsed, it was documented as "the default path + * when no other conditions match", and it routed nothing — an author who + * reached for it got an ordinary unconditional edge that ran on every + * pass, in parallel with the branch that *did* match. + * + * A default edge is therefore NOT part of the unconditional parallel fan-out + * — that distinction is the whole point of the marker. An edge that carries + * both a `condition` and `isDefault` is self-contradictory (BPMN forbids it); + * the `condition` wins here, and the flow linter flags the shape at authoring + * time (`flow-default-edge-with-condition`) so it is caught before it runs — + * Prime Directive #12. + * + * @param branchLabel - When set, restrict traversal to out-edges whose + * `label` matches — this is how an Approval node's `approve`/`reject` + * decision selects its downstream branch. */ private async traverseNext( node: FlowNodeParsed, @@ -3612,31 +3652,61 @@ export class AutomationEngine implements IAutomationService { steps: StepLogEntry[], branchLabel?: string, ): Promise { - // Find next nodes — separate conditional and unconditional edges - let outEdges = flow.edges.filter( + // Find next nodes — separate conditional, default and unconditional edges + const allOutEdges = flow.edges.filter( e => e.source === node.id && e.type !== 'fault', ); + let outEdges = allOutEdges; - // Branch selection (resume): prefer edges tagged with the decision label. + // Branch selection: prefer edges tagged with the decision label. if (branchLabel) { - const labeled = outEdges.filter(e => e.label === branchLabel); - if (labeled.length > 0) outEdges = labeled; + let claimed = outEdges.filter(e => e.label === branchLabel); + // The `default` sentinel is also claimed by the BPMN default edge, so + // "none of my conditions matched" routes to the declared fallback + // without the author having to ALSO label that edge 'default'. + if (claimed.length === 0 && branchLabel === DEFAULT_BRANCH_LABEL) { + claimed = outEdges.filter(e => e.isDefault); + } + if (claimed.length > 0) { + outEdges = claimed; + } else { + // #4414 — do not fall back silently. The node computed a branch + // and no out-edge claims it, so every out-edge is about to be + // considered: the guard the author wrote is not guarding. + const declared = allOutEdges + .map(e => (e.label ? `'${e.label}'` : `(unlabelled ${e.id})`)) + .join(', '); + this.logger.warn( + // `flow.name` is absent on the synthetic view `runRegion` builds. + `Flow '${flow.name ?? '(region)'}' node '${node.id}' (${node.type}) selected branch ` + + `'${branchLabel}', but no out-edge carries that label — out-edge labels are ` + + `[${declared || 'none'}]. The branch selection is IGNORED and every out-edge is ` + + `evaluated instead, so unconditional siblings run regardless of the decision. ` + + `Make an out-edge's \`label\` match the branch, or mark the fallback edge ` + + `\`isDefault: true\`. (#4414)`, + ); + } } const conditionalEdges: FlowEdgeParsed[] = []; + const defaultEdges: FlowEdgeParsed[] = []; const unconditionalEdges: FlowEdgeParsed[] = []; for (const edge of outEdges) { if (edge.condition) { conditionalEdges.push(edge); + } else if (edge.isDefault) { + defaultEdges.push(edge); } else { unconditionalEdges.push(edge); } } // Conditional edges: evaluate sequentially (mutually exclusive) + let anyConditionMet = false; for (const edge of conditionalEdges) { const nextNode = flow.nodes.find(n => n.id === edge.target); if (this.evaluateCondition(edge.condition!, variables)) { + anyConditionMet = true; if (nextNode) { await this.executeNode(nextNode, flow, variables, context, steps); } @@ -3670,6 +3740,38 @@ export class AutomationEngine implements IAutomationService { } } + // Default edges (BPMN default flow, #4414): the fallback, taken only + // when NO conditional sibling matched. `isDefault` is what makes this an + // "otherwise" rather than a second unconditional path — without it the + // author's only spelling of "otherwise" was to hand-write the negation + // of every sibling condition, and forgetting to do that ran both + // branches. A default edge passed over because a real branch won records + // the same `skipped` trace a closed gate does (#4354). + for (const edge of defaultEdges) { + const nextNode = flow.nodes.find(n => n.id === edge.target); + if (!anyConditionMet) { + if (nextNode) { + await this.executeNode(nextNode, flow, variables, context, steps); + } + } else if (nextNode) { + const at = new Date().toISOString(); + steps.push({ + nodeId: nextNode.id, + nodeType: nextNode.type, + ...(nextNode.label ? { nodeLabel: nextNode.label } : {}), + status: 'skipped', + startedAt: at, + completedAt: at, + durationMs: 0, + skippedBy: { + nodeId: node.id, + ...(edge.id ? { edgeId: edge.id } : {}), + ...(edge.label ? { label: edge.label } : {}), + }, + }); + } + } + // Unconditional edges: execute in parallel (Promise.all) if (unconditionalEdges.length > 0) { const parallelTasks = unconditionalEdges diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index 7dc6a1b1c1..f48cff461f 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -315,11 +315,25 @@ export const FlowEdgeSchema = lazySchema(() => z.object({ /** * Default Sequence Flow marker (BPMN Default Flow semantics). - * When true, this edge is taken when no sibling conditional edges match. - * Only meaningful on outgoing edges of decision/gateway nodes. + * + * When true, this edge is traversed only when NO sibling conditional edge of + * the same source node matched — the "otherwise" branch. A default edge is + * therefore not part of the unconditional parallel fan-out; when a conditional + * sibling wins, this edge's target records a `skipped` step instead. + * + * Enforced by `AutomationEngine.traverseNext` since #4414. It had promised + * exactly this since it was declared and had **zero readers** for as long: an + * author who marked the fallback edge got an ordinary unconditional edge that + * ran on every pass, alongside whichever branch actually matched. Combining it + * with `condition` on the same edge is self-contradictory (BPMN forbids a + * conditional default flow) and is flagged by the `os build` / `os validate` + * flow linter, as is a second default edge out of the same node. */ isDefault: z.boolean().default(false) - .describe('Marks this edge as the default path when no other conditions match'), + .describe( + 'BPMN default flow: traverse this edge only when no sibling conditional edge of the same ' + + 'source node matched. Mutually exclusive with `condition`; at most one per source node.', + ), }, { error: flowEdgeUnknownKeyError }).strict()); /** diff --git a/packages/spec/src/automation/schemaless-node-config.zod.ts b/packages/spec/src/automation/schemaless-node-config.zod.ts index 084d7d4e0e..99cfd81e9e 100644 --- a/packages/spec/src/automation/schemaless-node-config.zod.ts +++ b/packages/spec/src/automation/schemaless-node-config.zod.ts @@ -178,8 +178,14 @@ export type SubflowConfigParsed = z.infer; /** * One `decision` branch — what the executor reads per condition * (logic-nodes.ts): the first branch whose bare-CEL `expression` evaluates - * true wins, and the run continues down the out-edge labelled `label` - * (no match → the edge labelled `default`). + * true wins, and the run continues down the out-edge labelled `label`. When no + * branch matches, the run takes the declared fallback — the out-edge marked + * `isDefault: true`, or one literally labelled `default`. + * + * `label` must match an out-edge's `label` **exactly**. A label nothing claims + * cannot route: traversal logs a warning and falls back to considering every + * out-edge, and `os validate` reports it as `flow-branch-label-unmatched` + * (#4414 — every decision label in the repo used to miss, silently). * * The designer's branch rows also show a **Target** column — that is a * VIRTUAL column projected from the node's out-edges by the designer @@ -188,7 +194,7 @@ export type SubflowConfigParsed = z.infer; */ export const DecisionConditionSchema = lazySchema(() => z.object({ /** Branch label — must match an out-edge's `label` to route anywhere. */ - label: z.string().describe("Branch label; the winning branch resumes down the out-edge with this label ('true' expression = default/else path)"), + label: z.string().describe("Branch label; the winning branch resumes down the out-edge with this label (no match → the out-edge marked isDefault, or one labelled 'default')"), /** Bare-CEL predicate (ADR-0032) — `{…}` template braces are the #1491 trap. */ expression: z.string().describe('Bare CEL predicate deciding this branch'), })); @@ -198,15 +204,22 @@ export type DecisionCondition = z.input; /** * `decision` node config — what the executor reads. * - * A decision may also carry no `conditions` at all and rely purely on - * condition-bearing OUT-EDGES (`edge.condition`, evaluated by the engine's - * traversal) — that is the legacy shape. The legacy singular - * `config.condition` is a structural surface the engine parse-validates on - * every node at registration but the decision executor never reads; branching - * predicates live in `conditions[]` or on the edges. + * A decision may also carry no `conditions` at all and rely purely on the + * OUT-EDGES (`edge.condition` per branch + `isDefault` on the fallback, + * evaluated by the engine's traversal) — a plain BPMN exclusive gateway, and + * the shape every bundled example uses. A node that declares no `conditions` + * reports no branch at all, so nothing competes with the edges. + * + * Pick **one** mechanism per decision. Declaring `conditions` here *and* + * per-edge `condition`s means the node picks a branch and then that branch's + * edge re-decides — the double-declaration behind #4414. + * + * The legacy singular `config.condition` is a structural surface the engine + * parse-validates on every node at registration but the decision executor never + * reads; branching predicates live in `conditions[]` or on the edges. */ export const DecisionConfigSchema = lazySchema(() => z.object({ - /** Ordered branches; first true expression wins, else the `default`-labelled edge. */ + /** Ordered branches; first true expression wins, else the declared default edge. */ conditions: z.array(DecisionConditionSchema).optional() .describe('Ordered decision branches (first true expression wins; omit to branch purely on edge conditions)'), }));