fix(automation): evaluateCondition decides the dialect from the source, not from the caller (#4336)

.changeset/flow-condition-bare-string-is-cel.md

@@ -0,0 +1,53 @@
+---
+"@objectstack/service-automation": minor
+---
+
+fix(automation): `evaluateCondition` decides the dialect from the source, not from the caller (#4336)
+
+`AutomationEngine.evaluateCondition` picked its engine by asking whether an
+`{ dialect, source }` **envelope** was present. A condition handed to it as a
+plain string therefore never reached the CEL engine: it fell through to the
+legacy `{var}` template path, which substitutes brace holes and then compares
+whatever text is left — **as text**. Nothing errored, and the run was recorded
+as `success`, with the failure direction depending on the predicate:
+
+| Handed in | Actually evaluated | Result |
+|:---|:---|:---|
+| `existingTask == null` | `'existingTask' === 'null'` | always **false** — gate never opens |
+| `record.rating >= 4` | `'record.rating' >= '4'` → `'r' > '4'` | always **true** — branch pinned open |
+
+#4414 fixed the one built-in that was reaching this — the `decision` executor
+now wraps `conditions[].expression` in a CEL envelope before calling. This
+fixes the **evaluator**, so the next caller does not have to remember: the
+dialect is now read from the source, and a condition is CEL unless it actually
+contains a `{var}` hole. `evaluateCondition` is public API, so a
+plugin-registered node executor evaluating its own predicate was getting the
+table above with nothing to warn it.
+
+**The legacy `{var}` dialect keeps working** where it always did —
+`{amount} > 100`, `{status} == active`, `{a.b} == 7` — and gains the two things
+it was missing:
+
+- **A quoted literal compares as its contents.** `{status} == 'active'` used to
+  compare `active` against `'active'` — quotes included — and was false for
+  every value of `status`. It is the spelling the flow docs showed, and quoting
+  a string literal is what every other predicate surface requires.
+- **It no longer answers `false` when it could not resolve something.** A `{…}`
+  hole matching no flow variable (`{lead_record.status}` — `get_record` stores
+  the whole row under one name, so that key never exists) and a substituted
+  value that is neither a boolean, a number, nor part of a comparison are
+  refused with the source and the offending reference attached. Both used to be
+  a silent `false`, which ADR-0032 §1c forbids: a predicate that cannot be
+  evaluated is a fault, never a quiet branch decision.
+
+Braces inside an explicit `dialect: 'cel'` envelope remain the #1491 brace-trap
+and still throw — stating the dialect is the author saying "this is CEL". The
+sniff reads the source outside string literals, so `record.label == '{pending}'`
+stays CEL and compares the field.
+
+**Tightening to know about:** a bare string that is not valid CEL now raises
+where it previously string-compared to some answer. That includes the
+host-language payloads the safety tests use (`process.exit(1)`,
+`require("fs")…`) — nothing executed before and nothing executes now, since CEL
+has no `process`, no `require` and no arrow functions, but the failure is a
+reported fault instead of a silent `false`.

content/docs/automation/flows.mdx

@@ -31,10 +31,10 @@ const approvalFlow = {
       type: 'decision',
       label: 'Check Amount',
       config: {
-        // decision expressions use {braces} around variables — see Expressions in flows
+        // decision expressions are bare CEL, like every other condition — no braces
         conditions: [
-          { label: 'High Value', expression: '{order_amount} > 10000' },
-          { label: 'Standard', expression: '{order_amount} <= 10000' },
+          { label: 'High Value', expression: 'order_amount > 10000' },
+          { label: 'Standard', expression: 'order_amount <= 10000' },
         ],
       },
     },
@@ -169,8 +169,8 @@ or missing-`required` violation (#4277). A node type that publishes no
   label: 'Check Status',
   config: {
     conditions: [
-      { label: 'Approved', expression: "{status} == 'approved'" },
-      { label: 'Rejected', expression: "{status} == 'rejected'" },
+      { label: 'Approved', expression: "status == 'approved'" },
+      { label: 'Rejected', expression: "status == 'rejected'" },
     ],
   },
 }
@@ -972,30 +972,40 @@ failures so one broken flow does not abort startup.
 
 ## Expressions in flows
 
-A flow mixes **three expression dialects**, and using the wrong one is the
-single most common way a flow silently misbehaves. Which dialect applies is
-decided by *where* the expression sits — not by what it looks like:
+A flow mixes **two expression dialects**, and the rule is short: **every
+condition is CEL; braces are for values.**
 
 | Where | Dialect | Write it like | Bindings |
 |:---|:---|:---|:---|
 | Start-node `condition` | **CEL** (bare, no braces) | `record.amount > 500` | `record.*`, `previous.*`, bare field names, `vars.*` |
 | Edge `condition` | **CEL** (bare, no braces) | `record.status == 'open'` | same as above |
-| Decision-node `conditions[].expression` | **Template compare** (braces required) | `{order_amount} > 10000` | flow variables by name, in `{…}` |
+| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | same as above |
 | Field values in `create_record` / `update_record` | **Interpolation** (braces required) | `'Follow up on {record.name}'`, `'{TODAY() + 7}'` | `{var}`, `{var.path}`, `{$User.Id}`, `{$User.Email}`, `{NOW()}`, `{TODAY()}`, `{TODAY() + 90}` (whole days) |
 
 <Callout type="warn">
-**The two failure modes to memorize:**
+**The failure modes to memorize:**
 
-1. **Braces missing in a decision expression** — `'order_amount > 10000'` isn't
-   evaluated as a variable at all. It compares the *string* `"order_amount"`
-   against `"10000"`, which is **always true**, so the flow always takes the
-   first branch and never tells you. Write `'{order_amount} > 10000'`.
-2. **Braces missing in a field value** — `due_date: 'TODAY() + 7'` writes the
+1. **Braces missing in a field value** — `due_date: 'TODAY() + 7'` writes the
    literal text `TODAY() + 7` into the field. Write `'{TODAY() + 7}'`.
+2. **Braces put *into* a condition** — `'{record.amount} > 500'`. Conditions
+   fail loudly rather than silently, with an error that tells you to drop the
+   braces.
+</Callout>
 
-The mirror mistake is putting braces *into* a CEL condition
-(`'{record.amount} > 500'`) — CEL conditions fail loudly rather than silently,
-with an error that tells you to drop the braces.
+<Callout type="info">
+**Decision-node expressions used to be compared as text** (#4414, #4336), so
+`'order_amount > 10000'` compared the string `"order_amount"` against `"10000"`
+and was **always true**, while `'{lead_record.status} == "converted"'` was
+**always false** — the brace form substitutes a whole flow variable by name, and
+a field access on an object variable is not one. Both reported `success`. They
+are bare CEL now, so the spellings in the table above are the correct ones and
+`lead_record.status == 'converted'` resolves the field.
+
+The `{var}` form still works where it always did — `{amount} > 100`,
+`{status} == 'active'` — but the two ways it used to answer `false` without
+saying so are now **loud errors** naming the reference: a `{…}` hole that
+matches no flow variable, and a substituted value that is neither a boolean, a
+number, nor part of a comparison (#4336).
 </Callout>
 
 CEL conditions that fail to evaluate raise an error and stop the run — they

packages/services/service-automation/src/engine.test.ts

@@ -1905,10 +1905,15 @@ describe('AutomationEngine - Safe Expression Evaluation', () => {
 
     it('should not execute malicious code', () => {
         const vars = new Map<string, unknown>();
-        // These should all return false safely
-        expect(engine.evaluateCondition('process.exit(1)', vars)).toBe(false);
-        expect(engine.evaluateCondition('require("fs").readFileSync("/etc/passwd")', vars)).toBe(false);
-        expect(engine.evaluateCondition('(() => { while(true) {} })()', vars)).toBe(false);
+        // None of these is a host-language program to this engine — there is no
+        // `new Function`, no `eval`, no `require` on either path. They REFUSE
+        // rather than return `false` since #4336: a brace-free condition is CEL,
+        // and CEL has no `process`, no `require`, and no arrow-function syntax,
+        // so each one is a fault the run reports. The safety property is
+        // unchanged (nothing executes) and the diagnosis is no longer silent.
+        expect(() => engine.evaluateCondition('process.exit(1)', vars)).toThrow(/exit/);
+        expect(() => engine.evaluateCondition('require("fs").readFileSync("/etc/passwd")', vars)).toThrow(/require/);
+        expect(() => engine.evaluateCondition('(() => { while(true) {} })()', vars)).toThrow(/source:/);
     });
 
     it('should handle string comparisons', () => {
@@ -1917,6 +1922,106 @@ describe('AutomationEngine - Safe Expression Evaluation', () => {
 
         expect(engine.evaluateCondition('{status} == active', vars)).toBe(true);
         expect(engine.evaluateCondition('{status} != inactive', vars)).toBe(true);
+        // #4336 — a QUOTED literal on the right compares as its contents. This is
+        // the spelling the flow docs show for a decision node, and it used to
+        // compare `active` against `'active'` (quotes included) and be false for
+        // every value of `status`.
+        expect(engine.evaluateCondition("{status} == 'active'", vars)).toBe(true);
+        expect(engine.evaluateCondition('{status} == "active"', vars)).toBe(true);
+        expect(engine.evaluateCondition("{status} == 'closed'", vars)).toBe(false);
+        expect(engine.evaluateCondition("{status} != 'closed'", vars)).toBe(true);
+    });
+});
+
+// ─── #4336: a bare-string condition is CEL, not a string compare ─────
+//
+// The reported defect: `evaluateCondition` branched on whether an `Expression`
+// envelope was present, so a condition authored as a plain string never reached
+// the CEL engine and both sides were compared as TEXT. Every case below is one
+// of the two failure directions from the issue (a gate that never opens, a
+// branch pinned open) or one of the two silent `false`s found afterwards.
+describe('AutomationEngine - bare-string conditions evaluate as CEL (#4336)', () => {
+    let engine: AutomationEngine;
+    beforeEach(() => { engine = new AutomationEngine(createTestLogger()); });
+
+    it('opens a null-check gate that used to be pinned shut', () => {
+        // `'existingTask' === 'null'` → false, forever. The flow selected its
+        // records, took no branch, and recorded `success`.
+        expect(engine.evaluateCondition('existingTask == null', new Map([['existingTask', null]]))).toBe(true);
+        expect(engine.evaluateCondition('existingTask == null', new Map([['existingTask', { id: 'a' }]]))).toBe(false);
+    });
+
+    it('gates a numeric comparison that used to be pinned open', () => {
+        // `'record.rating' >= '4'` → `'r' > '4'` → true for every record.
+        const low = new Map<string, unknown>([['record', { rating: 2 }]]);
+        const high = new Map<string, unknown>([['record', { rating: 5 }]]);
+        expect(engine.evaluateCondition('record.rating >= 4', high)).toBe(true);
+        expect(engine.evaluateCondition('record.rating >= 4', low)).toBe(false);
+    });
+
+    it('evaluates a bare truthy gate instead of answering false', () => {
+        // No comparison operator at all: the template path fell through to
+        // `Number('record.isActive')` → NaN → `false`.
+        expect(engine.evaluateCondition('record.isActive', new Map<string, unknown>([['record', { isActive: true }]]))).toBe(true);
+        expect(engine.evaluateCondition('record.isActive', new Map<string, unknown>([['record', { isActive: false }]]))).toBe(false);
+    });
+
+    it('resolves field access on an object variable — the get_record output shape', () => {
+        // `get_record`'s `outputVariable` stores the WHOLE record under one name,
+        // which is why the `{lead_record.status}` spelling can never resolve.
+        const vars = new Map<string, unknown>([['lead_record', { status: 'converted' }]]);
+        expect(engine.evaluateCondition("lead_record.status == 'converted'", vars)).toBe(true);
+        expect(engine.evaluateCondition("lead_record.status == 'new'", vars)).toBe(false);
+    });
+
+    it('refuses a brace-wrapped reference that resolves to nothing, naming it', () => {
+        const vars = new Map<string, unknown>([['lead_record', { status: 'converted' }]]);
+        // Silently false today even though the status IS 'converted'.
+        expect(() => engine.evaluateCondition("{lead_record.status} == 'converted'", vars))
+            .toThrow(/`\{lead_record\.status\}` did not resolve/);
+        expect(() => engine.evaluateCondition("{lead_record.status} == 'converted'", vars))
+            .toThrow(/Drop the braces/);
+        // Same for a brace-wrapped truthy gate.
+        expect(() => engine.evaluateCondition('{record.isActive}', new Map<string, unknown>([['record', { isActive: true }]])))
+            .toThrow(/did not resolve/);
+    });
+
+    it('refuses a template-dialect condition it cannot turn into a predicate', () => {
+        // `{status}` substitutes to `open` — not a boolean, not a number, and no
+        // operator to compare it with. That used to be `false`.
+        expect(() => engine.evaluateCondition('{status}', new Map([['status', 'open']])))
+            .toThrow(/is not a predicate/);
+        // A boolean or numeric value still reads as a gate.
+        expect(engine.evaluateCondition('{flag}', new Map([['flag', true]]))).toBe(true);
+        expect(engine.evaluateCondition('{flag}', new Map([['flag', false]]))).toBe(false);
+        expect(engine.evaluateCondition('{count}', new Map([['count', 3]]))).toBe(true);
+        expect(engine.evaluateCondition('{count}', new Map([['count', 0]]))).toBe(false);
+    });
+
+    it('does not mistake braces inside a string literal for a template hole', () => {
+        // `'{pending}'` is text the predicate compares AGAINST, not a reference to
+        // substitute. The dialect sniff reads the source outside string literals,
+        // so this stays CEL and compares the field.
+        expect(engine.evaluateCondition("record.label == '{pending}'",
+            new Map<string, unknown>([['record', { label: '{pending}' }]]))).toBe(true);
+        expect(engine.evaluateCondition("record.label == '{pending}'",
+            new Map<string, unknown>([['record', { label: 'pending' }]]))).toBe(false);
+    });
+
+    it('keeps braces inside an explicit CEL envelope a hard error', () => {
+        // The dialect sniff applies only where no dialect was stated. `dialect:
+        // 'cel'` is the author saying "this is CEL", where `{…}` is a map literal
+        // and the #1491 brace-trap.
+        expect(() => engine.evaluateCondition({ dialect: 'cel', source: '{record.rating} >= 4' },
+            new Map<string, unknown>([['record', { rating: 5 }]]))).toThrow(/source:/);
+    });
+
+    it('treats an absent or empty condition as no branch, not as a fault', () => {
+        // A `decision` entry with no `expression` is the one caller that does not
+        // pre-check; an unauthored branch must not open, and must not throw either.
+        expect(engine.evaluateCondition('', new Map())).toBe(false);
+        expect(engine.evaluateCondition('   ', new Map())).toBe(false);
+        expect(engine.evaluateCondition(undefined as unknown as string, new Map())).toBe(false);
     });
 });