fix(cli): gate the two decision-routing shapes that can never work, flag the inert config.condition (#4414)

.changeset/flow-branch-gates-and-inert-condition.md

@@ -0,0 +1,61 @@
+---
+"@objectstack/cli": minor
+"@objectstack/example-showcase": patch
+"@objectstack/example-todo": patch
+---
+
+fix(cli): gate the two decision-routing shapes that can never work, and flag the inert `config.condition` (#4414)
+
+Two follow-ups to #4440, both about metadata that reads like a guard and is not
+one.
+
+## Two rules promoted to `error`
+
+`flow-branch-label-unmatched` and `flow-default-edge-with-condition` now FAIL the
+build instead of warning. The bar for that — restated at the top of
+`lint-flow-patterns.ts`, because the old one no longer described the set — is
+**no reading of the author's metadata does what it says, deterministically, on
+every run**. Both qualify: a branch label no out-edge carries cannot route, and
+an edge that is both `isDefault` and conditional always lets the condition win,
+so the marker routes nothing. Neither *fails*; both are wrong every time and
+silently, which is worse.
+
+The other two stay advisory on purpose, and the policy now says why:
+`flow-decision-unconditional-branch` is usually a guard that does not guard, but
+one guarded plus one unconditional out-edge is also a legal "maybe notify,
+always continue" fan-out, and `flow-multiple-default-edges` can genuinely mean
+"when nothing matched, do both". The bar is about *provability*, not severity of
+consequence — failing a customer's build on a shape we cannot prove wrong is the
+worse trade.
+
+No wiring change was needed: `lintFlowPatterns` is already registered as
+`tier: 'gating'` across all three commands (#4409), which is exactly the seam
+`authoring-rule-wiring.test.ts` exists to guard.
+
+## New rule: `flow-inert-node-condition`
+
+`config.condition` is the trigger gate on a `start` node and is read by **no
+other node type** — the engine parse-validates it everywhere (so a malformed one
+is caught) and then ignores it. On a `decision` the name makes it read as the
+branch predicate, which is exactly how it got authored.
+
+Three of the three bundled apps had one. `app-todo`'s `check_recurring` and
+`app-showcase`'s `needs_exec` both carried a predicate their out-edges were
+already enforcing — a third copy doing nothing. The showcase even had a comment
+next to it saying the node condition "is not evaluated by the engine", and kept
+it anyway; that is the residue this rule exists to stop accumulating. Both are
+now plain exclusive gateways.
+
+Advisory, not gating: the surrounding edges usually still route correctly, so
+this is dead weight rather than a provable misroute. The node-type list is a
+closed set of builtins we have actually read, not "everything that isn't
+`start`" — ADR-0018 keeps `node.type` open and a plugin executor may legitimately
+declare and read its own `config.condition`.
+
+## Studio
+
+`objectstack-ai/objectui` carries the matching help-text fixes: the branch editor
+said a `true` branch **is** the default/else path (it is how you *ask* for one —
+the marker goes on the out-edge), and the legacy single `Condition` field said
+"prefer Branches above", which reads as "this works, but the other is better".
+It does not work at all.

examples/app-showcase/src/automation/flows/index.ts

@@ -211,12 +211,12 @@ export const BudgetApprovalFlow = defineFlow({
       // load, but the showcase should demonstrate the declared spelling.
       waitEventConfig: { eventType: 'signal', signalName: 'budget_revision' },
     },
-    {
-      id: 'needs_exec',
-      type: 'decision',
-      label: 'Budget Above $500k?',
-      config: { condition: 'budget > 500000' },
-    },
+    // A plain exclusive gateway: the predicate is on the out-edges (e4/e5).
+    // It also carried `config.condition` — inert on every node but `start`, and
+    // the comment on those edges already said so. Keeping a copy that nothing
+    // reads is the shape #4414 is about, so it is gone; `os validate` reports
+    // it as `flow-inert-node-condition`.
+    { id: 'needs_exec', type: 'decision', label: 'Budget Above $500k?' },
     {
       id: 'exec_review',
       type: 'approval',
@@ -236,10 +236,11 @@ export const BudgetApprovalFlow = defineFlow({
     { id: 'e1', source: 'start', target: 'manager_review' },
     { id: 'e2', source: 'manager_review', target: 'needs_exec', label: 'approve' },
     { id: 'e3', source: 'manager_review', target: 'rejected', label: 'reject' },
-    // Decision branching is edge-condition driven (flow spec): the engine
-    // routes a decision node by evaluating each out-edge's `condition`. Carry
-    // the predicate on the edges (the node `config.condition` alone is not
-    // evaluated by the engine), so budgets ≤ $500k skip the executive step.
+    // Decision branching is edge-condition driven: the engine routes a decision
+    // by evaluating each out-edge's `condition`, so the predicate lives here and
+    // budgets ≤ $500k skip the executive step. These two are complementary, so
+    // exactly one runs; the other correct spelling is one `condition` plus
+    // `isDefault: true` on the fallback edge (#4414).
     { id: 'e4', source: 'needs_exec', target: 'exec_review', label: 'true', condition: 'budget > 500000' },
     { id: 'e5', source: 'needs_exec', target: 'approved', label: 'false', condition: 'budget <= 500000' },
     { id: 'e6', source: 'exec_review', target: 'approved', label: 'approve' },

examples/app-todo/src/flows/task.flow.ts

@@ -131,10 +131,12 @@ export const TaskCompletionFlow: Flow = {
       id: 'get_task', type: 'get_record', label: 'Get Completed Task',
       config: { objectName: 'todo_task', filter: { id: '{taskId}' }, outputVariable: 'completedTask' },
     },
-    {
-      id: 'check_recurring', type: 'decision', label: 'Is Recurring Task?',
-      config: { condition: 'vars.completedTask.is_recurring == true' },
-    },
+    // A plain exclusive gateway — the branching is on the OUT-EDGES (e3/e4
+    // carry the predicate and its negation). It used to also set
+    // `config.condition`, which no executor reads: that key is the trigger gate
+    // on a `start` node and inert everywhere else, so it was a third copy of the
+    // same predicate, doing nothing (#4414).
+    { id: 'check_recurring', type: 'decision', label: 'Is Recurring Task?' },
     {
       id: 'create_next_task', type: 'create_record', label: 'Create Next Recurring Task',
       config: {

packages/cli/src/lint/authoring-rules.ts

@@ -419,8 +419,11 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [
     run: (stack) => validateVisibilityPredicates(stack),
   },
   // #1874 — flow authoring anti-patterns. Advisory by default; a finding marked
-  // `error` gates (#3760 promoted `flow-runas-unscoped`, which flags metadata
-  // the runtime now REFUSES to execute).
+  // `error` gates. Three do today: `flow-runas-unscoped` (#3760 — metadata the
+  // runtime REFUSES to execute), plus `flow-branch-label-unmatched` and
+  // `flow-default-edge-with-condition` (#4414 — a declaration that is inert, so
+  // the route silently differs from what the author wrote). The bar for
+  // promoting one is stated at the top of `lint-flow-patterns.ts`.
   {
     name: 'lintFlowPatterns',
     tier: 'gating',

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

@@ -17,6 +17,7 @@ import {
   FLOW_DECISION_UNCONDITIONAL_BRANCH,
   FLOW_DEFAULT_EDGE_WITH_CONDITION,
   FLOW_MULTIPLE_DEFAULT_EDGES,
+  FLOW_INERT_NODE_CONDITION,
 } from './lint-flow-patterns.js';
 
 const CEL = (source: string) => ({ dialect: 'cel', source });
@@ -189,10 +190,16 @@ describe('lintFlowPatterns — wrong interpolation syntax (#1315)', () => {
       expect(rules(nodeFlow({ objectName: 'm', fields: { price: '$5.00', label: 'Total $5' } }))).toEqual([]);
     });
     it('a CEL condition (skipped — not a template value)', () => {
-      expect(rules({ flows: [{ name: 'd', nodes: [
+      // Scoped to the #1315 interpolation rules on purpose: this shape DOES
+      // trip `flow-inert-node-condition` (#4414 — a decision never reads
+      // `config.condition`), a different finding about a different defect,
+      // which must not make this case read as a brace mistake.
+      const found = rules({ flows: [{ name: 'd', nodes: [
         { id: 'start', type: 'start', config: {} },
         { id: 'dec', type: 'decision', config: { condition: 'record.amount > 100' } },
-      ], edges: [] }] })).toEqual([]);
+      ], edges: [] }] });
+      expect(found).not.toContain(FLOW_DOUBLE_BRACE_INTERP);
+      expect(found).not.toContain(FLOW_BARE_DOLLAR_REF);
     });
   });
 });
@@ -500,6 +507,8 @@ describe('flow-branch-label-unmatched (#4414)', () => {
     })).filter((f) => f.rule === FLOW_BRANCH_LABEL_UNMATCHED);
 
     expect(fnds).toHaveLength(1);
+    // Gating: a label nothing claims cannot route under any reading (#4414).
+    expect(fnds[0].severity).toBe('error');
     expect(fnds[0].where).toContain("decision 'check'");
     expect(fnds[0].message).toContain("'yes — already converted'");
     expect(fnds[0].message).toContain("'no — proceed'");
@@ -541,6 +550,10 @@ describe('flow-decision-unconditional-branch (#4414)', () => {
       (f) => f.rule === FLOW_DECISION_UNCONDITIONAL_BRANCH,
     );
     expect(fnds).toHaveLength(1);
+    // Advisory, deliberately: one guarded + one unconditional out-edge is also
+    // a legal "maybe notify, always continue" fan-out, so this shape cannot be
+    // proved wrong the way the two gating rules can.
+    expect(fnds[0].severity).toBeUndefined();
     expect(fnds[0].message).toContain("'proceed'");
     expect(fnds[0].message).toContain('EVERY pass');
     expect(fnds[0].hint).toContain('isDefault');
@@ -589,6 +602,8 @@ describe('flow-default-edge-with-condition / flow-multiple-default-edges (#4414)
       proceed: { isDefault: true, condition: "lead.status != 'converted'" },
     })).filter((f) => f.rule === FLOW_DEFAULT_EDGE_WITH_CONDITION);
     expect(fnds).toHaveLength(1);
+    // Gating: the condition always wins, so the marker never routes (#4414).
+    expect(fnds[0].severity).toBe('error');
     expect(fnds[0].message).toContain('contradictory');
   });
 
@@ -598,10 +613,76 @@ describe('flow-default-edge-with-condition / flow-multiple-default-edges (#4414)
       extra: [{ id: 'e_also', source: 'check', target: 'abort', isDefault: true }],
     })).filter((f) => f.rule === FLOW_MULTIPLE_DEFAULT_EDGES);
     expect(fnds).toHaveLength(1);
