fix(flows): make record-change conditions total so three automations stop silently not running

.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<null> > 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.

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")`,
       },
     },
     {

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"))`,
       },
     },
     {

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)`,
       },
     },
     {

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<null> >= 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' },

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<null> > 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"))`,
       },
     },
     {

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<null> > 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`,
       },
     },
     {

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")`,
       },
     },
     {

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'],
   },