From 2ec93391e96b82a734b39102cdb6a5c86aa6e363 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 18:56:10 +0000 Subject: [PATCH] fix(flows): make conditions over flow variables total, in the two ways their two failure classes need (#643) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test/flow-condition-totality.test.ts` (#633) made every `record.x` / `previous.x` read in a record-change flow condition total. It filtered explicitly to the trigger record, leaving conditions over flow-local variables unmeasured. Measured now, on 17.0.0-rc.1, they are exposed in TWO distinct ways whose remedies are opposite. Measured mechanism (`AutomationEngine.evaluateCondition`): | expression | var unbound | var = null | sparse row | | ---------------- | ------------------------ | ---------- | ---------- | | `X.f == 1` | ABORT Unknown variable | ABORT | ABORT | | `vars.X.f == 1` | ABORT No such key | ABORT | ABORT | | `has(X.f)` | ABORT Unknown variable | false | false | | `has(vars.X)` | false | true | true | Only the `vars.`-scoped guard survives an unbound variable, so every guard added here leads with `has(vars.X)`. Three further facts decided the shape of the fix: `get_record` always writes its `outputVariable` and `findOne` answers a miss with `null`; a failed node throws and the run stops, so a node that could not bind never reaches a reader; and declaring a variable in `flow.variables` binds NOTHING at runtime — `FlowVariableSchema` is strict `{ name, type, isInput, isOutput }` with no `defaultValue`, and `execute` binds a declared input only when the caller passed it in `context.params`. Class 1 — field reads off a `get_record` output are driver rows, and get #633's guards (`vars.`-scoped): - `campaign_enrollment` `vars.campaignRecord.status` (node check_campaign_open, edge e4) — LIVE DEFECT, reproduced end-to-end. A campaign deleted or sharing-hidden between the action click and the run left the variable bound to `null`; the read aborted with `No such key: status`, the run was recorded failed and no lead was enrolled. `crm_campaign.status` is `required`, so the sparse-COLUMN variant was already closed — the null-RECORD variant was not. - `quote_generation` `oppRecord.stage` (check_stage, e4a/e4b) and `opportunity_approval` (+ `_on_create`) `oppRecord.amount` (check_high_value, e5/e6) — total TODAY only because neighbouring schemas close the gap (`stage`/`amount` are `required`; `crm_quote.crm_account` is `required` so a null `oppRecord` fails `create_quote` one node earlier). Guarded anyway. - `contract_renewal` `currentContract.end_date` / `.renewal_notice_days` / `.auto_renewal` (check_notice_window, check_auto_renewal, b1/b6) — NOT in the issue's table; found by sweeping every flow. A loop item over `data.find` rows, with the two renewal columns only DEFAULTED, and the aborts land inside `timestamp()` / `int()`, taking a 500-contract sweep down with them. Class 2 — an unbindable VARIABLE, which must NOT get a `has()` guard: - `lead_conversion` `vars.createOpportunity` (decision_opportunity, e16/e17) — LIVE DEFECT, reproduced end-to-end. It is a screen- collected input, so it is bound only if the runner sends it back in the resume signal; a runner posting just the touched fields left it unbound and edge e16 aborted with `No such key: createOpportunity`, so the lead was never marked converted. Fixed by BINDING it — an `assignment` node ahead of the screen seeding `false`, matching the screen field's own `defaultValue` — not by guarding. A guard would bury the policy "a missing answer means No" inside a predicate and leave the graph defect in place. - `matchedAccount` / `matchedContact` / `firstUser` / `existingMember` / `existingRenewalTask` / `existingRenewalOpp` / `existingStallTask` / `ownerAnyDeal` / `existingForecast` measured CLEAN — a `get_record` dominates every read — and are deliberately left unguarded. `test/flow-variable-conditions.test.ts` pins both properties as separate assertions with distinct failure messages (per #643 item 3): a `has()` sweep for field reads, and a structural intersection-dataflow proof that every variable a condition reads is bound on every path reaching it. It also re-measures the abort table on this evaluator, sweeps every condition on the real engine, and reproduces both defects end-to-end over `InMemoryDriver`. Without the flow fixes it fails 21 assertions. Also documented, and used by the sweep: a `decision` node's SINGULAR `config.condition` — the shape every flow here authors — is never read by the engine, which evaluates `config.conditions[]` and the edges. Both copies are guarded so the inert statement of intent cannot drift from the edge that decides. Fixes #643 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019SS7C5SXpniKeCApxgARyf --- .../flow-variable-condition-totality.md | 25 + src/flows/campaign-enrollment.flow.ts | 21 +- src/flows/contract-renewal.flow.ts | 33 +- src/flows/lead-conversion.flow.ts | 29 +- src/flows/opportunity-approval.flow.ts | 29 +- src/flows/quote-generation.flow.ts | 28 +- test/flow-condition-totality.test.ts | 11 +- test/flow-variable-conditions.test.ts | 843 ++++++++++++++++++ 8 files changed, 1003 insertions(+), 16 deletions(-) create mode 100644 .changeset/flow-variable-condition-totality.md create mode 100644 test/flow-variable-conditions.test.ts diff --git a/.changeset/flow-variable-condition-totality.md b/.changeset/flow-variable-condition-totality.md new file mode 100644 index 00000000..b0264e1e --- /dev/null +++ b/.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. diff --git a/src/flows/campaign-enrollment.flow.ts b/src/flows/campaign-enrollment.flow.ts index 89bc4976..dcd9b206 100644 --- a/src/flows/campaign-enrollment.flow.ts +++ b/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' }, ], diff --git a/src/flows/contract-renewal.flow.ts b/src/flows/contract-renewal.flow.ts index 365dc1c2..1fe7419c 100644 --- a/src/flows/contract-renewal.flow.ts +++ b/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' }, ], diff --git a/src/flows/lead-conversion.flow.ts b/src/flows/lead-conversion.flow.ts index 7be4455f..c0d25411 100644 --- a/src/flows/lead-conversion.flow.ts +++ b/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' }, diff --git a/src/flows/opportunity-approval.flow.ts b/src/flows/opportunity-approval.flow.ts index 9ded4e16..2e66bcb8 100644 --- a/src/flows/opportunity-approval.flow.ts +++ b/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 > 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' }, diff --git a/src/flows/quote-generation.flow.ts b/src/flows/quote-generation.flow.ts index d4a72a2b..111b42b2 100644 --- a/src/flows/quote-generation.flow.ts +++ b/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' }, ], diff --git a/test/flow-condition-totality.test.ts b/test/flow-condition-totality.test.ts index 4036347d..7749de9a 100644 --- a/test/flow-condition-totality.test.ts +++ b/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') diff --git a/test/flow-variable-conditions.test.ts b/test/flow-variable-conditions.test.ts new file mode 100644 index 00000000..13030d25 --- /dev/null +++ b/test/flow-variable-conditions.test.ts @@ -0,0 +1,843 @@ +// 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 stack from '../objectstack.config'; + +/** + * ═══ HOUSE RULE: flow conditions over FLOW VARIABLES ═══════════════════════ + * + * `test/flow-condition-totality.test.ts` owns the **trigger record** scope + * (`record.x` / `previous.x`, #633). This file owns the other scope a flow + * condition can read: **flow-local variables** — `vars.x`, or the bare name of + * a `get_record` / `assignment` / loop output. Same evaluator, same abort, same + * consequence; a different source of the sparse shape, and — this is the part + * that must not be collapsed — **two classes with OPPOSITE remedies**: + * + * | class | remedy | + * | -------------------------------------- | ----------------------------- | + * | field read off a `get_record` OUTPUT | `has()` guards (as #633) | + * | the VARIABLE ITSELF may be unbound | bind it in the graph, NOT `has()` | + * + * A `has()` guard on an unbound variable would encode a policy ("a missing + * answer means No") inside a predicate, hiding the real defect: the graph left + * the variable unbound. So this file asserts the two properties separately — + * `has(...)` coverage for field reads, and a structural **binding** proof for + * variable roots — and its failure messages tell you which one you broke. + * + * ### What was measured on 17.0.0-rc.1, and how + * + * **Where conditions actually evaluate.** Three sites, not four: + * - the START node's `config.condition` (`AutomationEngine.execute`); + * - a `decision` node's `config.conditions[]` — the PLURAL, each entry an + * `{ label, expression }`; + * - every out-edge's `condition` (`AutomationEngine.traverseNext`). + * + * A `decision` node's SINGULAR `config.condition` — the shape every flow in + * this repo authors — is **never read by the engine**. The branch is decided + * entirely by the edge copies. That is why the reproductions below all abort on + * an EDGE, and why this file sweeps node and edge conditions alike: the node + * copy is inert today but it is the authored statement of intent, and it must + * not drift from the edge that actually decides. + * + * **How variables reach CEL.** `evaluateCondition` flattens the run's variable + * Map into one object and evaluates with `{ extra: { ...vars, vars }, record: vars }` + * — so a variable `X` is readable BOTH bare (`X.f`) and scoped (`vars.X.f`). + * They are not equally safe: + * + * | expression | X unbound | X = null | X = {} (sparse) | X = {f:1} | + * | ------------------- | --------------------------- | -------- | --------------- | --------- | + * | `X.f == 1` | ABORT `Unknown variable: X` | ABORT `No such key: f` | ABORT `No such key: f` | verdict | + * | `vars.X.f == 1` | ABORT `No such key: X` | ABORT `No such key: f` | ABORT `No such key: f` | verdict | + * | `vars.X != null` | ABORT `No such key: X` | `false` | `true` | `true` | + * | `has(X.f)` | **ABORT `Unknown variable: X`** | `false` | `false` | `true` | + * | `has(vars.X)` | `false` | `true` | `true` | `true` | + * | `has(vars.X.f)` | ABORT `No such key: X` | `false` | `false` | `true` | + * + * Two consequences, both load-bearing and both pinned below: + * - **Only `has(vars.X)` survives an unbound variable.** The bare `has(X.f)` + * form — the obvious one to reach for — still aborts. Guards in this repo + * are therefore written `vars.`-scoped, and always lead with `has(vars.X)` + * before `has(vars.X.f)`. + * - **No guard makes an unbound variable mean anything.** `has(vars.X)` + * answers `false`, which is a verdict, not a fix — see the class table + * above. + * + * **What binds a variable.** `get_record` runs + * `if (outputVariable) variables.set(outputVariable, record)` unconditionally, + * and `findOne` answers a miss with `null` (measured against `InMemoryDriver` + * below) — so after a `get_record`, the variable IS bound, possibly to `null`. + * `assignment` sets every key of `config.assignments`. A `loop` binds its + * `iteratorVariable` inside the body region, which runs in the ENCLOSING + * variable scope. A failed node throws and the run stops, so a node that could + * not bind its output never reaches a downstream reader. + * + * **What does NOT bind a variable — the trap.** Declaring it in + * `flow.variables` does nothing at runtime. `FlowVariableSchema` is strict + * `{ name, type, isInput, isOutput }` with **no `defaultValue`**, and + * `AutomationEngine.execute` binds a declared input only when + * `context.params[name] !== undefined`. A `screen` node's collected values + * arrive only in the RESUME SIGNAL, from the client. So a declared, + * screen-collected input is unbound on any run whose runner did not send it + * back — which is exactly how `lead_conversion` aborted. + * + * ### The defects this measured (both reproduced end-to-end below) + * + * - `campaign_enrollment` — CLASS 1. `vars.campaignRecord.status` on edge + * `e4`. A campaign deleted (or hidden by sharing) between the action click + * and the flow run leaves `campaignRecord` bound to `null`, and the read + * aborted with `No such key: status`. The run was recorded `failed` and not + * one lead was enrolled. `crm_campaign.status` is `required`, so the + * sparse-COLUMN variant was already closed — the null-RECORD variant was + * not, and no amount of reasoning from #633 would have found it. + * - `lead_conversion` — CLASS 2. `vars.createOpportunity` on edges `e16` / + * `e17`. A runner that posts only the fields the user touched leaves the + * untouched checkbox unbound; the read aborted with + * `No such key: createOpportunity` and the lead was never marked converted. + * Fixed by an `assignment` node ahead of the screen, NOT by a guard. + * + * ### What measured CLEAN, and why that is not the same as safe + * + * - `demo_bootstrap` (`vars.firstUser`) — `get_user` dominates every read and + * binds `null` when the org has no users yet; the whole flow completes on a + * zero-user org (reproduced below). Nothing to fix. + * - `lead_conversion`'s `vars.matchedAccount` / `vars.matchedContact` — same + * shape, same reason: a `get_record` dominates each read. Nothing to fix, + * and deliberately NOT guarded — a guard here would be the papering-over + * the class table warns about. + * - `quote_generation` (`oppRecord.stage`) and `opportunity_approval` + * (`oppRecord.amount`) — reachable only with a bound, non-null `oppRecord` + * TODAY, because two neighbouring schemas close the gap: `stage` / `amount` + * are `required` on `crm_opportunity`, and `crm_quote.crm_account` is + * `required` so a null `oppRecord` fails `create_quote` one node earlier. + * That is a property of the neighbours, not of the predicate, and it is one + * `required: false` away from changing. They carry guards too. + */ + +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 ''; +} + +// ═══════════════════════════════════════════════════════════════════════════ +// Extraction — every condition site, and every flow-variable read in it +// ═══════════════════════════════════════════════════════════════════════════ + +/** + * Scopes this file does NOT own. + * + * `record` / `previous` are the trigger record — `flow-condition-totality.test.ts` + * sweeps those. `$`-prefixed names are engine internals the engine always binds. + */ +const FOREIGN_ROOTS = new Set(['record', 'previous', 'vars', 'true', 'false', 'null']); + +/** Source with string literals blanked, so a literal's contents never parse as a read. */ +const withoutStrings = (source: string) => source.replace(/"[^"]*"|'[^']*'/g, '""'); + +/** + * Every `` a condition reads out of the flow-variable scope, whether it + * was written bare (`oppRecord.stage`) or scoped (`vars.oppRecord.stage`). + * Identifiers immediately followed by `(` are CEL functions (`has`, `size`, …), + * not variables. + */ +function variableRoots(source: string): string[] { + const bare = withoutStrings(source); + const roots = new Set(); + for (const [, root] of bare.matchAll(/\bvars\.([A-Za-z_]\w*)/g)) roots.add(root); + // `(?.` FIELD read in the flow-variable scope, both spellings. */ +function variableFieldReads(source: string): { root: string; field: string }[] { + const bare = withoutStrings(source); + const out = new Map(); + const add = (root: string, field: string) => { + if (FOREIGN_ROOTS.has(root) || root.startsWith('$')) return; + out.set(`${root}.${field}`, { root, field }); + }; + for (const [, root, field] of bare.matchAll(/\bvars\.([A-Za-z_]\w*)\.([A-Za-z_]\w*)/g)) add(root, field); + for (const [, root, field] of bare.matchAll(/(?.` — stable enough to name in a failure message. */ + id: string; + flow: string; + source: string; + /** Node the condition is attached to (a node site), or an edge's SOURCE node. */ + at: string; + /** Where the engine reads this condition from. `node:condition` is INERT — see the header. */ + kind: 'start' | 'node:condition' | 'node:conditions' | 'edge'; +} + +/** A flow's nodes and edges, with every `loop` body region flattened in. */ +function graphOf(flow: AnyRec): { nodes: AnyRec[]; edges: AnyRec[] } { + const nodes: AnyRec[] = []; + const edges: AnyRec[] = []; + const walk = (ns: AnyRec[], es: AnyRec[]) => { + for (const n of ns ?? []) { + nodes.push(n); + const body = n.config?.body; + if (body) walk(body.nodes ?? [], body.edges ?? []); + } + edges.push(...(es ?? [])); + }; + walk(flow.nodes ?? [], flow.edges ?? []); + return { nodes, edges }; +} + +/** Every condition site in every flow that reads at least one flow variable. */ +const sites: Site[] = flows.flatMap((f) => { + const { nodes, edges } = graphOf(f); + const out: Site[] = []; + const add = (where: string, at: string, kind: Site['kind'], condition: unknown) => { + const source = celSource(condition); + if (source) out.push({ id: `${f.name}.${where}`, flow: f.name as string, source, at, kind }); + }; + for (const n of nodes) { + add(`node:${n.id}`, n.id, n.type === 'start' ? 'start' : 'node:condition', n.config?.condition); + for (const c of (n.config?.conditions ?? []) as AnyRec[]) { + add(`node:${n.id}[${c?.label}]`, n.id, 'node:conditions', c?.expression); + } + } + for (const e of edges) add(`edge:${e.id}`, e.source, 'edge', e.condition); + return out; +}).filter((s) => variableRoots(s.source).length > 0); + +// ═══════════════════════════════════════════════════════════════════════════ +// CLASS 1 — field reads off a `get_record` output need has(...) guards +// ═══════════════════════════════════════════════════════════════════════════ + +describe('flow-variable 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(sites.length).toBeGreaterThanOrEqual(10); + expect(sites.some((s) => s.kind === 'edge')).toBe(true); + expect(sites.some((s) => s.kind === 'node:condition')).toBe(true); + expect(sites.some((s) => variableFieldReads(s.source).length > 0)).toBe(true); + }); + + it('leads every field read with has(vars.) and has(vars..)', () => { + const offenders = sites + .map((s) => ({ + id: s.id, + unguarded: variableFieldReads(s.source) + .filter(({ root, field }) => + !new RegExp(String.raw`has\(vars\.${root}\)`).test(s.source) || + !new RegExp(String.raw`has\(vars\.${root}\.${field}\)`).test(s.source)) + .map(({ root, field }) => `${root}.${field}`), + })) + .filter((s) => s.unguarded.length > 0); + + expect( + offenders, + 'These conditions read a FIELD off a flow variable without both guards. A ' + + '`get_record` output is a driver row: `findOne` answers a miss with null, and a ' + + 'sparse driver (driver-memory / driver-mongodb) omits any column the row was ' + + 'never written with. Strict CEL then aborts, the engine records the run FAILED ' + + 'and the automation does not happen. Write ' + + '`has(vars.X) && has(vars.X.f) && vars.X.f …`, or the opposite polarity ' + + '`!has(vars.X) || !has(vars.X.f) || …` where two branches must partition. Note ' + + 'the `vars.` scope is required: bare `has(X.f)` still aborts with ' + + '`Unknown variable: X` when X is unbound.', + ).toEqual([]); + }); + + it('null-guards every operand of every ordering comparison', () => { + // `has()` is not sufficient here: an explicit null PASSES has() and the + // comparison then aborts with `no such overload: dyn > int`. + const OPERAND = String.raw`(?:vars\.)?\w+(?:\.\w+)*|-?[\d.]+`; + const RELATIONAL = new RegExp(String.raw`(${OPERAND})\s*(?:<=|>=|<|>)\s*(${OPERAND})`, 'g'); + const offenders = sites + .map((s) => { + const operands = new Set(); + for (const [, lhs, rhs] of withoutStrings(s.source).matchAll(RELATIONAL)) { + for (const operand of [lhs, rhs]) { + const m = /^(?:vars\.)?([A-Za-z_]\w*)\.([A-Za-z_]\w*)$/.exec(operand); + if (m && !FOREIGN_ROOTS.has(m[1])) operands.add(`${m[1]}.${m[2]}`); + } + } + return { + id: s.id, + // Either polarity counts: `has(…) && … != null && … > n` is the + // "holds a value" shape, `!has(…) || … == null || … <= n` its + // complement, used where two branches must PARTITION. Both are total. + unguarded: [...operands].filter( + (operand) => + !new RegExp(String.raw`vars\.${operand.replace('.', String.raw`\.`)}\s*[!=]=\s*null`).test(s.source), + ), + }; + }) + .filter((s) => s.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([]); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// CLASS 2 — every variable a condition reads must be BOUND by the graph +// ═══════════════════════════════════════════════════════════════════════════ + +/** + * The variables a node binds for everything downstream of it. + * + * Deliberately NOT included: a `screen` node's collected fields. They arrive + * only in the resume signal, from the client, so the graph cannot guarantee + * them — which is the whole defect this check exists to catch. Also not + * included: `flow.variables` declarations, which bind nothing at runtime (see + * the header). + */ +function binds(node: AnyRec): string[] { + const cfg = node.config ?? {}; + const out: string[] = []; + if (typeof cfg.outputVariable === 'string' && cfg.outputVariable) out.push(cfg.outputVariable); + if (typeof cfg.iteratorVariable === 'string' && cfg.iteratorVariable) out.push(cfg.iteratorVariable); + if (typeof cfg.idVariable === 'string' && cfg.idVariable) out.push(cfg.idVariable); + const a = cfg.assignments; + if (Array.isArray(a)) { + for (const item of a) { + const name = item?.variable ?? item?.name ?? item?.key; + if (typeof name === 'string' && name) out.push(name); + } + } else if (a && typeof a === 'object') { + out.push(...Object.keys(a)); + } + return out; +} + +/** + * For every node, the variables guaranteed bound when the engine ENTERS it. + * + * Standard intersection dataflow over the flow DAG: a variable is guaranteed at + * `v` only if every path into `v` binds it first. A `loop` body region runs in + * the enclosing variable scope, entered from its container, so it is seeded + * with the container's exit set (plus the iterator). + */ +function boundOnEntry(flow: AnyRec): Map> { + const { nodes, edges } = graphOf(flow); + const universe = new Set(nodes.flatMap(binds)); + const byId = new Map(nodes.map((n) => [n.id as string, n])); + const incoming = new Map(); + for (const e of edges) { + if (!incoming.has(e.target)) incoming.set(e.target, []); + incoming.get(e.target)!.push(e); + } + + const entry = new Map>(); + /** Region entries: the container's exit set flows into the region's entry node. */ + const seeds = new Map Set>(); + for (const n of nodes) { + const body = n.config?.body; + if (!body) continue; + const bodyIds = new Set((body.nodes ?? []).map((b: AnyRec) => b.id as string)); + const bodyTargets = new Set((body.edges ?? []).map((b: AnyRec) => b.target as string)); + for (const id of bodyIds) { + if (!bodyTargets.has(id)) seeds.set(id, () => new Set([...(entry.get(n.id) ?? []), ...binds(n)])); + } + } + + for (const n of nodes) { + entry.set(n.id, n.type === 'start' ? new Set() : new Set(universe)); + } + for (let pass = 0; pass < nodes.length + 2; pass++) { + let changed = false; + for (const n of nodes) { + if (n.type === 'start') continue; + const preds = incoming.get(n.id) ?? []; + let next: Set; + if (seeds.has(n.id) && preds.length === 0) { + next = seeds.get(n.id)!(); + } else if (preds.length === 0) { + next = new Set(); // unreachable node — nothing is guaranteed + } else { + next = null as unknown as Set; + for (const e of preds) { + const u = byId.get(e.source); + const exit = new Set([...(entry.get(e.source) ?? []), ...(u ? binds(u) : [])]); + next = next === null ? exit : new Set([...next].filter((v) => exit.has(v))); + } + } + const prev = entry.get(n.id)!; + if (prev.size !== next.size || [...next].some((v) => !prev.has(v))) { + entry.set(n.id, next); + changed = true; + } + } + if (!changed) break; + } + return entry; +} + +/** The variables guaranteed bound where `site`'s condition is evaluated. */ +function guaranteedAt(flow: AnyRec, site: Site): Set { + const entry = boundOnEntry(flow); + const node = graphOf(flow).nodes.find((n) => n.id === site.at); + const base = new Set(entry.get(site.at) ?? []); + // An EDGE condition is evaluated after its source node has run, so the source + // node's own bindings count. A NODE condition is evaluated on entry. + if (site.kind === 'edge' && node) for (const v of binds(node)) base.add(v); + return base; +} + +describe('every variable a flow condition reads is BOUND on every path to it', () => { + const unbound = sites.flatMap((s) => { + const flow = flows.find((f) => f.name === s.flow)!; + const have = guaranteedAt(flow, s); + return variableRoots(s.source) + .filter((root) => !have.has(root)) + .map((root) => `${s.id}: reads \`${root}\`, which no node on every path to it binds`); + }); + + it('has no condition reading a variable the graph may leave unbound', () => { + expect( + unbound, + 'A flow condition reads a variable that some path reaching it never binds. On ' + + 'that path strict CEL aborts (`No such key: `, or `Unknown variable: ` ' + + 'for the bare spelling), the engine records the run FAILED and the automation ' + + 'does not happen.\n\n' + + 'Do NOT fix this with `has(...)`. A guard would bury a policy ("a missing ' + + 'answer means No") inside a predicate and leave the graph defect in place. Bind ' + + 'the variable instead — an `assignment` node upstream of every reader, or a ' + + '`get_record` whose `outputVariable` is that name. Declaring it in ' + + '`flow.variables` does NOT bind it: FlowVariableSchema has no `defaultValue` and ' + + 'the engine binds a declared input only when the caller passed it in ' + + '`context.params`.', + ).toEqual([]); + }); + + it('the analysis has teeth: an unbound read on a synthetic flow is caught', () => { + // Without this, a bug in `binds()` or the dataflow above would silently + // make the sweep vacuous — it would report "all bound" for everything. + const synthetic: AnyRec = { + name: 'synthetic', + type: 'screen', + variables: [{ name: 'answer', type: 'boolean', isInput: true, isOutput: false }], + nodes: [ + { id: 'start', type: 'start', label: 'Start', config: {} }, + { id: 'ask', type: 'screen', label: 'Ask', config: { fields: [{ name: 'answer', type: 'boolean' }] } }, + { id: 'yes', type: 'end', label: 'Yes' }, + ], + edges: [ + { id: 's1', source: 'start', target: 'ask', type: 'default' }, + { id: 's2', source: 'ask', target: 'yes', type: 'default', condition: { dialect: 'cel', source: 'vars.answer == true' } }, + ], + }; + const site: Site = { + id: 'synthetic.edge:s2', flow: 'synthetic', at: 'ask', kind: 'edge', + source: 'vars.answer == true', + }; + // Declared in `flow.variables` AND collected by a screen — and still not + // guaranteed, which is precisely the trap. + expect(guaranteedAt(synthetic, site).has('answer')).toBe(false); + + // …and an `assignment` ahead of the screen fixes it, which is the remedy + // `lead_conversion` now uses. + const fixed: AnyRec = { + ...synthetic, + nodes: [ + synthetic.nodes[0], + { id: 'init', type: 'assignment', label: 'Defaults', config: { assignments: { answer: false } } }, + ...synthetic.nodes.slice(1), + ], + edges: [ + { id: 's0', source: 'start', target: 'init', type: 'default' }, + { id: 's1', source: 'init', target: 'ask', type: 'default' }, + synthetic.edges[1], + ], + }; + expect(guaranteedAt(fixed, site).has('answer')).toBe(true); + }); + + it('binds `createOpportunity` before lead_conversion reads it', () => { + // The concrete defect, named: an `assignment` node ahead of the screen, not + // a `has()` guard on the edges. + const flow = flows.find((f) => f.name === 'lead_conversion')!; + const init = (flow.nodes as AnyRec[]).find((n) => n.type === 'assignment' && 'createOpportunity' in (n.config?.assignments ?? {})); + expect(init, 'lead_conversion no longer seeds createOpportunity').toBeDefined(); + expect(init!.config.assignments.createOpportunity).toBe(false); + for (const s of sites.filter((x) => x.flow === 'lead_conversion')) { + expect( + /has\(vars\.(createOpportunity|matchedAccount|matchedContact)\)/.test(s.source), + `${s.id} guards a flow VARIABLE with has(...) — bind it in the graph instead`, + ).toBe(false); + } + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// The same properties, measured on the real engine instead of grepped +// ═══════════════════════════════════════════════════════════════════════════ + +describe('flow-variable 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]; + } + } + 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))); + + it('is the reason the guards are needed: the unguarded forms abort', () => { + // The abort table from the header, re-measured on THIS evaluator rather + // than inherited from the record scope. If a platform upgrade makes strict + // CEL tolerant, this test fails and the guards can be revisited — that is + // the intended signal, not a nuisance. + expect(abortOf('vars.X.f == 1', { X: {} })).toMatch(/No such key: f/); + expect(abortOf('vars.X.f == 1', { X: null })).toMatch(/No such key: f/); + expect(abortOf('vars.X.f == 1', {})).toMatch(/No such key: X/); + expect(abortOf('X.f == 1', {})).toMatch(/Unknown variable: X/); + expect(abortOf('vars.X != null', {})).toMatch(/No such key: X/); + // A variable bound to `undefined` reads exactly like an unbound one. + expect(abortOf('vars.X != null', { X: undefined })).toMatch(/No such key: X/); + // has() passes an explicit null; the ordering comparison then aborts. + expect(abortOf('has(vars.X) && has(vars.X.f) && vars.X.f > 3', { X: { f: null } })) + .toMatch(/no such overload/); + expect(abortOf('has(vars.X) && has(vars.X.f) && vars.X.f != null && vars.X.f > 3', { X: { f: null } })) + .toBeNull(); + }); + + it('only the vars.-scoped guard survives an UNBOUND variable', () => { + // The single most important row of the table: the obvious bare spelling of + // the guard does not protect, so guards in `src/flows/` are `vars.`-scoped. + expect(abortOf('has(X.f)', {})).toMatch(/Unknown variable: X/); + expect(abortOf('has(vars.X)', {})).toBeNull(); + expect(ev('has(vars.X)', {})).toBe(false); + expect(ev('has(vars.X)', { X: null })).toBe(true); + expect(ev('has(vars.X)', { X: { f: 1 } })).toBe(true); + // …and `has(vars.X.f)` alone does NOT cover the unbound root. + expect(abortOf('has(vars.X.f)', {})).toMatch(/No such key: X/); + expect(ev('has(vars.X) && has(vars.X.f)', {})).toBe(false); + }); + + it.each(sites.map((s) => [s.id, s] as const))( + '%s answers on every shape a get_record output can take', + (_id, s) => { + const roots = variableRoots(s.source); + const fields = variableFieldReads(s.source); + /** Values a root can hold: unbound is modelled by omitting the key. */ + const shapes: AnyRec[] = [{}]; + for (const root of roots) { + const own = fields.filter((f) => f.root === root).map((f) => f.field); + const candidates: unknown[] = [null, {}, Object.fromEntries(own.map((f) => [f, null]))]; + // Literal-derived values keep the probes TYPE-correct: filling a field + // with an arbitrary value tests type agreement, not totality. + for (const own1 of own) { + // Values are drawn from the condition's OWN literals. Filling a field + // with an arbitrary value would not test totality, it would test type + // agreement: `amount = "x"` makes `amount > 500000` abort with + // `no such overload: dyn > int`, which is a mis-typed record + // no driver produces. Straddling each numeric literal probes both + // sides of an ordering test, which matters because CEL's `&&` + // ABSORBS an error beside a false operand. + const LITERAL = String.raw`"[^"]*"|-?\d+(?:\.\d+)?|true|false`; + const ref = String.raw`(?:vars\.)?${root}\.${own1}\b`; + const found = [ + ...s.source.matchAll(new RegExp(String.raw`${ref}\s*(?:==|!=|>=|<=|>|<)\s*(${LITERAL})`, 'g')), + ...s.source.matchAll(new RegExp(String.raw`(${LITERAL})\s*(?:==|!=|>=|<=|>|<)\s*${ref}`, 'g')), + ].map((m) => m[1]); + let sawString = false; + for (const raw of found) { + if (raw.startsWith('"')) { candidates.push({ [own1]: raw.slice(1, -1) }); sawString = true; continue; } + if (raw === 'true' || raw === 'false') { candidates.push({ [own1]: true }); candidates.push({ [own1]: false }); continue; } + const n = Number(raw); + candidates.push({ [own1]: n }, { [own1]: n - 1 }, { [own1]: n + 1 }); + } + // Only where the field is compared to a STRING — so `!=` chains are + // probed true as well, without mis-typing a numeric column. + if (sawString) candidates.push({ [own1]: '__neither__' }); + } + // Whole-variable candidates too, for `vars.X != null` style reads. + candidates.push({ id: 'x' }); + const next: AnyRec[] = []; + for (const base of shapes) for (const value of candidates) next.push({ ...base, [root]: value }); + shapes.splice(0, shapes.length, ...next.slice(0, 400)); + } + const bad: string[] = []; + for (const shape of shapes) { + const abort = abortOf(s.source, shape); + if (abort) { bad.push(`${JSON.stringify(shape)}: ${abort}`); break; } + } + expect(bad, `${s.id} aborted:\n ${bad.join('\n ')}`).toEqual([]); + }, + ); + + it('CEL && absorbs errors beside a false operand — why this went unnoticed', () => { + // The same mechanism `flow-condition-totality.test.ts` documents for the + // record scope, re-measured here: an unguarded condition answers fine for + // every run that fails some OTHER conjunct and aborts precisely on the runs + // the flow was written to act on. + expect(abortOf('vars.missing == 1 && vars.b == "no"', { b: 'x' })).toBeNull(); + expect(abortOf('vars.missing == 1 && vars.b == "x"', { b: 'x' })).toMatch(/No such key: missing/); + }); +}); + +// ═══════════════════════════════════════════════════════════════════════════ +// End-to-end, on the engine + driver that actually produce the shapes +// ═══════════════════════════════════════════════════════════════════════════ + +describe('the two defects, reproduced end-to-end', () => { + const objMap = Object.fromEntries(objects.map((o) => [o.name as string, o])); + + interface Booted { + ql: AnyRec; + engine: AnyRec; + 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. + ...({ 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); + // `approval` ships in `plugin-approvals` and is registered by the runtime, + // not by `installBuiltinNodes`. Stubbed to the `approve` branch so the run + // reaches the tier gate — the approval DECISION is not what is under test. + (engine as unknown as AnyRec).registerNodeExecutor({ + type: 'approval', + descriptor: { type: 'approval', version: '1.0.0', name: 'Approval (test stub)', category: 'approval', source: 'builtin' }, + async execute() { return { success: true, branchLabel: 'approve' }; }, + }); + + 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); + } + return { ql, engine: engine as unknown as AnyRec, close: () => ql.close() }; + } + + /** The invocation contract the console's flow-action trigger sends. */ + const asUser = (params: AnyRec) => ({ userId: 'u1', user: { id: 'u1' }, params }); + + it('findOne answers a miss with null — the precondition for class 1', async () => { + const b = await boot([]); + try { + const api = b.ql.createContext({ isSystem: true }); + const miss = await api.object('crm_account').findOne({ where: { name: '__nothing__' } }); + // Not `toBeFalsy()`: `null` specifically is what `get_record` then binds, + // and `vars.X.f` on a null X aborts with `No such key: f`. + expect(miss).toBeNull(); + // …while `crm_campaign.status` IS required, so the sparse-COLUMN variant + // was never the exposure here. If that changes this assertion fails and + // the guard's second clause starts earning its keep. + const camp = await api.object('crm_campaign').insert({ + name: 'Spring', type: 'email', status: 'planning', + start_date: '2026-01-01', end_date: '2026-03-01', + }); + const stored = await api.object('crm_campaign').findOne({ where: { id: camp.id } }); + expect('status' in (stored ?? {})).toBe(true); + } finally { + await b.close(); + } + }, 60_000); + + it('campaign_enrollment: a campaign that vanished still reaches a verdict', async () => { + const b = await boot(['campaign_enrollment']); + try { + const api = b.ql.createContext({ isSystem: true }); + const camp = await api.object('crm_campaign').insert({ + name: 'Spring', type: 'email', status: 'planning', + start_date: '2026-01-01', end_date: '2026-03-01', + }); + // Deleted (or hidden by sharing) between the action click and the run. + await api.object('crm_campaign').delete({ where: { id: camp.id } }); + + const started = await b.engine.execute('campaign_enrollment', asUser({ recordId: camp.id })); + expect(started.status).toBe('paused'); + const done = await b.engine.resume(started.runId, { variables: { leadStatus: 'new' } }); + // Before the guard: `condition failed to evaluate as CEL: No such key: status`. + expect(done.error ?? null).toBeNull(); + expect(done.success).not.toBe(false); + } finally { + await b.close(); + } + }, 60_000); + + it('campaign_enrollment: an OPEN campaign still enrols, so the guard did not close the door', async () => { + const b = await boot(['campaign_enrollment']); + try { + const api = b.ql.createContext({ isSystem: true }); + const camp = await api.object('crm_campaign').insert({ + name: 'Spring', type: 'email', status: 'planning', + start_date: '2026-01-01', end_date: '2026-03-01', + }); + await api.object('crm_lead').insert({ + first_name: 'Jo', last_name: 'Smith', company: 'Acme', + status: 'new', email: 'jo@acme.com', email_opt_out: false, is_converted: false, + }); + const started = await b.engine.execute('campaign_enrollment', asUser({ recordId: camp.id })); + const done = await b.engine.resume(started.runId, { variables: { leadStatus: 'new' } }); + expect(done.error ?? null).toBeNull(); + const members = await api.object('crm_campaign_member').find({ where: { crm_campaign: camp.id } }); + expect(members.length, 'the guard suppressed a legitimate enrolment').toBe(1); + } finally { + await b.close(); + } + }, 60_000); + + it('lead_conversion: a resume signal that omits the untouched checkbox still converts', async () => { + const b = await boot(['lead_conversion']); + try { + const api = b.ql.createContext({ isSystem: true }); + const lead = await api.object('crm_lead').insert({ + first_name: 'Sam', last_name: 'Doe', company: 'Globex', + status: 'qualified', email: 'sam@globex.com', + }); + const started = await b.engine.execute('lead_conversion', asUser({ recordId: lead.id })); + expect(started.status).toBe('paused'); + // The runner posts only what the user touched — the checkbox was left + // alone, so `createOpportunity` is absent from the signal. + const done = await b.engine.resume(started.runId, { variables: {} }); + // Before the binding: `condition failed to evaluate as CEL: No such key: createOpportunity`. + expect(done.error ?? null).toBeNull(); + const stored = await api.object('crm_lead').findOne({ where: { id: lead.id } }); + expect(stored?.is_converted, 'the lead was never marked converted').toBe(true); + // …and the default really is "no opportunity", matching the screen field. + expect(stored?.converted_opportunity ?? null).toBeNull(); + } finally { + await b.close(); + } + }, 60_000); + + it('lead_conversion: an explicit yes still creates the opportunity', async () => { + const b = await boot(['lead_conversion']); + try { + const api = b.ql.createContext({ isSystem: true }); + const lead = await api.object('crm_lead').insert({ + first_name: 'Jo', last_name: 'Smith', company: 'Acme', + status: 'qualified', email: 'jo@acme.com', + }); + const started = await b.engine.execute('lead_conversion', asUser({ recordId: lead.id })); + const done = await b.engine.resume(started.runId, { + variables: { createOpportunity: true, opportunityName: 'Acme Deal', opportunityAmount: 50_000 }, + }); + expect(done.error ?? null).toBeNull(); + const opp = await api.object('crm_opportunity').findOne({ where: { name: 'Acme Deal' } }); + expect(opp, 'the seeded default overrode the user answer').toBeTruthy(); + } finally { + await b.close(); + } + }, 60_000); + + it('demo_bootstrap: a zero-user org completes instead of aborting', async () => { + // The `vars.firstUser` reads measured CLEAN — `get_user` dominates them and + // binds `null`. This pins that, so a future edit that moves the read above + // the `get_record` is caught here rather than on a demo org. + const b = await boot(['demo_bootstrap']); + try { + const done = await b.engine.execute('demo_bootstrap', {}); + expect(done.error ?? null).toBeNull(); + expect(done.success).not.toBe(false); + } finally { + await b.close(); + } + }, 60_000); + + it('quote_generation: an ordinary run still advances the stage', async () => { + const b = await boot(['quote_generation']); + try { + const api = b.ql.createContext({ isSystem: true }); + 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: 'prospecting', + close_date: '2026-12-31', crm_account: account.id, + }); + const started = await b.engine.execute('quote_generation', asUser({ recordId: opp.id })); + const done = await b.engine.resume(started.runId, { + variables: { quoteName: 'Q-1', expirationDays: 30, discount: 0 }, + }); + expect(done.error ?? null).toBeNull(); + const stored = await api.object('crm_opportunity').findOne({ where: { id: opp.id } }); + expect(stored?.stage, 'the guard suppressed a legitimate stage advance').toBe('proposal'); + } finally { + await b.close(); + } + }, 60_000); + + it('opportunity_approval: both tiers still route correctly through the guarded gate', async () => { + const b = await boot(['opportunity_approval_on_create']); + try { + const api = b.ql.createContext({ isSystem: true }); + const account = await api.object('crm_account').insert({ name: 'Acme Corp' }); + const run = async (amount: number) => { + const opp = await api.object('crm_opportunity').insert({ + name: `Deal ${amount}`, amount, stage: 'negotiation', + close_date: '2026-12-31', crm_account: account.id, + }); + const fresh = await api.object('crm_opportunity').findOne({ where: { id: opp.id } }); + const res = await b.engine.execute('opportunity_approval_on_create', { + record: fresh, previous: null, userId: 'u1', user: { id: 'u1' }, params: {}, + }); + return { res, id: opp.id }; + }; + for (const amount of [750_000, 200_000]) { + const { res } = await run(amount); + // Deliberately narrower than "the run succeeded": this flow also fails + // downstream in a user-less harness (`notify` addressed to + // `{oppRecord.owner}`, which the owner default cannot fill without a + // user). That is a real but SEPARATE concern; what is asserted here is + // that the tier gate reached a verdict. + expect( + /failed to evaluate as CEL|No such key|no such overload|Unknown variable/.test(res.error ?? ''), + `tier gate aborted at ${amount}: ${res.error}`, + ).toBe(false); + expect(res.output?.skipped ?? false, `flow did not fire at ${amount}`).toBe(false); + } + } finally { + await b.close(); + } + }, 60_000); +});