fix(automation): enforce isDefault and stop swallowing an unclaimable branch label (#4414)

.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.

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`.
+
+<Callout type="warn">
+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.
+</Callout>
 
 ### Fault edges — handling a failed node
 

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. |
 
 
 ---

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 |
 
 

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' },

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<string, unknown>;
+  extra?: Array<Record<string, unknown>>;
+} = {}) {
+  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);
+  });
+});