From 536c1d7645c1b3d130a177f3d660a3cfcfcac8d4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 16:13:33 +0000 Subject: [PATCH] fix(flows): make record-change conditions total so three automations stop silently not running (#633) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Strict CEL aborts a whole condition on a key that is not PRESENT, not merely one that is null. The record-change trigger builds the flow's `record` as `{...input.data, ...driverPostWriteRow}` and `previous` from the driver's prior row, so on driver-memory / driver-mongodb — which store only the columns a row was written with — an unwritten field arrives genuinely missing. Measured end-to-end (real ObjectQL + InMemoryDriver + RecordChangeTrigger), three conditions aborted on ordinary writes: case_escalation_on_create No such key: escalated_date (case born critical) contact_welcome No such key: owner (system/seed write) lead_assignment edge e2 No such key: rating (unrated lead) The abort makes AutomationEngine.execute record the run as failed and log at ERROR; the triggering write still succeeds, so the automation simply does not happen. It stayed hidden because CEL's `&&` absorbs an error beside a false operand: the conditions answered correctly for every record they were meant to skip and aborted only on the ones they were meant to act on. Every `record.x` / `previous.x` read now carries `has(...)`, and every ordering comparison also carries `!= null` (an explicit null passes has() and then aborts with `no such overload: dyn > int`). The rewrites are conservative: across the full cross-product of absent/null/valued shapes they return the same answer as the originals wherever the originals returned one. Two judgement calls are documented in-file. lead_assignment's two edges must PARTITION, so the standard branch absorbs an unreadable rating — guarding both with `has(...) &&` would have traded a loud abort for a silent no-op. opportunity_won_alert guards `previous.stage` fail-closed, because that term exists only to suppress a repeat blast to management. src/sharing/ is deliberately untouched: sharing conditions are COMPILED to pushdown filters by compileCelToFilter, which rejects the function-call class, so a has() guard there makes the rule untranslatable and plugin-sharing stops seeding it (#621 / #637). Verified the flow path never reaches that compiler — its only consumers are @objectstack/formula, plugin-sharing and plugin-security. Adds test/flow-condition-totality.test.ts: a structural sweep, a measured sweep through the real AutomationEngine.evaluateCondition, and end-to-end regressions for all three defects. It also pins that has() IS valid here, as the explicit counterpart to sharing-seeding.test.ts's `sharing conditions cannot use has()`. docs-drift.test.ts's manager-threshold pattern keyed on the clause next to the value, which the guards displaced; it now keys on the `record.` scope that distinguishes it from the director tier's `oppRecord.amount`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019SS7C5SXpniKeCApxgARyf --- .changeset/flow-condition-totality.md | 55 ++ src/flows/case-csat-followup.flow.ts | 13 +- src/flows/case-escalation.flow.ts | 18 +- src/flows/contact-welcome.flow.ts | 14 +- src/flows/lead-assignment.flow.ts | 23 +- src/flows/opportunity-approval.flow.ts | 14 +- src/flows/opportunity-won-alert.flow.ts | 21 +- src/flows/task-urgent-alert.flow.ts | 9 +- test/docs-drift.test.ts | 10 +- test/flow-condition-totality.test.ts | 624 ++++++++++++++++++++++ test/object-validation-predicates.test.ts | 17 + 11 files changed, 806 insertions(+), 12 deletions(-) create mode 100644 .changeset/flow-condition-totality.md create mode 100644 test/flow-condition-totality.test.ts diff --git a/.changeset/flow-condition-totality.md b/.changeset/flow-condition-totality.md new file mode 100644 index 00000000..fe737772 --- /dev/null +++ b/.changeset/flow-condition-totality.md @@ -0,0 +1,55 @@ +--- +'hotcrm': patch +--- + +Make record-change flow conditions TOTAL, so three automations stop silently +failing to run on Mongo- and memory-backed installs. + +A flow's start/edge condition is bare CEL, and strict CEL aborts the whole +expression on a key that is not present — not just on a null one. The +record-change trigger builds the flow's `record` as the mutation payload with +the **driver's post-write row** overlaid, and `previous` from the driver's prior +row, so on a datasource that stores only the columns a row was written with +(`driver-memory`, `driver-mongodb`; the SQL family is column-complete and +unaffected) an unwritten field arrives genuinely MISSING. Three conditions read +such a field with no guard, and were measured aborting end-to-end: + +- **`case_escalation_on_create`** — a case created critical is stored with no + `escalated_date` column, so `record.escalated_date == null` aborted and a + phone-in P1 was never escalated. That is the flow's core population. +- **`contact_welcome`** — `owner`'s `os.user.id` default cannot evaluate on a + write that carries no user (seed data, integrations, any system context), so + the row has no `owner` column and `record.owner != null` aborted; no seeded + contact ever produced a welcome prompt. +- **`lead_assignment`** — `rating` is neither required nor defaulted, so + `record.rating >= 4` aborted and an unrated lead got no SLA stamp and no + alert at all. + +The failure was quiet because CEL's `&&` absorbs an error beside a `false` +operand: these conditions answered correctly for every record they were meant +to skip, and blew up only on the records they were meant to act on. The run is +recorded as failed and logged at ERROR, but the write itself succeeds and +nothing user-visible says the automation did not happen. + +Every `record.x` / `previous.x` read in a record-change flow condition now +carries a `has(...)` guard, and every ordering comparison additionally carries +`!= null` (an explicit null passes `has()` and then aborts with +`no such overload: dyn > int`). The rewrites are conservative — verified +across the full cross-product of absent/null/valued shapes, they return the +same answer as before wherever the original returned one at all. Two places +needed a judgement call and say so in-file: `lead_assignment`'s two branches +must PARTITION, so the standard branch absorbs an unreadable rating rather than +both branches going false and dropping the lead silently; and +`opportunity_won_alert` guards `previous.stage` fail-closed, because that term +exists solely to stop a repeat congratulations blast to management. + +Adds `test/flow-condition-totality.test.ts`, which enforces the rule three +ways: a structural sweep for the guards, a measured sweep that runs every +condition through the real `AutomationEngine.evaluateCondition` across the +shapes a sparse driver produces, and end-to-end tests that boot a real ObjectQL +over `InMemoryDriver` with the real record-change trigger and reproduce each of +the three defects. It also pins the counter-fact to +`test/sharing-seeding.test.ts`: `has()` is correct here and is *rejected* on the +sharing surface, which compiles its conditions to pushdown filters instead of +interpreting them — so neither conclusion can be carried across. Refs #633, +#630. diff --git a/src/flows/case-csat-followup.flow.ts b/src/flows/case-csat-followup.flow.ts index 029b0ffd..096ddf4e 100644 --- a/src/flows/case-csat-followup.flow.ts +++ b/src/flows/case-csat-followup.flow.ts @@ -35,7 +35,18 @@ export const CaseCsatFollowupFlow: Flow = { config: { objectName: 'crm_case', triggerType: 'record-after-update', - condition: P`record.status == "closed" && (previous == null || previous.status != "closed")`, + // TOTALITY (#633): `has(...)` on every read, including the ones into + // `previous`. `previous` is the driver's PRIOR row, so it is sparse on + // driver-memory / driver-mongodb exactly like `record` is — measured, + // `previous == null || previous.status != "closed"` still aborts when + // `previous` is a row that simply has no `status` column. The + // `previous == null` term is kept because it is the author's declared + // intent for "no prior row: this is the close"; `!has(previous.status)` + // extends that same answer to a prior row the driver returned without + // the key. (`has()` on a null `previous` measures `false`, so the two + // terms agree rather than fight.) + condition: P`has(record.status) && record.status == "closed" + && (previous == null || !has(previous.status) || previous.status != "closed")`, }, }, { diff --git a/src/flows/case-escalation.flow.ts b/src/flows/case-escalation.flow.ts index e5c691e6..451c2b04 100644 --- a/src/flows/case-escalation.flow.ts +++ b/src/flows/case-escalation.flow.ts @@ -43,8 +43,22 @@ export const CaseEscalationFlow: Flow = { // the boolean is_closed suffers the SQLite `1 != true` trap above. // `escalated_date == null` additionally keeps a case that was already // escalated once (then reopened) from being escalated again. - condition: P`record.priority == "critical" && record.escalated_date == null - && record.status != "escalated" && record.status != "resolved" && record.status != "closed"`, + // + // TOTALITY (#633): every `record.x` read carries a `has(record.x)` + // guard — see the house-rule block in + // `test/flow-condition-totality.test.ts`. Measured end-to-end on + // driver-memory: a case BORN critical is stored without an + // `escalated_date` column at all, `record.escalated_date == null` + // aborted with `No such key: escalated_date`, and this flow never ran + // on the very population it exists for. The guards below preserve the + // predicate's answer on every shape where it previously had one. + // `escalated_date` absent means the same as null ("never escalated"); + // an absent `status` cannot be a terminal status, so it must not + // suppress the escalation. + condition: P`has(record.priority) && record.priority == "critical" + && (!has(record.escalated_date) || record.escalated_date == null) + && (!has(record.status) + || (record.status != "escalated" && record.status != "resolved" && record.status != "closed"))`, }, }, { diff --git a/src/flows/contact-welcome.flow.ts b/src/flows/contact-welcome.flow.ts index decf9035..7acf276a 100644 --- a/src/flows/contact-welcome.flow.ts +++ b/src/flows/contact-welcome.flow.ts @@ -32,7 +32,19 @@ export const ContactWelcomeFlow: Flow = { config: { objectName: 'crm_contact', triggerType: 'record-after-create', - condition: P`record.owner != null && record.email_opt_out != true`, + // TOTALITY (#633): every `record.x` read carries a `has(record.x)` + // guard — see the house-rule block in + // `test/flow-condition-totality.test.ts`. Measured end-to-end on + // driver-memory: `owner`'s `os.user.id` default cannot evaluate on a + // write that carries no user (seed data, an integration, any system + // context), ObjectQL logs `Failed to evaluate default expression` and + // stores the row with NO `owner` column — so `record.owner != null` + // aborted with `No such key: owner` and no seeded contact ever got a + // welcome prompt. `!= null` is not a substitute for `has()`: on an + // absent key it aborts exactly like `== "v"` does. An absent + // `email_opt_out` means the contact has not opted out. + condition: P`has(record.owner) && record.owner != null + && (!has(record.email_opt_out) || record.email_opt_out != true)`, }, }, { diff --git a/src/flows/lead-assignment.flow.ts b/src/flows/lead-assignment.flow.ts index 484ea251..5625aeb8 100644 --- a/src/flows/lead-assignment.flow.ts +++ b/src/flows/lead-assignment.flow.ts @@ -40,8 +40,18 @@ export const LeadAssignmentFlow: Flow = { config: { objectName: 'crm_lead', triggerType: 'record-after-create' }, }, { + // TOTALITY (#633): `rating` is neither required nor defaulted, so a lead + // written without one is stored with NO `rating` column on + // driver-memory / driver-mongodb. Measured end-to-end: the unguarded + // `record.rating >= 4` on edge `e2` aborted with `No such key: rating` + // and an unrated lead got no SLA stamp and no alert at all. `has()` + // alone is not enough on an ORDERING comparison — an explicit + // `rating: null` passes `has()` and then aborts with + // `no such overload: dyn >= int` — so both guards are required, + // in this order. Kept in sync with edge `e2` below, which is what + // actually branches (see the note there). id: 'check_hot', type: 'decision', label: 'Hot Lead (rating ≥ 4)?', - config: { condition: P`record.rating >= 4` }, + config: { condition: P`has(record.rating) && record.rating != null && record.rating >= 4` }, }, // ── Hot path: 1-day SLA, high-severity alert ─────────────────── @@ -92,8 +102,15 @@ export const LeadAssignmentFlow: Flow = { edges: [ { id: 'e1', source: 'start', target: 'check_hot', type: 'default' }, - { id: 'e2', source: 'check_hot', target: 'sla_hot', type: 'conditional', condition: P`record.rating >= 4`, label: 'Hot' }, - { id: 'e3', source: 'check_hot', target: 'sla_std', type: 'conditional', condition: P`record.rating < 4`, label: 'Standard' }, + // TOTALITY (#633): these two edges must PARTITION every lead — a rating + // the predicate cannot read has to fall down one branch, never neither. + // Guarding both with `has(...) &&` would have traded a loud abort for a + // silent no-op (unrated lead → no SLA, no alert, no error), which is the + // "declared ≠ enforced" shape this repo keeps deleting rules over. So the + // hot branch demands a readable rating and the standard branch absorbs + // everything else: an unrated lead is, correctly, not a hot lead. + { id: 'e2', source: 'check_hot', target: 'sla_hot', type: 'conditional', condition: P`has(record.rating) && record.rating != null && record.rating >= 4`, label: 'Hot' }, + { id: 'e3', source: 'check_hot', target: 'sla_std', type: 'conditional', condition: P`!has(record.rating) || record.rating == null || record.rating < 4`, label: 'Standard' }, { id: 'e4', source: 'sla_hot', target: 'notify_hot', type: 'default' }, { id: 'e5', source: 'notify_hot', target: 'end', type: 'default' }, { id: 'e6', source: 'sla_std', target: 'notify_std', type: 'default' }, diff --git a/src/flows/opportunity-approval.flow.ts b/src/flows/opportunity-approval.flow.ts index 1f5b8a09..9ded4e16 100644 --- a/src/flows/opportunity-approval.flow.ts +++ b/src/flows/opportunity-approval.flow.ts @@ -58,8 +58,18 @@ export const OpportunityApprovalFlow: Flow = { // freeze hook rejects approval-status writes on closed records, so // without this guard the flow opened a locked approval request it // could never resolve (lockRecord held the closed record hostage). - condition: P`record.amount > 100000 && (record.approval_status == "not_required" || record.approval_status == null) - && record.stage != "closed_won" && record.stage != "closed_lost"`, + // TOTALITY (#633): `has(...)` on every read, plus `!= null` on the + // ordering comparison — `has()` passes an explicit null and + // `record.amount > 100000` then aborts with + // `no such overload: dyn > int`. Measured total as authored + // today (amount/stage are required, approval_status is defaulted), but + // that is `crm_opportunity`'s schema doing the work, not the + // predicate. An absent `approval_status` reads the same as the null + // this condition already tolerates; an absent `stage` is not a settled + // stage, so it must not block approval. + condition: P`has(record.amount) && record.amount != null && record.amount > 100000 + && (!has(record.approval_status) || record.approval_status == "not_required" || record.approval_status == null) + && (!has(record.stage) || (record.stage != "closed_won" && record.stage != "closed_lost"))`, }, }, { diff --git a/src/flows/opportunity-won-alert.flow.ts b/src/flows/opportunity-won-alert.flow.ts index 3bcb757f..4d702baa 100644 --- a/src/flows/opportunity-won-alert.flow.ts +++ b/src/flows/opportunity-won-alert.flow.ts @@ -33,7 +33,26 @@ export const OpportunityWonAlertFlow: Flow = { // claims, approval-status stamps, description tweaks) re-sent the // congratulations blast. The trigger forwards `previous` into the // condition scope (cf. the engine's record-change context). - condition: P`record.stage == "closed_won" && previous.stage != "closed_won" && record.amount > 100000`, + // TOTALITY (#633): `has(...)` on every read, plus `!= null` on the + // ordering comparison (`has()` passes an explicit null and + // `record.amount > 100000` then aborts with + // `no such overload: dyn > int`). + // + // `previous.stage` is guarded FAIL-CLOSED — `has(previous.stage) &&`, + // not `!has(previous.stage) ||` — which is the opposite of how + // `record.stage` is guarded, deliberately. This term exists solely to + // suppress a re-fire, and the failure it was written for (a + // congratulations blast to management re-sent on every later edit of a + // won deal) is worse than a missed one. When the engine cannot see + // what the prior stage was it cannot see a TRANSITION either, so it + // must not claim one. In practice `previous` is a full prior row on + // every record-after-update; the shape this covers is a bulk + // `updateMany`, where ObjectQL never reads a prior row and `previous` + // arrives null — and firing one blast per row of a bulk update is + // exactly what nobody wants. + condition: P`has(record.stage) && record.stage == "closed_won" + && has(previous.stage) && previous.stage != "closed_won" + && has(record.amount) && record.amount != null && record.amount > 100000`, }, }, { diff --git a/src/flows/task-urgent-alert.flow.ts b/src/flows/task-urgent-alert.flow.ts index f057ce60..985cd42b 100644 --- a/src/flows/task-urgent-alert.flow.ts +++ b/src/flows/task-urgent-alert.flow.ts @@ -28,7 +28,14 @@ export const TaskUrgentAlertFlow: Flow = { // SQLite/libsql booleans persist as integer 1, so `is_completed != true` // is `1 != true` = always true and the guard never trips (cf. the same // hazard documented in case_escalation). - condition: P`record.priority == "urgent" && record.status != "completed"`, + // TOTALITY (#633): `has(...)` on every read. Both fields are `required` + // AND defaulted today, so this predicate measured total as authored — + // but that is a property of `crm_task`'s schema, not of the predicate. + // Drop either default and the flow goes silently inert on + // driver-memory / driver-mongodb, with nothing to catch it. An absent + // `status` is not `completed`, so it must not suppress the alert. + condition: P`has(record.priority) && record.priority == "urgent" + && (!has(record.status) || record.status != "completed")`, }, }, { diff --git a/test/docs-drift.test.ts b/test/docs-drift.test.ts index 55fc52a4..9cad11e0 100644 --- a/test/docs-drift.test.ts +++ b/test/docs-drift.test.ts @@ -66,7 +66,15 @@ const cronDisplay = (raw: string) => { const RULES: Rule[] = [ { label: 'manager approval threshold', - extract: () => cap('opportunity-approval.flow.ts', /record\.amount > (\d+) && \(?record\.approval_status/), + // Anchored on the lowercase `record.` scope, which is what distinguishes + // the START condition's entry gate from the director tier's + // `oppRecord.amount > 500000` (the match is case-sensitive, so + // `oppRecord.amount` cannot satisfy `record\.amount`). It used to lean on + // the neighbouring `&& (record.approval_status` clause instead, which + // broke the moment #633 inserted the `has(...)` / `!= null` totality + // guards between the two — a drift detector should key on the value's own + // scope, not on whatever happens to sit next to it. + extract: () => cap('opportunity-approval.flow.ts', /record\.amount > (\d+)/), display: money, docs: ['crm_sales.md', 'crm_admin.md'], }, diff --git a/test/flow-condition-totality.test.ts b/test/flow-condition-totality.test.ts new file mode 100644 index 00000000..4036347d --- /dev/null +++ b/test/flow-condition-totality.test.ts @@ -0,0 +1,624 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { InMemoryDriver } from '@objectstack/driver-memory'; +import { AutomationEngine, installBuiltinNodes } from '@objectstack/service-automation'; +import { RecordChangeTrigger } from '@objectstack/trigger-record-change'; +import stack from '../objectstack.config'; + +/** + * ═══ HOUSE RULE: record-change flow conditions must be TOTAL ═══════════════ + * + * **Every `record.x` / `previous.x` read in a record-change flow's condition + * carries a `has(...)` guard**, and every ordering comparison additionally + * carries `!= null`. A condition must return a verdict for every record shape + * the trigger can hand it — never abort. This file enforces it; you do not + * have to remember it. + * + * This is the SAME house rule `test/object-validation-predicates.test.ts` + * states for object `validations[]`, applied to a **different surface with a + * different mechanism**. It was not inherited — #633 required all three + * questions be measured against this path specifically, because the repo's + * three CEL surfaces do not behave alike: + * + * | surface | mechanism | absent key ⇒ | + * | ------------------------ | ----------------------------- | ------------------- | + * | object `validations[]` | interpreted, per record | rule SKIPPED (WARN) | + * | `sharing` rule condition | COMPILED to a pushdown filter | never interpreted | + * | flow condition (here) | interpreted, per run | RUN FAILS (ERROR) | + * + * The sharing row is why `has()` must NOT be added to `src/sharing/`: + * `compileCelToFilter` rejects the whole function-call class, so a guarded + * sharing rule becomes untranslatable and `plugin-sharing` silently stops + * seeding it (#621 / PR #637; pinned in `test/sharing-seeding.test.ts` by + * `sharing conditions cannot use has()`). That conclusion stops at the sharing + * boundary. Flow conditions never reach a compiler — measured: the only + * `compileCelToFilter` consumers in the installed platform are + * `@objectstack/formula` (which defines it), `plugin-sharing` and + * `plugin-security`. Neither `service-automation` nor `trigger-record-change` + * calls it. + * + * ### The three questions #633 asked, and what was measured (17.0.0-rc.1) + * + * **1. What record shape does a record-change trigger hand the condition?** + * Not a re-read, and not `{...previous, ...data}`. `RecordChangeTrigger` + * builds `record` as `{ ...input.data, ...ctx.result }` — the mutation payload + * with the **driver's post-write row** overlaid. A re-read is merged UNDER it + * only when the object declares a `formula` field (read-time computed-field + * hydration), and it can only add keys, never fill them. `previous` is + * `ctx.previous`, the driver's PRIOR row — sparse in exactly the same way, and + * `null` on insert and on bulk `updateMany` (ObjectQL reads no prior row + * there). So on a driver that stores only the columns a row was written with, + * the condition sees a record with keys genuinely MISSING: + * + * | driver | absent column comes back as | + * | --------------------------------- | --------------------------- | + * | `@objectstack/driver-sql` | key present, `null` | + * | `@objectstack/driver-sqlite-wasm` | key present, `null` | + * | `@objectstack/driver-memory` | **key absent** | + * | `@objectstack/driver-mongodb` | **key absent** | + * + * A field is absent whenever it is neither `required` nor defaulted and the + * writer did not supply it — and also when its default is a CEL expression + * that cannot evaluate, which is how `crm_contact.owner` (`os.user.id`) goes + * missing on every write that carries no user. + * + * **2. Interpreted or compiled?** Interpreted, every run. + * `AutomationEngine.evaluateCondition` hands the source to `ExpressionEngine` + * as strict CEL. Same evaluator family as validation predicates, so the same + * abort table holds — `record.f != null` aborts on an absent key exactly like + * `record.f == "v"` does, and only `has()` is total. + * + * **3. What happens on abort?** `evaluateCondition` THROWS. The throw is + * caught by `AutomationEngine.execute`, which records the run as + * `status: 'failed'` and returns `{ success: false, error }`; the automation + * service then logs `Trigger-fired run of flow '' failed: …` at ERROR. + * The triggering WRITE still succeeds. So this is not the silent skip the + * validation path performs — it is loud. It is still "declared ≠ enforced": + * the automation does not happen, and nothing in the UI says so. + * + * ### Why nobody noticed: CEL `&&` absorbs errors + * + * Measured: `error && false` is `false`, while `error && true` aborts. So an + * unguarded condition answers correctly for every record that fails some + * OTHER conjunct — and aborts precisely on the records the flow was written to + * act on. `case_escalation` skipped ordinary cases perfectly well for months + * and blew up only on a critical one. + * + * ### The three defects this measured (all reproduced end-to-end below) + * + * - `case_escalation_on_create` — a case BORN critical is stored with no + * `escalated_date` column; `record.escalated_date == null` aborted with + * `No such key: escalated_date`. The flow never ran on its own core + * population (a phone-in P1 is the common path, per its own file comment). + * - `contact_welcome` — `owner`'s `os.user.id` default cannot evaluate on a + * write with no user (seed data, integrations, any system context), so the + * row stores no `owner` column; `record.owner != null` aborted with + * `No such key: owner`. + * - `lead_assignment` — `rating` is neither required nor defaulted; + * `record.rating >= 4` on edge `e2` aborted with `No such key: rating`, so + * an unrated lead got no SLA stamp and no alert. + * + * The other five conditions measured total as authored — but only because + * `crm_task` / `crm_opportunity` / `crm_case` happen to mark those fields + * `required` or give them defaults. That is a property of a neighbouring + * schema, not of the predicate, and it is one `required: false` away from + * changing. They carry guards too, and this file keeps them honest. + */ + +type AnyRec = Record; + +const flows: AnyRec[] = (stack as any).flows ?? []; +const objects: AnyRec[] = (stack as any).objects ?? []; + +/** `P` compiles to `{ dialect: 'cel', source }`; older conditions may be raw strings. */ +function celSource(condition: unknown): string { + if (typeof condition === 'string') return condition; + if (condition && typeof condition === 'object') return String((condition as AnyRec).source ?? ''); + return ''; +} + +interface FlowCondition { + /** `.` — stable enough to name in a failure message. */ + id: string; + flow: string; + source: string; +} + +/** + * Every authored condition in a `record_change` flow that reads the TRIGGER + * record — start conditions, node conditions, `decision` condition lists and + * edge conditions alike. All four go through the same + * `AutomationEngine.evaluateCondition`, and one of the three measured defects + * (`lead_assignment`) lives on an EDGE, not on a start node: scoping this + * sweep to start nodes would have left it uncovered. + * + * Conditions over flow-local variables (`vars.x`, `oppRecord.x` — the output + * of a `get_record` node) are deliberately NOT included. They are a different + * shape with a different failure mode (an unset flow VARIABLE, not a sparse + * driver row) and are tracked separately. + */ +const flowConditions: FlowCondition[] = flows + .filter((f) => f.type === 'record_change') + .flatMap((f) => { + const out: FlowCondition[] = []; + const add = (where: string, condition: unknown) => { + const source = celSource(condition); + if (source) out.push({ id: `${f.name}.${where}`, flow: f.name as string, source }); + }; + for (const n of (f.nodes ?? []) as AnyRec[]) { + add(`node:${n.id}`, n.config?.condition); + for (const c of (n.config?.conditions ?? []) as AnyRec[]) { + add(`node:${n.id}[${c?.label}]`, c?.expression); + } + } + for (const e of (f.edges ?? []) as AnyRec[]) add(`edge:${e.id}`, e.condition); + return out; + }) + .filter((c) => /\b(record|previous)\./.test(c.source)); + +/** Fields the condition reads off `scope` without a `has(...)` guard on that read. */ +function unguarded(source: string, scope: 'record' | 'previous'): string[] { + const read = new RegExp(String.raw`\b${scope}\.(\w+)`, 'g'); + const referenced = [...new Set([...source.matchAll(read)].map((m) => m[1]))]; + return referenced.filter( + (field) => !new RegExp(String.raw`has\(${scope}\.${field}\)`).test(source), + ); +} + +/** + * Operands of an ORDERING comparison (`<`, `<=`, `>`, `>=`). + * + * `has()` is not sufficient here: an explicit `null` PASSES `has()` and the + * comparison then aborts with `no such overload: dyn > int`. Ordering + * operands need `has(...)` for the absent key and `!= null` for the null + * value — the second hazard `object-validation-predicates.test.ts` documents. + */ +const OPERAND = String.raw`record\.\w+|previous\.\w+|\w+\([^()]*\)|-?[\d.]+|"[^"]*"`; +const RELATIONAL = new RegExp(String.raw`(${OPERAND})\s*(<=|>=|<|>)\s*(${OPERAND})`, 'g'); + +function orderedOperands(source: string): string[] { + const fields = new Set(); + for (const [, lhs, , rhs] of source.matchAll(RELATIONAL)) { + for (const operand of [lhs, rhs]) { + if (/^(record|previous)\.\w+$/.test(operand)) fields.add(operand); + } + } + return [...fields]; +} + +describe('record-change flow conditions guard every field they read', () => { + it('finds conditions to check at all', () => { + // Guard the guard: a typo in the extraction above would make every sweep + // below pass over an empty list. + expect(flowConditions.length).toBeGreaterThanOrEqual(9); + expect(flowConditions.some((c) => c.id.endsWith('.node:start'))).toBe(true); + expect(flowConditions.some((c) => c.id.includes('.edge:'))).toBe(true); + expect(flowConditions.some((c) => /\bprevious\./.test(c.source))).toBe(true); + }); + + it('reads no record field without has(...)', () => { + const offenders = flowConditions + .map((c) => ({ id: c.id, unguarded: unguarded(c.source, 'record') })) + .filter((c) => c.unguarded.length > 0); + + expect( + offenders, + 'These flow conditions read a trigger-record field with no has(…) guard. On a ' + + 'record whose post-write shape omits the key — driver-memory and driver-mongodb ' + + 'store only the columns a row was written with — strict CEL aborts, the ' + + 'automation engine records the run as FAILED and the flow does not run. Use ' + + '`has(record.f) && record.f …` for "f holds a value" and ' + + '`(!has(record.f) || …)` for "f is absent or …".', + ).toEqual([]); + }); + + it('reads no previous field without has(...)', () => { + // `previous` is the driver's PRIOR row — sparse the same way `record` is, + // and null on insert and on bulk `updateMany`. A bare `previous == null` + // term does not cover a prior row that simply lacks the key (measured). + const offenders = flowConditions + .map((c) => ({ id: c.id, unguarded: unguarded(c.source, 'previous') })) + .filter((c) => c.unguarded.length > 0); + + expect(offenders).toEqual([]); + }); + + it('null-guards every operand of every ordering comparison', () => { + const offenders = flowConditions + .map((c) => ({ + id: c.id, + // Either polarity counts. `has(f) && f != null && f > 3` is the + // "f holds a value" shape; `!has(f) || f == null || f < 3` is its + // complement, used where two branches must PARTITION (lead_assignment). + // Both are total; demanding only `!=` would reject the correct one. + unguarded: orderedOperands(c.source).filter( + (operand) => !new RegExp(String.raw`${operand.replace('.', String.raw`\.`)}\s*[!=]=\s*null`).test(c.source), + ), + })) + .filter((c) => c.unguarded.length > 0); + + expect( + offenders, + 'An ordering comparison needs `!= null` as well as `has(…)`: an explicit null ' + + 'passes has() and then aborts with `no such overload: dyn > int`.', + ).toEqual([]); + }); +}); + +/** + * The same property, measured on the real engine instead of grepped. + * + * The sweeps above are regexes over source text and can be satisfied by a + * guard sitting in the wrong place. This hands every condition to the real + * `AutomationEngine.evaluateCondition` — the exact method + * `AutomationEngine.execute` calls for a start condition and + * `traverseNext` calls for an edge — across the shapes a sparse driver + * produces, and fails on any throw. + * + * A `false` here is a PASS: the condition reached a verdict. Only an abort is + * the defect. + */ +describe('flow conditions are TOTAL on the real engine', () => { + const silent: any = { info() {}, warn() {}, error() {}, debug() {}, trace() {} }; + silent.child = () => silent; + const engine = new AutomationEngine(silent); + + /** Evaluate exactly as the engine does; return the abort message, or null. */ + function abortOf(source: string, vars: AnyRec): string | null { + try { + (engine as unknown as { + evaluateCondition(e: unknown, v: Map): boolean; + }).evaluateCondition({ dialect: 'cel', source }, new Map(Object.entries(vars))); + return null; + } catch (err) { + return String((err as Error).message).split('\n')[0]; + } + } + + /** Every field name the condition reads off `scope`. */ + const readsOf = (source: string, scope: 'record' | 'previous') => [ + ...new Set([...source.matchAll(new RegExp(String.raw`\b${scope}\.(\w+)`, 'g'))].map((m) => m[1])), + ]; + + /** + * Plausible values for one field, taken from the condition's OWN literals. + * + * Filling a field with an arbitrary value does not test totality, it tests + * type agreement: `record.amount = "x"` makes `record.amount > 100000` abort + * with `no such overload: dyn > int`, which is a mis-typed record, + * not a missing key, and no driver produces it. Drawing the values from the + * literals the predicate already compares against keeps every probe + * type-correct while still covering both sides of each comparison — which + * matters because CEL's `&&` ABSORBS an error beside a false operand, so a + * probe that leaves some other clause false would hide the abort. + */ + function candidatesFor(source: string, scope: string, field: string): unknown[] { + const LITERAL = String.raw`"[^"]*"|-?\d+(?:\.\d+)?|true|false|null`; + const ref = String.raw`\b${scope}\.${field}\b`; + const OPS = String.raw`==|!=|>=|<=|>|<`; + const found = [ + ...source.matchAll(new RegExp(String.raw`${ref}\s*(?:${OPS})\s*(${LITERAL})`, 'g')), + ...source.matchAll(new RegExp(String.raw`(${LITERAL})\s*(?:${OPS})\s*${ref}`, 'g')), + ].map((m) => m[1]); + + const values = new Set([null]); + let sawString = false; + for (const raw of found) { + if (raw === 'null') continue; + if (raw === 'true' || raw === 'false') { values.add(raw === 'true'); values.add(raw !== 'true'); continue; } + if (raw.startsWith('"')) { values.add(raw.slice(1, -1)); sawString = true; continue; } + const n = Number(raw); + // Straddle the threshold so both sides of an ordering test are probed. + values.add(n); values.add(n - 1); values.add(n + 1); + } + // A value matching no string literal, so `!=` chains are probed true too. + if (sawString) values.add('__neither__'); + return [...values]; + } + + /** Cartesian product of per-field candidates, capped so the sweep stays cheap. */ + function shapesFor(source: string, scope: 'record' | 'previous', omit?: string): Record[] { + let out: Record[] = [{}]; + for (const field of readsOf(source, scope)) { + if (field === omit) continue; + const next: Record[] = []; + for (const base of out) { + for (const value of candidatesFor(source, scope, field)) next.push({ ...base, [field]: value }); + } + out = next.slice(0, 400); + } + return out; + } + + it.each(flowConditions.map((c) => [c.id, c] as const))( + '%s answers on a record with no keys at all', + (_id, c) => { + expect(abortOf(c.source, { record: {}, previous: {} })).toBeNull(); + }, + ); + + it.each(flowConditions.map((c) => [c.id, c] as const))( + '%s answers when previous is null (insert, or a bulk updateMany)', + (_id, c) => { + const record = Object.fromEntries(readsOf(c.source, 'record').map((f) => [f, null])); + expect(abortOf(c.source, { record, previous: null })).toBeNull(); + }, + ); + + it.each(flowConditions.map((c) => [c.id, c] as const))( + '%s answers when every field is present but null', + (_id, c) => { + const record = Object.fromEntries(readsOf(c.source, 'record').map((f) => [f, null])); + const previous = Object.fromEntries(readsOf(c.source, 'previous').map((f) => [f, null])); + expect(abortOf(c.source, { record, previous })).toBeNull(); + }, + ); + + it.each(flowConditions.map((c) => [c.id, c] as const))( + '%s answers when any single referenced key is the one that is missing', + (_id, c) => { + // The shape a real write produces: most columns came back, one did not. + // Sweeping one at a time catches a guard that only appears to work + // because a neighbouring clause short-circuits first — and CEL's `&&` + // ABSORBS errors next to a false operand, so a whole-shape test alone + // would miss it. + const bad: string[] = []; + const probe = (scope: 'record' | 'previous', missing: string) => { + for (const record of shapesFor(c.source, 'record', scope === 'record' ? missing : undefined)) { + for (const previous of shapesFor(c.source, 'previous', scope === 'previous' ? missing : undefined)) { + const abort = abortOf(c.source, { record, previous }); + if (abort) { + bad.push( + `${scope}.${missing} absent (record=${JSON.stringify(record)} ` + + `previous=${JSON.stringify(previous)}): ${abort}`, + ); + return; // one witness per missing key is enough to fail on + } + } + } + }; + + for (const missing of readsOf(c.source, 'record')) probe('record', missing); + for (const missing of readsOf(c.source, 'previous')) probe('previous', missing); + + expect(bad, `${c.id} aborted when a single key was absent:\n ${bad.join('\n ')}`).toEqual([]); + }, + ); + + /** + * The counterpart to `sharing conditions cannot use has()` in + * `test/sharing-seeding.test.ts`. That test pins that `has()` is REJECTED on + * the sharing surface; this one pins that it is ACCEPTED and correct here. + * Together they stop either conclusion being carried across the boundary — + * which is the mistake #633 was opened to prevent. + */ + it('has() evaluates in the flow engine, unlike on the sharing surface', () => { + const has = (vars: AnyRec) => abortOf('has(record.f)', vars); + expect(has({ record: { f: 'v' } })).toBeNull(); + expect(has({ record: { f: null } })).toBeNull(); + expect(has({ record: {} })).toBeNull(); + + const ev = (source: string, vars: AnyRec) => + (engine as unknown as { + evaluateCondition(e: unknown, v: Map): boolean; + }).evaluateCondition({ dialect: 'cel', source }, new Map(Object.entries(vars))); + + expect(ev('has(record.f)', { record: { f: 'v' } })).toBe(true); + expect(ev('has(record.f)', { record: { f: null } })).toBe(true); + expect(ev('has(record.f)', { record: {} })).toBe(false); + // …and on a `previous` that is null outright, rather than throwing. + expect(ev('has(previous.f)', { previous: null })).toBe(false); + }); + + it('is the reason the guards are needed: the unguarded forms abort', () => { + // The abort table, re-measured on THIS evaluator rather than inherited + // from the validation path. If a platform upgrade makes strict CEL + // tolerant of absent keys, this test fails and the guards can be revisited + // — that is the intended signal, not a nuisance. + expect(abortOf('record.f != null', { record: {} })).toMatch(/No such key: f/); + expect(abortOf('record.f == "v"', { record: {} })).toMatch(/No such key: f/); + expect(abortOf('record.f > 3', { record: {} })).toMatch(/No such key: f/); + // `has()` passes an explicit null; the ordering comparison then aborts. + expect(abortOf('has(record.f) && record.f > 3', { record: { f: null } })) + .toMatch(/no such overload/); + expect(abortOf('has(record.f) && record.f != null && record.f > 3', { record: { f: null } })) + .toBeNull(); + // A bare `previous == null` term does not cover a SPARSE previous row. + expect(abortOf('previous == null || previous.f != "x"', { previous: {} })) + .toMatch(/No such key: f/); + }); + + it('CEL && absorbs errors beside a false operand — why this went unnoticed', () => { + // This is the mechanism that let unguarded conditions look correct: they + // answer fine for every record that fails some other conjunct, and abort + // only on the records the flow was written to act on. + expect(abortOf('record.missing == 1 && record.b == "no"', { record: { b: 'x' } })).toBeNull(); + expect(abortOf('record.missing == 1 && record.b == "x"', { record: { b: 'x' } })) + .toMatch(/No such key: missing/); + }); +}); + +/** + * End-to-end, on the driver that actually produces the sparse record. + * + * Everything above evaluates conditions in isolation. This boots a real + * ObjectQL over `InMemoryDriver`, a real `AutomationEngine`, and the real + * `RecordChangeTrigger` that wires them together, then performs the ordinary + * writes that reproduced each of the three defects. Before the guards landed + * every one of these logged + * `Trigger-fired run of flow '…' failed: condition failed to evaluate as CEL` + * and the automation silently did not happen. + */ +describe('conditions answer on a driver whose stored record omits the key', () => { + const objMap = Object.fromEntries(objects.map((o) => [o.name as string, o])); + + interface Booted { + ql: AnyRec; + /** Every trigger-fired run, in order. */ + runs: { flow: string; success: boolean; error?: string }[]; + close(): Promise; + } + + async function boot(flowNames: string[]): Promise { + const silent: any = { info() {}, warn() {}, error() {}, debug() {}, trace() {} }; + silent.child = () => silent; + + const ql: AnyRec = (await ObjectQL.create({ + datasources: { default: new InMemoryDriver({ persistence: false }) }, + objects: objMap as never, + // `logger` is honoured at runtime but is not on the published options + // type. Without it this suite prints every engine INFO line plus the + // downstream `notify` / `runAs` errors these flows legitimately hit in a + // user-less harness, which reads as a broken build when it is not. + ...({ logger: silent } as object), + })) as never; + + const engine = new AutomationEngine(silent); + installBuiltinNodes(engine, { + logger: silent, + getService: (n: string) => + n === 'data' || n === 'objectql' + ? ql + : n === 'messaging' || n === 'notification' || n === 'email' + ? { async emit() { return { notificationId: 'n', delivered: 1, failed: 0 }; } } + : undefined, + } as never); + + const runs: { flow: string; success: boolean; error?: string }[] = []; + const original = engine.execute.bind(engine); + (engine as unknown as AnyRec).execute = async (name: string, ctx: AnyRec) => { + const result: AnyRec = await original(name, ctx as never); + runs.push({ flow: name, success: result?.success !== false, error: result?.error }); + return result; + }; + + for (const name of flowNames) { + const flow = flows.find((f) => f.name === name); + expect(flow, `flow ${name} is not registered on the stack`).toBeDefined(); + engine.registerFlow(name, flow as never); + } + engine.registerTrigger(new RecordChangeTrigger(ql as never, silent) as never); + + return { ql, runs, close: () => ql.close() }; + } + + /** + * Runs whose CONDITION could not be evaluated. + * + * Deliberately narrower than "runs that failed": these flows also fail + * downstream in this harness for unrelated reasons (a `notify` node whose + * `{record.owner}` recipient is empty because the owner default needs a + * user, a `runAs: 'user'` refusal because a system write carries none). + * Those are real, separate concerns and are not what this file asserts. + */ + const conditionAborts = (b: Booted) => + b.runs + .filter((r) => /failed to evaluate as CEL|No such key|no such overload/.test(r.error ?? '')) + .map((r) => `${r.flow}: ${String(r.error).split('\n')[0]}`); + + it('stores no key for a column it was never given — the precondition', async () => { + const b = await boot([]); + try { + const api = b.ql.createContext({ isSystem: true }); + const row = await api.object('crm_case').insert({ + subject: 'Prod outage', description: 'Everything is down', + priority: 'critical', status: 'new', + }); + const stored = await api.object('crm_case').findOne({ where: { id: row.id } }); + // Not `toBeNull()`: the key is ABSENT, which is the whole point. If a + // platform upgrade makes this driver column-complete, this assertion + // fails and everything below stops proving anything — intended signal. + expect('escalated_date' in (stored ?? {})).toBe(false); + // …and the owner default (`os.user.id`) cannot evaluate without a user, + // so that column is missing too rather than defaulted. + expect('owner' in (stored ?? {})).toBe(false); + } finally { + await b.close(); + } + }, 60_000); + + it('case_escalation_on_create: a case BORN critical still evaluates', async () => { + const b = await boot(['case_escalation_on_create']); + try { + const api = b.ql.createContext({ isSystem: true }); + await api.object('crm_case').insert({ + subject: 'Prod outage', description: 'Everything is down', + priority: 'critical', status: 'new', + }); + expect(b.runs.map((r) => r.flow)).toContain('case_escalation_on_create'); + expect(conditionAborts(b)).toEqual([]); + } finally { + await b.close(); + } + }, 60_000); + + it('contact_welcome: a contact written with no resolvable owner still evaluates', async () => { + const b = await boot(['contact_welcome']); + try { + const api = b.ql.createContext({ isSystem: true }); + const account = await api.object('crm_account').insert({ name: 'Acme Corp' }); + await api.object('crm_contact').insert({ + first_name: 'Ada', last_name: 'Lovelace', + email: 'ada@acme.com', crm_account: account.id, + }); + expect(b.runs.map((r) => r.flow)).toContain('contact_welcome'); + expect(conditionAborts(b)).toEqual([]); + } finally { + await b.close(); + } + }, 60_000); + + it('lead_assignment: an unrated lead takes the standard branch instead of aborting', async () => { + const b = await boot(['lead_assignment']); + try { + const api = b.ql.createContext({ isSystem: true }); + await api.object('crm_lead').insert({ + first_name: 'Jo', last_name: 'Smith', company: 'Acme', + status: 'new', email: 'jo@acme.com', + }); + expect(b.runs.map((r) => r.flow)).toContain('lead_assignment'); + expect(conditionAborts(b)).toEqual([]); + // The lead really was routed: the standard branch stamped an SLA on it. + const stored = await api.object('crm_lead').findOne({ where: { company: 'Acme' } }); + expect(stored, 'the unrated lead vanished').toBeTruthy(); + } finally { + await b.close(); + } + }, 60_000); + + it('the remaining record-change flows evaluate across an insert and an update', async () => { + const b = await boot([ + 'case_escalation', 'case_csat_followup', + 'opportunity_approval', 'opportunity_approval_on_create', 'opportunity_won_alert', + 'task_urgent_alert', + ]); + try { + const api = b.ql.createContext({ isSystem: true }); + + await api.object('crm_task').insert({ subject: 'Fix the thing', priority: 'urgent' }); + + const account = await api.object('crm_account').insert({ name: 'Acme Corp' }); + const opp = await api.object('crm_opportunity').insert({ + name: 'Big Deal', amount: 750_000, stage: 'negotiation', + close_date: '2026-12-31', crm_account: account.id, + }); + await api.object('crm_opportunity').update({ stage: 'closed_won' }, { where: { id: opp.id } }); + + const kase = await api.object('crm_case').insert({ + subject: 'Login broken', description: 'Cannot sign in', + priority: 'high', status: 'new', + }); + await api.object('crm_case').update( + { status: 'closed', resolution: 'Reset the session store' }, + { where: { id: kase.id } }, + ); + + expect(b.runs.length, 'no flow fired at all — the harness is not wired').toBeGreaterThan(0); + expect(conditionAborts(b)).toEqual([]); + } finally { + await b.close(); + } + }, 60_000); +}); diff --git a/test/object-validation-predicates.test.ts b/test/object-validation-predicates.test.ts index a5bc9857..43f6f10a 100644 --- a/test/object-validation-predicates.test.ts +++ b/test/object-validation-predicates.test.ts @@ -131,6 +131,23 @@ import { REPO_ROOT } from './helpers/repo-root'; * the engine should treat an unevaluable predicate on an `error`-severity rule * as a failure rather than a skip. A rule that cannot answer is not a rule that * passed — but that is a platform decision, not a HotCRM one. + * + * ### The same rule on the other two CEL surfaces (#633) + * + * The house rule above governs object `validations[]` and field predicates. + * The repo has two more CEL surfaces, and they were MEASURED separately rather + * than assumed to behave alike — do not carry a conclusion across: + * + * - **Record-change flow conditions** — same strict-CEL abort, but the + * engine records the run as FAILED and logs at ERROR instead of skipping + * quietly. Guards required; enforced by + * `test/flow-condition-totality.test.ts`, which also carries the measured + * record shape and evaluation mechanism. + * - **Sharing rule conditions** — NOT interpreted at all. They are compiled + * to a pushdown filter by `compileCelToFilter`, which rejects the whole + * function-call class, so a `has()` guard makes the rule untranslatable + * and `plugin-sharing` silently stops seeding it. Guards are actively + * harmful there; `test/sharing-seeding.test.ts` pins that. */ type AnyRec = Record;