+    // Advisory: two defaults can genuinely mean "when nothing matched, do both".
+    expect(fnds[0].severity).toBeUndefined();
     expect(fnds[0].where).toContain("node 'check'");
   });
 
   it('does NOT flag one default edge per node', () => {
     expect(lintFlowPatterns(guardFlow({ proceed: { isDefault: true } }))).toHaveLength(0);
   });
 });
+
+/**
+ * #4414 — `config.condition` on a node that never reads it.
+ *
+ * The key is LIVE on `start` (the trigger gate) and dead on every other
+ * builtin. `app-todo`'s `check_recurring` carried one for years: a third copy
+ * of a predicate its out-edges were already enforcing.
+ */
+function conditionNodeFlow(nodeType: string, config: Record<string, unknown>) {
+  return {
+    flows: [{
+      name: 'cond_flow',
+      nodes: [
+        { id: 'start', type: 'start', config: { objectName: 'todo_task', triggerType: 'record-after-update' } },
+        { id: 'n', type: nodeType, config },
+      ],
+      edges: [{ id: 'e1', source: 'start', target: 'n' }],
+    }],
+  };
+}
+
+describe('flow-inert-node-condition (#4414)', () => {
+  it('flags `config.condition` on a decision, pointing at the out-edges', () => {
+    const fnds = lintFlowPatterns(
+      conditionNodeFlow('decision', { condition: 'vars.completedTask.is_recurring == true' }),
+    ).filter((f) => f.rule === FLOW_INERT_NODE_CONDITION);
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].where).toContain("node 'n' (decision)");
+    expect(fnds[0].message).toContain('nothing reads it');
+    expect(fnds[0].hint).toContain('isDefault');
+    // Advisory: the surrounding edges usually still route correctly.
+    expect(fnds[0].severity).toBeUndefined();
+  });
+
+  it('flags it on a non-decision node too, with the generic hint', () => {
+    const fnds = lintFlowPatterns(
+      conditionNodeFlow('update_record', { objectName: 'todo_task', condition: 'a == b' }),
+    ).filter((f) => f.rule === FLOW_INERT_NODE_CONDITION);
+    expect(fnds).toHaveLength(1);
+    expect(fnds[0].hint).toContain("incoming edge's `condition`");
+  });
+
+  it('does NOT flag the start node — that is where the key is read', () => {
+    expect(lintFlowPatterns({
+      flows: [{
+        name: 'gated',
+        runAs: 'system',
+        nodes: [{ id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 9 * * *', condition: 'record.active == true' } }],
+        edges: [],
+      }],
+    }).filter((f) => f.rule === FLOW_INERT_NODE_CONDITION)).toHaveLength(0);
+  });
+
+  it('does NOT flag a node with no condition, or an empty one', () => {
+    expect(lintFlowPatterns(conditionNodeFlow('decision', {}))).toHaveLength(0);
+    expect(lintFlowPatterns(conditionNodeFlow('decision', { condition: '   ' }))).toHaveLength(0);
+  });
+
+  it('does NOT flag a PLUGIN node type — its executor may legitimately read it', () => {
+    // ADR-0018 keeps `node.type` open; we can only prove the key inert for the
+    // builtins we ship.
+    expect(lintFlowPatterns(conditionNodeFlow('acme_custom_step', { condition: 'a == b' }))).toHaveLength(0);
+  });
+});