fix(flows): make conditions over flow variables total, in the two ways their two failure classes need (#643)

.changeset/flow-variable-condition-totality.md

@@ -0,0 +1,25 @@
+---
+"hotcrm": patch
+---
+
+Fix two automations that could stop running mid-flow, and pin the property that
+prevents it.
+
+**Enroll Leads in Campaign** aborted whenever the campaign it was launched from
+had been deleted — or was hidden from the running user by a sharing rule —
+between clicking the action and the flow reaching its "Campaign Open?" gate. The
+gate read the campaign's status off a record that was no longer there, the run
+was recorded as failed, and not one lead was enrolled. It now reaches a verdict
+on every shape and simply enrols nobody when the campaign cannot be read.
+
+**Lead Conversion Process** aborted at "Create Opportunity?" whenever the
+conversion screen came back without an answer for that checkbox — the ordinary
+case when the user leaves it alone. The lead was never marked converted and no
+account, contact or opportunity survived the run. The flow now starts from the
+same default the screen shows ("no opportunity"), so an unanswered checkbox
+converts the lead exactly as leaving it clear was always meant to.
+
+Two scheduled automations were hardened against the same class of failure before
+it could bite: **Contract Renewal** (a contract whose renewal-notice days or
+auto-renewal flag were never written would have taken the whole 500-contract
+sweep down with it) and the **Large Deal Approval** tier gate.

src/flows/campaign-enrollment.flow.ts

@@ -64,8 +64,20 @@ export const CampaignEnrollmentFlow: Flow = {
       // Only enroll into campaigns that are actually running (or planned):
       // topping up a completed/aborted campaign would corrupt its final
       // snapshot metrics. (Status values: planning / in_progress / completed / aborted.)
+      //
+      // TOTALITY (#643): `has(vars.campaignRecord)` first, then
+      // `has(vars.campaignRecord.status)`. `campaignRecord` is a `get_record`
+      // OUTPUT, and `findOne` answers a miss with `null` — so a campaign that
+      // was deleted (or hidden by sharing) between the action click and this
+      // node leaves the variable bound to null, and the unguarded read aborted
+      // with `No such key: status` on edge `e4`. Reproduced end-to-end: the run
+      // was recorded `failed` and not one lead was enrolled. `status` itself is
+      // `required` on `crm_campaign` today so the column is never sparse — but
+      // that is the neighbouring schema doing the work, not this predicate, so
+      // it is guarded too.
       id: 'check_campaign_open', type: 'decision', label: 'Campaign Open?',
-      config: { condition: P`vars.campaignRecord.status == "planning" || vars.campaignRecord.status == "in_progress"` },
+      config: { condition: P`has(vars.campaignRecord) && has(vars.campaignRecord.status)
+        && (vars.campaignRecord.status == "planning" || vars.campaignRecord.status == "in_progress")` },
     },
     {
       id: 'query_leads', type: 'get_record', label: 'Find Eligible Leads',
@@ -128,7 +140,12 @@ export const CampaignEnrollmentFlow: Flow = {
     { id: 'e2', source: 'screen_1', target: 'get_campaign', type: 'default' },
     { id: 'e3', source: 'get_campaign', target: 'check_campaign_open', type: 'default' },
     // Closed campaign → no edge → flow ends without enrolling.
-    { id: 'e4', source: 'check_campaign_open', target: 'query_leads', type: 'conditional', condition: P`vars.campaignRecord.status == "planning" || vars.campaignRecord.status == "in_progress"`, label: 'Open' },
+    // Guarded identically to `check_campaign_open` — see the note there. The
+    // EDGE is the live site: a `decision` node's singular `config.condition` is
+    // never read by the engine (it evaluates `config.conditions[]`), so this
+    // copy is the one that decides, and the one that aborted.
+    { id: 'e4', source: 'check_campaign_open', target: 'query_leads', type: 'conditional', condition: P`has(vars.campaignRecord) && has(vars.campaignRecord.status)
+      && (vars.campaignRecord.status == "planning" || vars.campaignRecord.status == "in_progress")`, label: 'Open' },
     { id: 'e5', source: 'query_leads', target: 'loop_leads', type: 'default' },
     { id: 'e6', source: 'loop_leads', target: 'end', type: 'default' },
   ],

src/flows/contract-renewal.flow.ts

@@ -66,7 +66,22 @@ export const ContractRenewalFlow: Flow = {
               // blowing up mid-sweep — a defect that only became REACHABLE once
               // the condition was wrapped as a real CEL envelope, because the
               // old bare string was never evaluated at all.
-              config: { condition: P`timestamp(currentContract.end_date + "T00:00:00Z") <= daysFromNow(int(currentContract.renewal_notice_days))` },
+              //
+              // TOTALITY (#643): `currentContract` is a LOOP ITEM over
+              // `contractList`, which `get_record` filled from `data.find` —
+              // every element is a raw driver row, sparse in exactly the way
+              // #633 measured. `end_date` is `required` on `crm_contract` so
+              // that column is always written, but `renewal_notice_days`
+              // (`defaultValue: 30`) and `auto_renewal` (`defaultValue: false`)
+              // are only DEFAULTED, and a row written before the default
+              // existed carries neither the column nor a value. Both operands
+              // additionally need `!= null`, because the abort here is not the
+              // usual overload error: `null + "T00:00:00Z"` and `int(null)`
+              // each blow up inside the function call, one contract into a
+              // 500-row sweep, taking the whole scheduled run with them.
+              config: { condition: P`has(vars.currentContract) && has(vars.currentContract.end_date) && has(vars.currentContract.renewal_notice_days)
+                && vars.currentContract.end_date != null && vars.currentContract.renewal_notice_days != null
+                && timestamp(vars.currentContract.end_date + "T00:00:00Z") <= daysFromNow(int(vars.currentContract.renewal_notice_days))` },
             },
             {
               // Idempotency gate: the sweep matches the same contract every
@@ -116,7 +131,11 @@ export const ContractRenewalFlow: Flow = {
             },
             {
               id: 'check_auto_renewal', type: 'decision', label: 'Auto-Renewal On?',
-              config: { condition: P`currentContract.auto_renewal == true` },
+              // TOTALITY (#643): same loop item, same sparse driver row. Only
+              // an explicit `true` opens a renewal deal, so an absent column
+              // reads as "auto-renewal off" — the conservative branch.
+              config: { condition: P`has(vars.currentContract) && has(vars.currentContract.auto_renewal)
+                && vars.currentContract.auto_renewal == true` },
             },
             {
               // Second gate: never open a second renewal opportunity while one
@@ -157,12 +176,18 @@ export const ContractRenewalFlow: Flow = {
           edges: [
             // Only act when inside the per-contract notice window; gates with
             // no matching edge simply end the iteration, so the loop moves on.
-            { id: 'b1', source: 'check_notice_window', target: 'find_existing_task', type: 'conditional', condition: P`timestamp(currentContract.end_date + "T00:00:00Z") <= daysFromNow(int(currentContract.renewal_notice_days))`, label: 'In window' },
+            // Guarded identically to `check_notice_window` — see the note there.
+            // The EDGE is the live site: the engine never reads a `decision`
+            // node's singular `config.condition`, only `config.conditions[]`.
+            { id: 'b1', source: 'check_notice_window', target: 'find_existing_task', type: 'conditional', condition: P`has(vars.currentContract) && has(vars.currentContract.end_date) && has(vars.currentContract.renewal_notice_days)
+              && vars.currentContract.end_date != null && vars.currentContract.renewal_notice_days != null
+              && timestamp(vars.currentContract.end_date + "T00:00:00Z") <= daysFromNow(int(vars.currentContract.renewal_notice_days))`, label: 'In window' },
             { id: 'b2', source: 'find_existing_task', target: 'check_not_reminded', type: 'default' },
             { id: 'b3', source: 'check_not_reminded', target: 'create_renewal_task', type: 'conditional', condition: P`existingRenewalTask == null`, label: 'First reminder' },
             { id: 'b4', source: 'create_renewal_task', target: 'notify_owner', type: 'default' },
             { id: 'b5', source: 'notify_owner', target: 'check_auto_renewal', type: 'default' },
-            { id: 'b6', source: 'check_auto_renewal', target: 'find_existing_renewal_opp', type: 'conditional', condition: P`currentContract.auto_renewal == true`, label: 'Auto-renew' },
+            { id: 'b6', source: 'check_auto_renewal', target: 'find_existing_renewal_opp', type: 'conditional', condition: P`has(vars.currentContract) && has(vars.currentContract.auto_renewal)
+              && vars.currentContract.auto_renewal == true`, label: 'Auto-renew' },
             { id: 'b7', source: 'find_existing_renewal_opp', target: 'check_no_open_renewal', type: 'default' },
             { id: 'b8', source: 'check_no_open_renewal', target: 'create_renewal_opp', type: 'conditional', condition: P`existingRenewalOpp == null`, label: 'Open renewal deal' },
           ],

src/flows/lead-conversion.flow.ts

@@ -25,6 +25,32 @@ export const LeadConversionFlow: Flow = {
 
   nodes: [
     { id: 'start', type: 'start', label: 'Start', config: { objectName: 'crm_lead' } },
+    {
+      // BINDING, not guarding (#643). `createOpportunity` is the only variable
+      // any condition in this flow reads that no node upstream of the read
+      // assigns: `matchedAccount` / `matchedContact` are `get_record` outputs
+      // and `get_record` always writes its `outputVariable` (with `null` on a
+      // miss), but `createOpportunity` arrives only if the screen runner sends
+      // it back in the resume signal. A runner that posts just the fields the
+      // user touched leaves it UNBOUND, and edge `e16` then aborts with
+      // `No such key: createOpportunity` — reproduced end-to-end: the run is
+      // recorded `failed` and the lead is never marked converted.
+      //
+      // The remedy is NOT a `has()` guard. A guard would encode "a missing
+      // answer means No" inside the predicate; what is actually wrong is that
+      // the graph left the variable unbound. Declaring it in `flow.variables`
+      // does not help either — measured on 17.0.0-rc.1, `FlowVariableSchema` is
+      // strict `{ name, type, isInput, isOutput }` with NO `defaultValue`, and
+      // `AutomationEngine.execute` binds a declared input only when
+      // `context.params[name] !== undefined`. So the binding has to be an
+      // `assignment` node, and it has to sit ahead of the screen so the resume
+      // signal overwrites it whenever the runner does answer.
+      //
+      // `false` mirrors the screen field's own `defaultValue: false` — the
+      // commonest path is "convert this lead WITHOUT an opportunity".
+      id: 'init_defaults', type: 'assignment', label: 'Default Conversion Options',
+      config: { assignments: { createOpportunity: false } },
+    },
     {
       id: 'screen_1', type: 'screen', label: 'Conversion Details',
       config: {
@@ -185,7 +211,8 @@ export const LeadConversionFlow: Flow = {
   ],
 
   edges: [
-    { id: 'e1', source: 'start', target: 'screen_1', type: 'default' },
+    { id: 'e0', source: 'start', target: 'init_defaults', type: 'default' },
+    { id: 'e1', source: 'init_defaults', target: 'screen_1', type: 'default' },
     { id: 'e2', source: 'screen_1', target: 'get_lead', type: 'default' },
     { id: 'e3', source: 'get_lead', target: 'find_account', type: 'default' },
     { id: 'e4', source: 'find_account', target: 'decision_account', type: 'default' },

src/flows/opportunity-approval.flow.ts

@@ -108,7 +108,20 @@ export const OpportunityApprovalFlow: Flow = {
       id: 'check_high_value',
       type: 'decision',
       label: 'High Value (> $500K)?',
-      config: { condition: P`oppRecord.amount > 500000` },
+      // TOTALITY (#643): `oppRecord` is a `get_record` OUTPUT, so the read
+      // needs `has(vars.oppRecord)` (the variable), `has(vars.oppRecord.amount)`
+      // (the column — `findOne` answers a miss with `null`, and a sparse driver
+      // row omits an unwritten column outright) and `!= null` (an explicit null
+      // passes `has()` and the ordering comparison then aborts with
+      // `no such overload: dyn<null> > int`). Measured total as authored today
+      // — `amount` is `required` on `crm_opportunity` and this `get_record` is
+      // keyed on the row that just fired the trigger — but that is the
+      // neighbouring schema doing the work, exactly as in the start condition
+      // above. `vars.`-scoped rather than bare: `has(oppRecord.amount)` still
+      // aborts with `Unknown variable: oppRecord` on an unbound variable,
+      // `has(vars.oppRecord)` answers `false` (measured).
+      config: { condition: P`has(vars.oppRecord) && has(vars.oppRecord.amount)
+        && vars.oppRecord.amount != null && vars.oppRecord.amount > 500000` },
     },
 
     // ── Tier 2: Sales Director sign-off (deals > $500K only) ────────
@@ -188,9 +201,17 @@ export const OpportunityApprovalFlow: Flow = {
     { id: 'e3', source: 'manager_review', target: 'check_high_value', type: 'default', label: 'approve' },
     { id: 'e4', source: 'manager_review', target: 'mark_rejected', type: 'default', label: 'reject' },
 
-    // Tier gate (decision-node conditional branches)
-    { id: 'e5', source: 'check_high_value', target: 'director_signoff', type: 'conditional', condition: P`oppRecord.amount > 500000`, label: 'High value (> $500K)' },
-    { id: 'e6', source: 'check_high_value', target: 'mark_approved', type: 'conditional', condition: P`oppRecord.amount <= 500000`, label: 'Standard (≤ $500K)' },
+    // Tier gate (decision-node conditional branches). These EDGES are the live
+    // sites — the engine never evaluates a `decision` node's singular
+    // `config.condition` — and they must PARTITION, so the guards are written
+    // in opposite polarity. A deal whose amount cannot be read lands on
+    // `mark_approved`: the manager has already approved it, and an unreadable
+    // amount must not strand an approved deal in a locked, undecidable
+    // director step.
+    { id: 'e5', source: 'check_high_value', target: 'director_signoff', type: 'conditional', condition: P`has(vars.oppRecord) && has(vars.oppRecord.amount)
+      && vars.oppRecord.amount != null && vars.oppRecord.amount > 500000`, label: 'High value (> $500K)' },
+    { id: 'e6', source: 'check_high_value', target: 'mark_approved', type: 'conditional', condition: P`!has(vars.oppRecord) || !has(vars.oppRecord.amount)
+      || vars.oppRecord.amount == null || vars.oppRecord.amount <= 500000`, label: 'Standard (≤ $500K)' },
 
     // Director decision (approval-node branch labels)
     { id: 'e7', source: 'director_signoff', target: 'mark_approved', type: 'default', label: 'approve' },

src/flows/quote-generation.flow.ts

@@ -60,8 +60,22 @@ export const QuoteGenerationFlow: Flow = {
       // machine allows `→ proposal` from all three). Re-writing `proposal` on
       // a deal already at proposal/negotiation was an illegal self/backward
       // transition — those deals keep their stage; the quote is still created.
+      //
+      // TOTALITY (#643): `oppRecord` is a `get_record` OUTPUT — `findOne`
+      // answers a miss with `null`, and reading a field off it then aborts with
+      // `No such key: stage`. Measured unreachable TODAY only because two
+      // neighbouring schemas happen to close it: `crm_opportunity.stage` is
+      // `required` (never a sparse column) and `crm_quote.crm_account` is
+      // `required`, so a null `oppRecord` makes `create_quote` fail one node
+      // earlier. Both are one `required: false` away from re-opening it, so the
+      // predicate carries its own guard. Note the scope is `vars.oppRecord`,
+      // not bare `oppRecord`: measured, `has(oppRecord.stage)` still aborts
+      // with `Unknown variable: oppRecord` when the variable is unbound, while
+      // `has(vars.oppRecord)` answers `false` — only the `vars.`-scoped form is
+      // total against both hazards.
       id: 'check_stage', type: 'decision', label: 'Can Advance to Proposal?',
-      config: { condition: P`oppRecord.stage == "prospecting" || oppRecord.stage == "qualification" || oppRecord.stage == "needs_analysis"` },
+      config: { condition: P`has(vars.oppRecord) && has(vars.oppRecord.stage)
+        && (vars.oppRecord.stage == "prospecting" || vars.oppRecord.stage == "qualification" || vars.oppRecord.stage == "needs_analysis")` },
     },
     {
       id: 'update_opportunity', type: 'update_record', label: 'Update Opportunity',
@@ -93,8 +107,16 @@ export const QuoteGenerationFlow: Flow = {
     { id: 'e2', source: 'screen_1', target: 'get_opportunity', type: 'default' },
     { id: 'e3', source: 'get_opportunity', target: 'create_quote', type: 'default' },
     { id: 'e4', source: 'create_quote', target: 'check_stage', type: 'default' },
-    { id: 'e4a', source: 'check_stage', target: 'update_opportunity', type: 'conditional', condition: P`oppRecord.stage == "prospecting" || oppRecord.stage == "qualification" || oppRecord.stage == "needs_analysis"`, label: 'Advance' },
-    { id: 'e4b', source: 'check_stage', target: 'notify_owner', type: 'conditional', condition: P`oppRecord.stage != "prospecting" && oppRecord.stage != "qualification" && oppRecord.stage != "needs_analysis"`, label: 'Keep stage' },
+    // The two branches must PARTITION, so the guards are written in opposite
+    // polarity: `has(…) && …` on the advance side, `!has(…) || …` on the keep
+    // side. An unknown stage therefore lands on "keep stage" — the quote is
+    // still created and nothing illegal is written to the state machine.
+    // These EDGES are the live sites; `check_stage`'s own `config.condition` is
+    // never evaluated by the engine (see the note on that node).
+    { id: 'e4a', source: 'check_stage', target: 'update_opportunity', type: 'conditional', condition: P`has(vars.oppRecord) && has(vars.oppRecord.stage)
+      && (vars.oppRecord.stage == "prospecting" || vars.oppRecord.stage == "qualification" || vars.oppRecord.stage == "needs_analysis")`, label: 'Advance' },
+    { id: 'e4b', source: 'check_stage', target: 'notify_owner', type: 'conditional', condition: P`!has(vars.oppRecord) || !has(vars.oppRecord.stage)
+      || (vars.oppRecord.stage != "prospecting" && vars.oppRecord.stage != "qualification" && vars.oppRecord.stage != "needs_analysis")`, label: 'Keep stage' },
     { id: 'e5', source: 'update_opportunity', target: 'notify_owner', type: 'default' },
     { id: 'e6', source: 'notify_owner', target: 'end', type: 'default' },
   ],

test/flow-condition-totality.test.ts

@@ -136,8 +136,15 @@ interface FlowCondition {
  *
  * 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.
+ * shape with a different failure mode and are owned by
+ * `test/flow-variable-conditions.test.ts`, which measured them separately
+ * (#643) and found TWO classes with opposite remedies: a field read off a
+ * `get_record` output wants `has()` guards like these, while an unbindable
+ * VARIABLE wants binding in the flow graph and must NOT be guarded. Do not
+ * carry either conclusion across — that file's guards are additionally
+ * `vars.`-scoped, because measured, the bare `has(oppRecord.f)` spelling still
+ * aborts with `Unknown variable: oppRecord` on an unbound variable while
+ * `has(vars.oppRecord)` answers `false`.
  */
 const flowConditions: FlowCondition[] = flows
   .filter((f) => f.type === 'record_change')