fix(lint): flow-decision-unconditional-branch reports the decision that gates on nothing

.changeset/flow-decision-gates-on-nothing.md

@@ -0,0 +1,11 @@
+---
+"@objectstack/lint": patch
+---
+
+`flow-decision-unconditional-branch` now reports the decision that gates on nothing — the shape the rule used to skip.
+
+A `decision` whose out-edges carry no `condition` and no `isDefault`, and whose node declares no `config.conditions[]`, selects no branch at all: the automation engine's own decision executor reports no branch when `conditions[]` is empty, so traversal considers every out-edge and each successor runs on every pass. The gateway is decoration. The rule could not see that shape, because it was framed as "an unconditional edge undercuts a guarded one" and read zero guarded edges as nothing to undercut — so the strictly worse gateway was the one case that stayed silent, and it is the harder one to notice in review, because the node still says `type: 'decision'`.
+
+Same rule id, same `warning` tier, with its own message: it names the out-edges that run unconditionally and offers the three fixes (a `condition` per branch plus `isDefault: true` on the fallback, a `config.conditions[]` whose `label` matches an out-edge, or dropping `type: 'decision'` for the node the gateway already behaves as). The mixed shape — one guarded out-edge beside an unconditional one — keeps its existing wording and its single finding.
+
+Decisions that do declare their routing stay silent, including the two that are easiest to catch by mistake: an ordinary gateway with guarded edges, and a decision that routes by `config.conditions[]` labels alone with bare out-edges. A decision declaring a label no out-edge claims remains the gating `flow-branch-label-unmatched` on its own, with no second finding piled on the same node.

packages/lint/src/lint-flow-patterns.test.ts

@@ -893,7 +893,18 @@ describe('flow-error-label-not-fault (#3863)', () => {
   it.each(['decision', 'approval'])(
     "does NOT flag label:'error' out of a %s node — the label IS the branch selector",
     (sourceType) => {
-      expect(lintFlowPatterns(edgeFlow({ label: 'error' }, sourceType))).toHaveLength(0);
+      // Scoped to THIS rule since #16093. The assertion was a global zero,
+      // which held only because the fully-inert decision was invisible: the
+      // `decision` half of this fixture declares no `conditions[]` and no edge
+      // is guarded, so it now (correctly) draws a `flow-decision-unconditional-
+      // branch` warning of its own. A bare `label` is not a guard in either
+      // half of that rule — the mixed branch has always counted a labelled
+      // edge the decision cannot select as ungated — so exempting it here to
+      // keep the zero would make one rule id answer two ways. What this case
+      // tests is unchanged: an error-ish label out of a branching node is not
+      // the #3863 footgun.
+      const fnds = lintFlowPatterns(edgeFlow({ label: 'error' }, sourceType));
+      expect(fnds.filter((f) => f.rule === FLOW_ERROR_LABEL_NOT_FAULT)).toHaveLength(0);
     },
   );
 });
