diff --git a/.changeset/flow-condition-bare-string-is-cel.md b/.changeset/flow-condition-bare-string-is-cel.md new file mode 100644 index 0000000000..392cd46119 --- /dev/null +++ b/.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`. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index 7d344b7f5a..8e19cb8868 100644 --- a/content/docs/automation/flows.mdx +++ b/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) | -**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. + -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. + +**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). CEL conditions that fail to evaluate raise an error and stop the run — they diff --git a/packages/services/service-automation/src/engine.test.ts b/packages/services/service-automation/src/engine.test.ts index f3c5cf9615..d560ca1b8a 100644 --- a/packages/services/service-automation/src/engine.test.ts +++ b/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(); - // 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([['record', { rating: 2 }]]); + const high = new Map([['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([['record', { isActive: true }]]))).toBe(true); + expect(engine.evaluateCondition('record.isActive', new Map([['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([['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([['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([['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([['record', { label: '{pending}' }]]))).toBe(true); + expect(engine.evaluateCondition("record.label == '{pending}'", + new Map([['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([['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); }); }); diff --git a/packages/services/service-automation/src/engine.ts b/packages/services/service-automation/src/engine.ts index 2855b574fb..37635038e8 100644 --- a/packages/services/service-automation/src/engine.ts +++ b/packages/services/service-automation/src/engine.ts @@ -41,6 +41,52 @@ import { ExpressionEngine, validateExpression, nearestName } from '@objectstack/ */ const UNRESOLVED_CEL_REFERENCE = /^[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)+$/; +/** + * A legacy single-brace **template hole** — `{amount}`, `{get_lead.id}`. This is + * the exact shape `{var}` substitution can consume (it splits on the literal + * `{}` text for each variable key), which is what makes it a sound dialect + * discriminator in {@link AutomationEngine.evaluateCondition}: a condition + * containing one was written in the template dialect, and a condition containing + * none was written as CEL (#4336). + * + * No whitespace is tolerated inside the braces, deliberately — `{ amount }` + * is not a token substitution can resolve, so treating it as a hole would only + * move the failure. It is not valid CEL either (a map literal needs `key: value` + * pairs), so it lands on the CEL path and gets the brace-trap diagnostic. + */ +const TEMPLATE_HOLE = /\{[A-Za-z_$][A-Za-z0-9_$]*(?:\.[A-Za-z_$][A-Za-z0-9_$]*)*\}/g; + +/** A quoted string literal, anywhere in a condition. */ +const QUOTED_SEGMENT = /'[^']*'|"[^"]*"/g; + +/** A single-quoted or double-quoted string literal, whole-operand. */ +const QUOTED_LITERAL = /^'([^']*)'$|^"([^"]*)"$/; + +/** + * Every legacy `{var}` hole in `source`, ignoring any that sits **inside a + * string literal** — `record.name == '{unresolved}'` is a CEL predicate + * comparing against text that happens to contain braces, not a template. + * + * This is both the dialect discriminator and the unresolved-hole report, so the + * two can never disagree about what counts as a hole. It reads the AUTHORED + * source rather than the substituted result, so a variable whose *value* + * contains braces cannot masquerade as an unresolved reference. + */ +function templateHoles(source: string): string[] { + return source.replace(QUOTED_SEGMENT, '').match(TEMPLATE_HOLE) ?? []; +} + +/** + * Strip one layer of matching quotes from a template-dialect operand, so a + * quoted string literal compares as its contents (#4336). Anything that is not + * a whole quoted literal is returned untouched — including a value that merely + * contains a quote. + */ +function unquoteLiteral(operand: string): string { + const m = QUOTED_LITERAL.exec(operand); + return m ? (m[1] ?? m[2] ?? '') : operand; +} + /** * The branch a `decision` node reports when it DECLARED `config.conditions` and * none of them matched — "fall through to the declared fallback". @@ -3864,15 +3910,42 @@ export class AutomationEngine implements IAutomationService { } /** - * Safe expression evaluator. - * Uses simple operator-based parsing without `new Function`. - * Supports: comparisons (>, <, >=, <=, ==, !=, ===, !==), - * boolean literals (true, false), and basic arithmetic. + * Evaluate a flow condition to a boolean. + * + * ## Which dialect a condition is in + * + * A condition is **CEL** unless it is written in the legacy single-brace + * `{var}` template dialect — and that is decided by looking at the *source*, + * not at whether an envelope happens to be present (#4336). + * + * It used to be decided by the envelope, and that was the bug: only an + * `{ dialect, source }` envelope reached the CEL engine, so a condition + * authored as a plain string — the shape `ExpressionInput` accepts by design, + * and the shape every node `config` still holds, since `FlowNodeSchema.config` + * is an open `z.record` no transform can reach — fell through to the template + * path and was compared **as text**. The failure direction depended on the + * predicate, which is what made it dangerous: + * + * 'existingTask == null' → 'existingTask' === 'null' → always FALSE + * 'record.rating >= 4' → 'record.rating' >= '4' → always TRUE + * + * — one gate that never opens, one branch pinned open, both reporting + * `success`. Reading the source instead means the same predicate evaluates + * the same way wherever it is authored: an edge (parsed into an envelope by + * `FlowEdgeSchema`), a start-node gate, or a `decision` node's + * `config.conditions[].expression`, which no schema normalizes. + * + * The `{var}` dialect stays supported for the flows that use it — but it no + * longer answers `false` when it could not resolve something. Per ADR-0032 + * §1c a predicate that cannot be evaluated is a **fault**, never a quiet + * branch decision, so an unresolved hole is refused with the source attached. + * + * Braces inside a **CEL envelope** remain the #1491 brace-trap and still + * throw: an explicit `dialect: 'cel'` is the author saying "this is CEL", and + * `{…}` is a map literal there. The sniff only applies where the dialect was + * never stated. */ evaluateCondition(expression: string | { dialect?: string; source?: string; ast?: unknown }, variables: Map): boolean { - // M9.5+ wiring: route Expression envelopes through @objectstack/formula - // ExpressionEngine. CEL is the default; legacy `{var}` template syntax - // is preserved as a fallback for back-compat. const isEnvelope = typeof expression === 'object' && expression != null && 'dialect' in expression; const dialect = isEnvelope ? (expression as { dialect?: string }).dialect : undefined; const exprStr = typeof expression === 'string' ? expression : ((expression as { source?: string })?.source ?? ''); @@ -3882,9 +3955,23 @@ export class AutomationEngine implements IAutomationService { return false; } + // An absent / empty condition is not a predicate to evaluate. Callers that + // mean "unconditional" guard before calling; this is the one that does not + // (a `decision` node whose `conditions[]` entry has no `expression`), and + // an unauthored branch must not open. + if (exprStr.trim() === '') return false; + + // The dialect decision (see the doc comment). An explicit `template`/`flow` + // envelope takes the author at their word; a bare string is sniffed for a + // `{var}` hole; everything else — including an envelope with no dialect — + // is CEL. + const holes = templateHoles(exprStr); + const declaredTemplate = isEnvelope && (dialect === 'template' || dialect === 'flow'); + const useTemplateDialect = declaredTemplate || (!isEnvelope && holes.length > 0); + // CEL path — bind `vars` scope for `{step.result}` style references via // the equivalent `vars.step.result` CEL identifier path. - if (dialect === 'cel' || (isEnvelope && !dialect)) { + if (!useTemplateDialect) { try { const vars: Record = {}; for (const [key, value] of variables) { @@ -3940,11 +4027,15 @@ export class AutomationEngine implements IAutomationService { // No `try { … } catch { return false }` around this block (#4347). Nothing // in it throws — `indexOf` / `slice` / `Number` / `compareValues` are all - // total — so the catch guarded nothing, and the one thing that CAN throw - // here now is the deliberate refusal below, which a swallow-to-`false` - // would turn straight back into the silent wrong answer it exists to + // total — so the catch guarded nothing, and the things that CAN throw + // here now are the deliberate refusals below, which a swallow-to-`false` + // would turn straight back into the silent wrong answer they exist to // prevent (ADR-0032 §1c, same rule as the CEL path above). + // A hole naming nothing in the variable map is unresolvable (#4336). + // Refuse — see the helper for why `false` was the wrong answer. + this.refuseUnresolvedTemplateHole(exprStr, holes.filter(h => !variables.has(h.slice(1, -1)))); + // Boolean literals if (resolved === 'true') return true; if (resolved === 'false') return false; @@ -3965,7 +4056,57 @@ export class AutomationEngine implements IAutomationService { const numVal = Number(resolved); if (!isNaN(numVal)) return numVal !== 0; - return false; + // No operator, not a boolean, not a number — this path has no way to + // decide the branch, and `false` used to be its answer (#4336). That made + // a truthy gate on a non-boolean variable — `'{record.status}'`, where + // the value is `'open'` — read as "condition not met" forever, with the + // run still recorded as `success`. Refuse instead, same rule as above. + throw new Error( + `condition evaluation error: \`${resolved}\` is not a predicate — source: \`${exprStr}\`. ` + + `The legacy \`{var}\` template dialect decides a branch by comparing the substituted ` + + `text, so it needs a comparison (\`{status} == 'open'\`) or a value that reads as a ` + + `boolean or number; a bare non-boolean value gives it nothing to compare and used to ` + + `answer \`false\` regardless of the value. Write the predicate as CEL — a condition ` + + `without \`{…}\` braces is evaluated by the CEL engine, where \`record.isActive\` is a ` + + `truthy gate and \`record.status == 'open'\` resolves the field.`, + ); + } + + /** + * Refuse a legacy-dialect condition whose `{…}` holes name no variable + * (#4336). + * + * Substitution replaces the literal text `{}` for each key in the + * variable map, so an unmatched hole survives into the comparison — and the + * template path would then compare the *brace text itself*: + * + * '{lead_record.status} == \'converted\'' + * → '{lead_record.status}' === "'converted'" → always FALSE + * + * The gate never opens, for any record, and the run still reports `success`. + * + * The common way to land here is a **field access on an object variable**: + * `get_record`'s `outputVariable` stores the whole record under one name + * (`lead_record`), so `{lead_record.status}` asks for a key that was never + * written. Note the asymmetry that let this survive — a node's outputs ARE + * flattened into dotted keys (`${node.id}.${key}`), so `{get_lead.id}` + * resolves and looks like proof the spelling works. + * + * CEL resolves that access properly, which is why the prescription is to drop + * the braces rather than to spell the hole differently. + */ + private refuseUnresolvedTemplateHole(source: string, unresolved: readonly string[]): void { + if (unresolved.length === 0) return; + const names = [...new Set(unresolved)]; + throw new Error( + `condition evaluation error: ${names.map(h => `\`${h}\``).join(', ')} did not resolve — ` + + `source: \`${source}\`. The legacy \`{var}\` template dialect substitutes a WHOLE flow ` + + `variable by name, and no variable is named ${names.map(h => `\`${h.slice(1, -1)}\``).join(', ')}. ` + + `Leaving it in place would compare the brace text as a STRING — a branch that is silently ` + + `wrong rather than merely unevaluated — so this is refused. Drop the braces: a condition ` + + `without them is evaluated as CEL, which resolves field access on an object variable ` + + `(\`lead_record.status == 'converted'\`) instead of looking for a variable spelled that way.`, + ); } /** @@ -3987,6 +4128,12 @@ export class AutomationEngine implements IAutomationService { * predicate that cannot be evaluated is a fault, never a quiet `false` (or, * here, a quiet `true`). * + * Since #4336 a *wholly* brace-free condition no longer arrives here at all — + * it is CEL, and `oppRecord.amount > 500000` simply evaluates. What still + * reaches this guard is a dotted reference **mixed into** a template-dialect + * condition (`'{limit} > record.amount'`), where the author is one operand + * away from the right dialect and the string compare would answer anyway. + * * Only *dotted* references are refused. A bare word compares as a string on * purpose — `'{status} == active'` is the documented legacy spelling, and * after substitution both sides are plain words. @@ -4008,8 +4155,19 @@ export class AutomationEngine implements IAutomationService { /** * Compare two string-represented values with an operator. + * + * Quoted operands are unquoted first (#4336). The template dialect compares + * text, so `'{status} == \'active\''` used to substitute to `active == + * 'active'` and compare `active` against `'active'` **with the quotes** — + * never equal, for any value of `status`. That spelling is not exotic: it is + * what the flow docs show for a decision node, and quoting a string literal + * is what every other predicate surface on the platform requires. So the + * quotes are stripped and both the quoted and the bare form (`{status} == + * active`, the older documented spelling) compare the same way. */ private compareValues(left: string, op: string, right: string): boolean { + left = unquoteLiteral(left); + right = unquoteLiteral(right); const lNum = Number(left); const rNum = Number(right); const bothNumeric = !isNaN(lNum) && !isNaN(rNum) && left !== '' && right !== ''; diff --git a/packages/services/service-automation/src/nested-region-parity.test.ts b/packages/services/service-automation/src/nested-region-parity.test.ts index 0b0976e71b..73d13c7d88 100644 --- a/packages/services/service-automation/src/nested-region-parity.test.ts +++ b/packages/services/service-automation/src/nested-region-parity.test.ts @@ -350,17 +350,25 @@ describe("#4347 — the legacy `{var}` path refuses an unresolved reference", () let engine: AutomationEngine; beforeEach(() => { engine = new AutomationEngine(silentLogger()); }); - it('refuses a dotted reference instead of comparing it lexicographically', () => { + it('evaluates a wholly brace-free dotted predicate as CEL', () => { const vars = new Map([['oppRecord', { amount: 10 }]]); // The reported footgun: 'oppRecord.amount' > '500000' compares 'o' to '5', - // so this used to be TRUE for every record regardless of the amount. - expect(() => engine.evaluateCondition('oppRecord.amount > 500000', vars)).toThrow(/unresolved expression reference/); - expect(() => engine.evaluateCondition('oppRecord.amount > 500000', vars)).toThrow(/dialect: 'cel'/); + // so this used to be TRUE for every record regardless of the amount. #4347 + // stopped the wrong answer by refusing it; #4336 goes on to give the RIGHT + // one — a condition with no `{…}` hole is CEL, so the amount is compared. + expect(engine.evaluateCondition('oppRecord.amount > 500000', vars)).toBe(false); + expect(engine.evaluateCondition('oppRecord.amount > 5', vars)).toBe(true); }); - it('refuses it on either side of the operator', () => { - const vars = new Map(); - expect(() => engine.evaluateCondition('5 == row.shouldRun', vars)).toThrow(/unresolved expression reference/); + it('still refuses a dotted reference mixed INTO a template-dialect condition', () => { + // A `{…}` hole puts the whole condition in the template dialect, where a + // dotted operand is text and the compare would answer anyway. One operand + // away from the right dialect, so the refusal names the fix. + const vars = new Map([['limit', 5]]); + expect(() => engine.evaluateCondition('{limit} > record.amount', vars)).toThrow(/unresolved expression reference/); + expect(() => engine.evaluateCondition('{limit} > record.amount', vars)).toThrow(/dialect: 'cel'/); + // Either side of the operator. + expect(() => engine.evaluateCondition('{limit} == row.shouldRun', vars)).toThrow(/unresolved expression reference/); }); it('evaluates the same predicate correctly once it is a CEL envelope', () => { diff --git a/packages/spec/src/automation/flow.zod.ts b/packages/spec/src/automation/flow.zod.ts index f48cff461f..b157d762b0 100644 --- a/packages/spec/src/automation/flow.zod.ts +++ b/packages/spec/src/automation/flow.zod.ts @@ -114,7 +114,8 @@ export const FlowVariableSchema = lazySchema(() => z.object({ * label: "Is High Value?", * config: { * conditions: [ - * { label: "Yes", expression: "{amount} > 10000" }, + * // Bare CEL, like every other condition — no `{…}` braces (#4336). + * { label: "Yes", expression: "amount > 10000" }, * { label: "No", expression: "true" } // default * ] * },