Skip to content

Commit 68c02c2

Browse files
os-zhuangclaude
andauthored
fix(automation): evaluateCondition decides the dialect from the source, not from the caller (#4453)
`AutomationEngine.evaluateCondition` picked its engine by asking whether an `{ dialect, source }` envelope was present, so a condition handed to it as a plain string never reached CEL — it fell to the legacy `{var}` template path and both sides were compared as text. `existingTask == null` became `'existingTask' === 'null'` (always false); `record.rating >= 4` became `'r' > '4'` (always true). Both reported success. #4414/#4440 fixed the one built-in reaching this by wrapping at the call site. This fixes the evaluator: the dialect is read from the source, and a condition is CEL unless it actually contains a `{var}` hole. `evaluateCondition` is public API, so a plugin-registered executor was still getting the old behaviour. The `{var}` dialect keeps working and gains what it was missing: a quoted literal compares as its contents (`{status} == 'active'` was false for every value), and its two silent-`false` exits — an unresolvable `{…}` hole, and a substituted value that is neither boolean, numeric, nor part of a comparison — are refused with the source attached (ADR-0032 §1c). Braces inside an explicit `dialect: 'cel'` envelope remain the #1491 brace-trap. The sniff skips string literals, so `record.label == '{pending}'` stays CEL. Closes #4336. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 84b4a3a commit 68c02c2

6 files changed

Lines changed: 377 additions & 42 deletions

File tree

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

content/docs/automation/flows.mdx

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ const approvalFlow = {
3131
type: 'decision',
3232
label: 'Check Amount',
3333
config: {
34-
// decision expressions use {braces} around variables — see Expressions in flows
34+
// decision expressions are bare CEL, like every other condition — no braces
3535
conditions: [
36-
{ label: 'High Value', expression: '{order_amount} > 10000' },
37-
{ label: 'Standard', expression: '{order_amount} <= 10000' },
36+
{ label: 'High Value', expression: 'order_amount > 10000' },
37+
{ label: 'Standard', expression: 'order_amount <= 10000' },
3838
],
3939
},
4040
},
@@ -169,8 +169,8 @@ or missing-`required` violation (#4277). A node type that publishes no
169169
label: 'Check Status',
170170
config: {
171171
conditions: [
172-
{ label: 'Approved', expression: "{status} == 'approved'" },
173-
{ label: 'Rejected', expression: "{status} == 'rejected'" },
172+
{ label: 'Approved', expression: "status == 'approved'" },
173+
{ label: 'Rejected', expression: "status == 'rejected'" },
174174
],
175175
},
176176
}
@@ -972,30 +972,40 @@ failures so one broken flow does not abort startup.
972972

973973
## Expressions in flows
974974

975-
A flow mixes **three expression dialects**, and using the wrong one is the
976-
single most common way a flow silently misbehaves. Which dialect applies is
977-
decided by *where* the expression sits — not by what it looks like:
975+
A flow mixes **two expression dialects**, and the rule is short: **every
976+
condition is CEL; braces are for values.**
978977

979978
| Where | Dialect | Write it like | Bindings |
980979
|:---|:---|:---|:---|
981980
| Start-node `condition` | **CEL** (bare, no braces) | `record.amount > 500` | `record.*`, `previous.*`, bare field names, `vars.*` |
982981
| Edge `condition` | **CEL** (bare, no braces) | `record.status == 'open'` | same as above |
983-
| Decision-node `conditions[].expression` | **Template compare** (braces required) | `{order_amount} > 10000` | flow variables by name, in `{…}` |
982+
| Decision-node `conditions[].expression` | **CEL** (bare, no braces) | `order_amount > 10000` | same as above |
984983
| 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) |
985984

986985
<Callout type="warn">
987-
**The two failure modes to memorize:**
986+
**The failure modes to memorize:**
988987

989-
1. **Braces missing in a decision expression**`'order_amount > 10000'` isn't
990-
evaluated as a variable at all. It compares the *string* `"order_amount"`
991-
against `"10000"`, which is **always true**, so the flow always takes the
992-
first branch and never tells you. Write `'{order_amount} > 10000'`.
993-
2. **Braces missing in a field value**`due_date: 'TODAY() + 7'` writes the
988+
1. **Braces missing in a field value**`due_date: 'TODAY() + 7'` writes the
994989
literal text `TODAY() + 7` into the field. Write `'{TODAY() + 7}'`.
990+
2. **Braces put *into* a condition**`'{record.amount} > 500'`. Conditions
991+
fail loudly rather than silently, with an error that tells you to drop the
992+
braces.
993+
</Callout>
995994