@@ -1013,8 +1024,15 @@ describe('flow-decision-unconditional-branch (#4414)', () => {
     })).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({
+  // #16093 — the fully-inert gateway, the shape the rule could not see. Until
+  // this case landed, `if (gated.length === 0) continue` dropped exactly the
+  // node whose declared branching is worst: a `decision` with NO edge
+  // `condition`, NO `isDefault` and NO `config.conditions[]`. The mixed case
+  // (one guarded edge, one not) was at least partially routed; this one was not
+  // routed at all, and it is the harder one to spot, because the node still
+  // says `type: 'decision'`.
+  it('flags a decision that gates on NOTHING — no edge guard, no conditions[]', () => {
+    const fnds = lintFlowPatterns({
       flows: [{
         name: 'plain',
         nodes: [{ id: 'start', type: 'start', config: {} }, { id: 'check', type: 'decision' }, { id: 'a', type: 'screen', config: {} }],
@@ -1023,9 +1041,152 @@ describe('flow-decision-unconditional-branch (#4414)', () => {
           { id: 'e2', source: 'check', target: 'a' },
         ],
       }],
+    }).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
+
+    expect(fnds).toHaveLength(1);
+    // Advisory, like the mixed shape: an unconditional fan-out is legal — the
+    // node is merely typed `decision` while behaving as a plain step. See the
+    // severity policy at the top of the rule module.
+    expect(fnds[0].severity).toBeUndefined();
+    expect(fnds[0].where).toContain("decision 'check'");
+    // Names the node's out-edge targets and what the decision gates on.
+    expect(fnds[0].message).toContain('gates on NOTHING');
+    expect(fnds[0].message).toContain("'a'");
+    expect(fnds[0].message).toContain('EVERY pass');
+    expect(fnds[0].hint).toContain('conditions');
+  });
+
+  // The wording is the whole point of keeping this under the SAME rule id: the
+  // fully-inert shape must NOT read as the mixed "guarded alongside unguarded"
+  // sentence, which would send the author looking for a guard that is not there.
+  it('does not describe the inert decision with the mixed-branch wording', () => {
+    const inert = 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' },
+        ],
+      }],
+    }).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
+    expect(inert[0].message).not.toContain('alongside');
+
+    const mixed = lintFlowPatterns(guardFlow()).filter(
+      (f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH,
+    );
+    expect(mixed[0].message).toContain('alongside');
+    // One finding each — the inert branch must not double-report the mixed one.
+    expect(mixed).toHaveLength(1);
+  });
+
+  it('flags EVERY out-edge of an inert decision in one finding, not one each', () => {
+    const fnds = lintFlowPatterns({
+      flows: [{
+        name: 'plain',
+        nodes: [
+          { id: 'start', type: 'start', config: {} },
+          { id: 'check', type: 'decision' },
+          { id: 'a', type: 'screen', config: {} },
+          { id: 'b', type: 'screen', config: {} },
+        ],
+        edges: [
+          { id: 'e1', source: 'start', target: 'check' },
+          { id: 'e2', source: 'check', target: 'a' },
+          { id: 'e3', source: 'check', target: 'b' },
+        ],
+      }],
+    }).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].message).toContain("'a'");
+    expect(fnds[0].message).toContain("'b'");
+  });
+
+  // As measured on the reporting app: the inert decision sat inside a `loop`
+  // body, so the finding has to survive region descent (#5383) too.
+  it('flags a nested inert decision inside a loop body', () => {
+    const fnds = lintFlowPatterns(loopBodyFlow({
+      nodes: [
+        { id: 'gate', type: 'decision' },
+        { id: 'nudge', type: 'create_record', config: { objectName: 'touch_log' } },
+      ],
+      edges: [{ id: 'b1', source: 'gate', target: 'nudge' }],
+    })).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH);
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].where).toBe("flow 'campaign_enrollment' · loop 'loop_leads' body · decision 'gate'");
+    expect(fnds[0].message).toContain("'nudge'");
+  });
+
+  // ⛔ NEGATIVE CONTROL — the ordinary gateway. Guarded edges AND a matching
+  // `config.conditions[]`: the shape every correct flow has, and the one a new
+  // positive case is most likely to break. It must stay silent.
+  it('does NOT flag a normal decision — guarded edges plus matching conditions[]', () => {
+    expect(lintFlowPatterns(guardFlow({
+      proceed: { isDefault: true },
+      conditions: [{ label: 'Yes', expression: "lead.status == 'converted'" }],
+    }))).toHaveLength(0);
+  });
+
+  // ⛔ NEGATIVE CONTROL — routing by `config.conditions[]` alone. The edges
+  // carry no `condition` and no `isDefault`, so `gated` is empty, but the
+  // decision DOES declare its branching: the labels select the path. Not inert.
+  it('does NOT flag a decision that routes by conditions[] alone', () => {
+    expect(lintFlowPatterns({
+      flows: [{
+        name: 'by_label',
+        nodes: [
+          { id: 'start', type: 'start', config: {} },
+          {
+            id: 'check', type: 'decision',
+            config: { conditions: [{ label: 'Yes', expression: 'lead.score > 50' }, { label: 'No', expression: 'true' }] },
+          },
+          { id: 'a', type: 'screen', config: {} },
+          { id: 'b', type: 'screen', config: {} },
+        ],
+        edges: [
+          { id: 'e1', source: 'start', target: 'check' },
+          { id: 'e2', source: 'check', target: 'a', label: 'Yes' },
+          { id: 'e3', source: 'check', target: 'b', label: 'No' },
+        ],
+      }],
     })).toHaveLength(0);
   });
 
