From ffae2ae3596689698cc8c8e24afd219e5f1d475a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 07:24:47 +0000 Subject: [PATCH 1/3] fix(automation): enforce isDefault and stop swallowing an unclaimable branch label (#4414) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `decision` node advertised three ways to split a path and only `edge.condition` did anything. `FlowEdgeSchema.isDefault` had zero readers outside its own declaration, and the `conditions[].label` → `branchLabel` route matched 0 out-edge labels across every example app before falling back to the full edge set in silence. Stacked, the two shipped a guard that does not guard: `crm_convert_lead_wizard` showed an already-converted lead the abort screen AND walked it into the conversion wizard behind it. The three mechanisms now compose as one model in `traverseNext`: `branchLabel` narrows the edge set, `condition` gates each edge, `isDefault` catches whatever is left. - `isDefault` is enforced as the BPMN default flow: traversed only when no conditional sibling matched, and kept out of the unconditional parallel fan-out. Passed over because a real branch won, its target records the same `skipped` step a closed gate does (#4354). - A branch label no out-edge carries is logged instead of swallowed. Traversal still falls back to the full edge set — a run mid-flight must not die on a metadata error — but says which branch was computed and which labels exist. - A decision that declares no `conditions` reports no branch. It used to report `'default'` regardless, a label no out-edge in the repo carried, which is why every decision node fell back to the full edge set. The sentinel survives for the case it describes (declared conditions, none matched) and is now claimed by the `isDefault` edge as well. - `conditions[].expression` is evaluated as the bare CEL it is declared to be. The raw string went to the legacy `{var}` template path, where a dotted reference cannot resolve and the branch is decided by string comparison; a brace-in-CEL predicate now fails loudly (ADR-0032 §1c). Caught at authoring time too, since a wrong route is silent at run time by nature (Prime Directive #12): `flow-branch-label-unmatched`, `flow-decision-unconditional-branch`, `flow-default-edge-with-condition` and `flow-multiple-default-edges`. The first two fire on the pre-fix `convert-lead.flow.ts` and are silent after it. app-crm's guard is now a plain exclusive gateway — the redundant `config.conditions` is gone and `e3b` carries `isDefault: true`, so exactly one branch runs. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9 --- .../decision-branch-routing-enforced.md | 86 ++++++ content/docs/automation/flows.mdx | 55 +++- .../app-crm/src/flows/convert-lead.flow.ts | 27 +- .../cli/src/utils/lint-flow-patterns.test.ts | 157 +++++++++++ packages/cli/src/utils/lint-flow-patterns.ts | 151 +++++++++++ .../builtin/decision-branch-routing.test.ts | 252 ++++++++++++++++++ .../src/builtin/logic-nodes.ts | 33 ++- .../services/service-automation/src/engine.ts | 120 ++++++++- packages/spec/src/automation/flow.zod.ts | 20 +- .../automation/schemaless-node-config.zod.ts | 33 ++- 10 files changed, 898 insertions(+), 36 deletions(-) create mode 100644 .changeset/decision-branch-routing-enforced.md create mode 100644 packages/services/service-automation/src/builtin/decision-branch-routing.test.ts 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..0a64920ada 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -731,7 +731,60 @@ 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 — which is +how the mismatch above shipped silently. `os validate` reports the same 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/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)'), })); From 0875c2b1fe19f77fe7b0bfeda4ca0741dececfdf Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 07:26:12 +0000 Subject: [PATCH 2/3] docs(automation): name the #4414 shape in the branching section Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9 --- content/docs/automation/flows.mdx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 0a64920ada..7d344b7f5a 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -773,9 +773,10 @@ edges: [ 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 — which is -how the mismatch above shipped silently. `os validate` reports the same shape as -`flow-branch-label-unmatched` at build time, along with +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`. From 09ed4cda5b8c498264838c711b03a3fb3113bb4d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 07:33:21 +0000 Subject: [PATCH 3/3] docs(spec): regenerate the flow/decision reference pages after the isDefault contract change `content/docs/references/` is generated from `packages/spec`; the #4414 `.describe()` rewrites left flow.mdx and schemaless-node-config.mdx stale, which is what `check:docs` caught. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q8as8yR67v41xEdomiTba9 --- content/docs/references/automation/flow.mdx | 2 +- content/docs/references/automation/schemaless-node-config.mdx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 |