996-
The mirror mistake is putting braces *into* a CEL condition
997-
(`'{record.amount} > 500'`) — CEL conditions fail loudly rather than silently,
998-
with an error that tells you to drop the braces.
995+
<Callout type="info">
996+
**Decision-node expressions used to be compared as text** (#4414, #4336), so
997+
`'order_amount > 10000'` compared the string `"order_amount"` against `"10000"`
998+
and was **always true**, while `'{lead_record.status} == "converted"'` was
999+
**always false** — the brace form substitutes a whole flow variable by name, and
1000+
a field access on an object variable is not one. Both reported `success`. They
1001+
are bare CEL now, so the spellings in the table above are the correct ones and
1002+
`lead_record.status == 'converted'` resolves the field.
1003+
1004+
The `{var}` form still works where it always did — `{amount} > 100`,
1005+
`{status} == 'active'` — but the two ways it used to answer `false` without
1006+
saying so are now **loud errors** naming the reference: a `{…}` hole that
1007+
matches no flow variable, and a substituted value that is neither a boolean, a
1008+
number, nor part of a comparison (#4336).
9991009
</Callout>
10001010

10011011
CEL conditions that fail to evaluate raise an error and stop the run — they

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

Lines changed: 109 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1905,10 +1905,15 @@ describe('AutomationEngine - Safe Expression Evaluation', () => {
19051905

19061906
it('should not execute malicious code', () => {
19071907
const vars = new Map<string, unknown>();
1908-
// These should all return false safely
1909-
expect(engine.evaluateCondition('process.exit(1)', vars)).toBe(false);
1910-
expect(engine.evaluateCondition('require("fs").readFileSync("/etc/passwd")', vars)).toBe(false);
1911-
expect(engine.evaluateCondition('(() => { while(true) {} })()', vars)).toBe(false);
1908+
// None of these is a host-language program to this engine — there is no
1909+
// `new Function`, no `eval`, no `require` on either path. They REFUSE
1910+
// rather than return `false` since #4336: a brace-free condition is CEL,
1911+
// and CEL has no `process`, no `require`, and no arrow-function syntax,
1912+
// so each one is a fault the run reports. The safety property is
1913+
// unchanged (nothing executes) and the diagnosis is no longer silent.
1914+
expect(() => engine.evaluateCondition('process.exit(1)', vars)).toThrow(/exit/);
1915+
expect(() => engine.evaluateCondition('require("fs").readFileSync("/etc/passwd")', vars)).toThrow(/require/);
1916+
expect(() => engine.evaluateCondition('(() => { while(true) {} })()', vars)).toThrow(/source:/);
19121917
});
19131918

19141919
it('should handle string comparisons', () => {
@@ -1917,6 +1922,106 @@ describe('AutomationEngine - Safe Expression Evaluation', () => {
19171922

19181923
expect(engine.evaluateCondition('{status} == active', vars)).toBe(true);
19191924
expect(engine.evaluateCondition('{status} != inactive', vars)).toBe(true);
1925+
// #4336 — a QUOTED literal on the right compares as its contents. This is
1926+
// the spelling the flow docs show for a decision node, and it used to
1927+
// compare `active` against `'active'` (quotes included) and be false for
1928+
// every value of `status`.
1929+
expect(engine.evaluateCondition("{status} == 'active'", vars)).toBe(true);
1930+
expect(engine.evaluateCondition('{status} == "active"', vars)).toBe(true);
1931+
expect(engine.evaluateCondition("{status} == 'closed'", vars)).toBe(false);
1932+
expect(engine.evaluateCondition("{status} != 'closed'", vars)).toBe(true);
1933+
});
1934+
});
1935+
1936+
// ─── #4336: a bare-string condition is CEL, not a string compare ─────
1937+
//
1938+
// The reported defect: `evaluateCondition` branched on whether an `Expression`
1939+
// envelope was present, so a condition authored as a plain string never reached
1940+
// the CEL engine and both sides were compared as TEXT. Every case below is one
1941+
// of the two failure directions from the issue (a gate that never opens, a
1942+
// branch pinned open) or one of the two silent `false`s found afterwards.
1943+
describe('AutomationEngine - bare-string conditions evaluate as CEL (#4336)', () => {
1944+
let engine: AutomationEngine;
1945+
beforeEach(() => { engine = new AutomationEngine(createTestLogger()); });
1946+
1947+
it('opens a null-check gate that used to be pinned shut', () => {
1948+
// `'existingTask' === 'null'` → false, forever. The flow selected its
1949+
// records, took no branch, and recorded `success`.
1950+
expect(engine.evaluateCondition('existingTask == null', new Map([['existingTask', null]]))).toBe(true);
1951+
expect(engine.evaluateCondition('existingTask == null', new Map([['existingTask', { id: 'a' }]]))).toBe(false);
1952+
});
1953+
1954+
it('gates a numeric comparison that used to be pinned open', () => {
1955+
// `'record.rating' >= '4'` → `'r' > '4'` → true for every record.
1956+
const low = new Map<string, unknown>([['record', { rating: 2 }]]);
1957+
const high = new Map<string, unknown>([['record', { rating: 5 }]]);
1958+
expect(engine.evaluateCondition('record.rating >= 4', high)).toBe(true);
1959+
expect(engine.evaluateCondition('record.rating >= 4', low)).toBe(false);
1960+
});
1961+
1962+
it('evaluates a bare truthy gate instead of answering false', () => {
1963+
// No comparison operator at all: the template path fell through to
1964+
// `Number('record.isActive')` → NaN → `false`.
1965+
expect(engine.evaluateCondition('record.isActive', new Map<string, unknown>([['record', { isActive: true }]]))).toBe(true);
1966+
expect(engine.evaluateCondition('record.isActive', new Map<string, unknown>([['record', { isActive: false }]]))).toBe(false);
1967+
});
1968+
1969+
it('resolves field access on an object variable — the get_record output shape', () => {
1970+
// `get_record`'s `outputVariable` stores the WHOLE record under one name,
1971+
// which is why the `{lead_record.status}` spelling can never resolve.
1972+
const vars = new Map<string, unknown>([['lead_record', { status: 'converted' }]]);
1973+
expect(engine.evaluateCondition("lead_record.status == 'converted'", vars)).toBe(true);
1974+
expect(engine.evaluateCondition("lead_record.status == 'new'", vars)).toBe(false);
1975+
});
1976+
1977+
it('refuses a brace-wrapped reference that resolves to nothing, naming it', () => {
1978+
const vars = new Map<string, unknown>([['lead_record', { status: 'converted' }]]);
1979+
// Silently false today even though the status IS 'converted'.
1980+
expect(() => engine.evaluateCondition("{lead_record.status} == 'converted'", vars))
1981+
.toThrow(/`\{lead_record\.status\}` did not resolve/);
1982+
expect(() => engine.evaluateCondition("{lead_record.status} == 'converted'", vars))
1983+
.toThrow(/Drop the braces/);
1984+
// Same for a brace-wrapped truthy gate.
1985+
expect(() => engine.evaluateCondition('{record.isActive}', new Map<string, unknown>([['record', { isActive: true }]])))
1986+
.toThrow(/did not resolve/);
1987+
});
1988+
1989+
it('refuses a template-dialect condition it cannot turn into a predicate', () => {
1990+
// `{status}` substitutes to `open` — not a boolean, not a number, and no
1991+
// operator to compare it with. That used to be `false`.
1992+
expect(() => engine.evaluateCondition('{status}', new Map([['status', 'open']])))
1993+
.toThrow(/is not a predicate/);
1994+
// A boolean or numeric value still reads as a gate.
1995+
expect(engine.evaluateCondition('{flag}', new Map([['flag', true]]))).toBe(true);
1996+
expect(engine.evaluateCondition('{flag}', new Map([['flag', false]]))).toBe(false);
1997+
expect(engine.evaluateCondition('{count}', new Map([['count', 3]]))).toBe(true);
1998+
expect(engine.evaluateCondition('{count}', new Map([['count', 0]]))).toBe(false);
1999+
});
2000+
2001+
it('does not mistake braces inside a string literal for a template hole', () => {
2002+
// `'{pending}'` is text the predicate compares AGAINST, not a reference to
2003+
// substitute. The dialect sniff reads the source outside string literals,
2004+
// so this stays CEL and compares the field.
2005+
expect(engine.evaluateCondition("record.label == '{pending}'",
2006+
new Map<string, unknown>([['record', { label: '{pending}' }]]))).toBe(true);
2007+
expect(engine.evaluateCondition("record.label == '{pending}'",
2008+
new Map<string, unknown>([['record', { label: 'pending' }]]))).toBe(false);
2009+
});
2010+
2011+
it('keeps braces inside an explicit CEL envelope a hard error', () => {
2012+
// The dialect sniff applies only where no dialect was stated. `dialect:
2013+
// 'cel'` is the author saying "this is CEL", where `{…}` is a map literal
2014+
// and the #1491 brace-trap.
2015+
expect(() => engine.evaluateCondition({ dialect: 'cel', source: '{record.rating} >= 4' },
2016+
new Map<string, unknown>([['record', { rating: 5 }]]))).toThrow(/source:/);
2017+
});
2018+
2019+
it('treats an absent or empty condition as no branch, not as a fault', () => {
2020+
// A `decision` entry with no `expression` is the one caller that does not
2021+
// pre-check; an unauthored branch must not open, and must not throw either.
2022+
expect(engine.evaluateCondition('', new Map())).toBe(false);
2023+
expect(engine.evaluateCondition(' ', new Map())).toBe(false);
2024+
expect(engine.evaluateCondition(undefined as unknown as string, new Map())).toBe(false);
19202025
});
19212026
});
19222027

0 commit comments

Comments
 (0)