+  // ⛔ NO DOUBLE REPORT — a decision that declares `conditions[]` no out-edge
+  // claims is already the GATING `flow-branch-label-unmatched`; the inert branch
+  // must not pile a second, contradictory finding on the same node.
+  it('leaves an unmatched-label decision to flow-branch-label-unmatched alone', () => {
+    const fnds = lintFlowPatterns({
+      flows: [{
+        name: 'unmatched',
+        nodes: [
+          { id: 'start', type: 'start', config: {} },
+          { id: 'check', type: 'decision', config: { conditions: [{ label: 'Yes', expression: 'true' }] } },
+          { id: 'a', type: 'screen', config: {} },
+        ],
+        edges: [
+          { id: 'e1', source: 'start', target: 'check' },
+          { id: 'e2', source: 'check', target: 'a' },
+        ],
+      }],
+    });
+    expect(fnds.filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0);
+    expect(fnds.filter((f) => f.rule === FLOW_BRANCH_LABEL_UNMATCHED)).toHaveLength(1);
+  });
+
+  // A decision with no out-edge at all is a different defect and is skipped
+  // before any of this (`outs.length === 0`); the inert branch must not claim it.
+  it('does NOT flag a decision with no out-edge at all', () => {
+    expect(lintFlowPatterns({
+      flows: [{
+        name: 'dead_end',
+        nodes: [{ id: 'start', type: 'start', config: {} }, { id: 'check', type: 'decision' }],
+        edges: [{ id: 'e1', source: 'start', target: 'check' }],
+      }],
+    }).filter((f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH)).toHaveLength(0);
+  });
+
+
   it('does NOT flag a fault edge — error routing is not branch selection', () => {
     expect(lintFlowPatterns(guardFlow({
       proceed: { isDefault: true },

packages/lint/src/lint-flow-patterns.ts

@@ -30,9 +30,11 @@
  *
  * The bar is deliberately about *provability*, not severity of consequence. A
  * shape with a legitimate reading stays a warning even when it is usually a
- * mistake — {@link FLOW_DECISION_UNCONDITIONAL_BRANCH} is normally a guard that
- * does not guard, but a decision with one guarded and one unconditional out-edge
- * is a legal "maybe notify, always continue" fan-out, and
+ * mistake — {@link FLOW_DECISION_UNCONDITIONAL_BRANCH} covers a guard that does
+ * not guard AND a decision that declares no branching at all, yet a decision
+ * with one guarded and one unconditional out-edge is a legal "maybe notify,
+ * always continue" fan-out, an all-unconditional one is that same fan-out under
+ * a mis-chosen node type rather than a route that is broken, and
  * {@link FLOW_MULTIPLE_DEFAULT_EDGES} can genuinely mean "when nothing matched,
  * do both". Failing a customer's build on a shape we cannot prove wrong is a
  * worse trade than letting the warning be ignored.
@@ -720,10 +722,21 @@ function scanErrorLabelledEdges(
  *      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.
+ *  (2) `flow-decision-unconditional-branch` — an out-edge nothing can gate: 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. #16093 gave the same id a second
+ *      message for the strictly worse shape it could not previously see: NO
+ *      out-edge gated and no `conditions[]` declared either. The rule was framed
+ *      as "an unconditional edge undercuts a guarded one", so zero guarded edges
+ *      read as nothing to undercut and the node was skipped — but a decision
+ *      with no guard anywhere does not have less wrong with it, it selects no
+ *      branch at all: every successor runs on every pass and the gateway is
+ *      decoration. It is also the harder shape to notice in review, because the
+ *      node still says `type: 'decision'`. A decision routing by
+ *      `config.conditions[]` labels alone has declared its branching on the node
+ *      and is not this shape; (1) already owns it when a declared label goes
+ *      unclaimed.
  *  (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.
@@ -737,8 +750,10 @@ function scanErrorLabelledEdges(
  * (1) and (3) GATE — neither has a reading under which the author's metadata
  * routes what it says, on any run, so a warning would just be a slower way of
  * finding out. (2) and (4) stay advisory: an unconditional sibling is a legal
- * "maybe notify, always continue" fan-out, and two defaults can mean "when
- * nothing matched, do both". See the severity policy at the top of this file.
+ * "maybe notify, always continue" fan-out, an all-unconditional decision is that
+ * same fan-out written on the wrong node type — the successors it names do run,
+ * every one of them, every pass — and two defaults can mean "when nothing
+ * matched, do both". See the severity policy at the top of this file.
  *
  * The engine also warns when it hits (1) live — a stored flow authored before
  * this rule existed still reaches run time.
@@ -874,7 +889,38 @@ function scanBranchRouting(
     // (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 declaresConditions = Array.isArray(cfg.conditions) && (cfg.conditions as unknown[]).length > 0;
+
+    if (gated.length === 0) {
+      // (2a) #16093 — the decision gates on NOTHING. The old framing read this
+      //      as "no branching declared at all, nothing to undercut" and skipped
+      //      it; that reads zero guarded edges as an absence when it is the
+      //      defect. A decision routing by `config.conditions[]` labels alone
+      //      is NOT inert — its branching is declared on the node — and (1)
+      //      above already reports it when no out-edge claims a declared label,
+      //      so it is excluded here rather than reported twice.
+      if (!declaresConditions) {
+        findings.push({
+          where: `${at} · decision '${nid}'`,
+          message:
+            `gates on NOTHING — none of its out-edge(s) ` +
+            `(${outs.map((e) => `'${String(e.target)}'`).join(', ')}) carries a \`condition\` or ` +
+            `\`isDefault\`, and the decision declares no \`config.conditions[]\`. With no branch to ` +
+            `select, EVERY out-edge is traversed on EVERY pass, in parallel — the node is typed ` +
+            `\`decision\` but routes exactly as a non-decision step would, so the gateway is decoration.`,
+          hint:
+            `Declare the branching one way and one way only: a \`condition\` per branch plus ` +
+            `\`isDefault: true\` on the fallback, or \`config.conditions[]\` whose \`label\` matches an ` +
+            `out-edge's. If the unconditional fan-out is what you meant, drop \`type: 'decision'\` and ` +
+            `use the node it already behaves as.`,
+          rule: FLOW_DECISION_UNCONDITIONAL_BRANCH,
+        });
+      }
+      // Either way no out-edge is gated, so the mixed-branch check below — an
+      // unconditional edge undercutting a guarded sibling — has no sibling to
+      // read and would only report the same node a second time.
+      continue;
+    }
     const ungated = outs.filter(
       (e) => !e.condition && e.isDefault !== true && !declaredLabels.has(edgeLabelOf(e)),
     );