diff --git a/.changeset/flow-decision-gates-on-nothing.md b/.changeset/flow-decision-gates-on-nothing.md new file mode 100644 index 0000000000..b80ccbc15d --- /dev/null +++ b/.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. diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index 3960a6a398..171c712412 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/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 }, diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index d7ba8f73c8..03215ed9fd 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/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)), );