From 44e0f5a004beec3968b65fa778d017a938dbb7c7 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 03:47:35 +0000 Subject: [PATCH] fix(flows): author conditions as CEL envelopes so they actually evaluate Every decision node and conditional edge in src/flows/ wrote its condition as a bare string. AutomationEngine.evaluateCondition only routes an Expression envelope ({ dialect, source }) to the CEL engine; a bare string falls through to a legacy template path that substitutes {var} braces and then compares both sides as strings. No condition in the app was ever evaluated as an expression. The failure mode was not a uniform no-op, which is what made it dangerous: existingStallTask == null -> 'existingStallTask' === 'null' -> always false record.rating >= 4 -> 'record.rating' >= '4' -> always TRUE so opportunity_stagnation dropped every stalled deal at its idempotency gate while reporting a successful run, and lead_assignment pinned the Hot branch open and never took the Standard path. All 41 condition sites now use the P tagged template from @objectstack/spec, which emits the envelope at authoring time. The sources are unchanged: they were already valid CEL, because the engine merges its variable map onto the CEL scope via ctx.extra, so flow variables resolve by bare name and `record` is the triggering record. Only the envelope was missing. defineFlow() would not have sufficed: it normalizes the typed edge condition, but a node's config is z.record(z.unknown()), so every start-node trigger gate would have stayed a bare string. Verified on a booted server with a temporary 1-minute cron: the stagnation sweep now creates 30 follow-up tasks, one per stalled deal, and holds at 30 across later ticks as the idempotency gate closes. It created zero before. A guard in test/metadata-references.test.ts fails on any condition that is not a CEL envelope, at either site, so the bare form cannot come back silently. The two case-escalation assertions that read the condition as a string now read the envelope's source. Refs #562 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FJygXUafjc7ckGMmE2AaGz --- .changeset/flow-conditions-cel-envelopes.md | 33 ++++++++++++ src/flows/campaign-enrollment.flow.ts | 9 ++-- src/flows/case-csat-followup.flow.ts | 3 +- src/flows/case-escalation.flow.ts | 6 +-- src/flows/contact-welcome.flow.ts | 3 +- src/flows/contract-renewal.flow.ts | 17 +++--- src/flows/demo-bootstrap.flow.ts | 7 +-- src/flows/lead-assignment.flow.ts | 7 +-- src/flows/lead-conversion.flow.ts | 19 +++---- src/flows/opportunity-approval.flow.ts | 12 ++--- src/flows/opportunity-stagnation.flow.ts | 5 +- src/flows/opportunity-won-alert.flow.ts | 3 +- src/flows/quote-generation.flow.ts | 7 +-- src/flows/task-urgent-alert.flow.ts | 3 +- test/actions-flows-integrity.test.ts | 4 +- test/metadata-references.test.ts | 60 +++++++++++++++++++++ 16 files changed, 152 insertions(+), 46 deletions(-) create mode 100644 .changeset/flow-conditions-cel-envelopes.md diff --git a/.changeset/flow-conditions-cel-envelopes.md b/.changeset/flow-conditions-cel-envelopes.md new file mode 100644 index 00000000..8c145aa0 --- /dev/null +++ b/.changeset/flow-conditions-cel-envelopes.md @@ -0,0 +1,33 @@ +--- +'hotcrm': patch +--- + +Make flow conditions actually evaluate (#562). Every `decision` node and +conditional edge in `src/flows/` authored its condition as a bare string, and +`AutomationEngine.evaluateCondition` only routes an `Expression` envelope +(`{ dialect, source }`) to the CEL engine. A bare string fell through to a +legacy template path that substitutes `{var}` braces and then compares both +sides as strings — so no condition in the app was ever evaluated as an +expression. + +The failure mode was not a uniform no-op, which is what made it dangerous. +`existingStallTask == null` compared `'existingStallTask'` to `'null'` and was +always false, so `opportunity_stagnation` selected the right stalled deals (once +#489 was fixed) and then silently dropped every one of them at the idempotency +gate — no notification, no follow-up task, and a `success` run record either +way. In the other direction `record.rating >= 4` compared `'record.rating'` to +`'4'`, and `'r' > '4'` is true, so `lead_assignment` pinned the Hot branch open +and never took the Standard path. + +All 41 condition sites are now authored with the `P` tagged template from +`@objectstack/spec`, which emits the envelope at authoring time. The condition +sources are unchanged: they were already valid CEL — flow variables resolve by +bare name and `record` is the triggering record, because the engine merges its +variable map onto the CEL scope via `ctx.extra`. Only the envelope was missing. + +Note that `defineFlow()` would not have been enough on its own: it normalizes +the typed edge `condition`, but a node's `config` is `z.record(z.unknown())`, so +every start-node trigger gate would have stayed a bare string. + +A guard in `test/metadata-references.test.ts` fails on any condition that is not +a CEL envelope, at either site, so the bare form cannot come back silently. diff --git a/src/flows/campaign-enrollment.flow.ts b/src/flows/campaign-enrollment.flow.ts index 8a0c03e2..83790b11 100644 --- a/src/flows/campaign-enrollment.flow.ts +++ b/src/flows/campaign-enrollment.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -64,7 +65,7 @@ export const CampaignEnrollmentFlow: Flow = { // topping up a completed/aborted campaign would corrupt its final // snapshot metrics. (Status values: planning / in_progress / completed / aborted.) id: 'check_campaign_open', type: 'decision', label: 'Campaign Open?', - config: { condition: 'campaignRecord.status == "planning" || campaignRecord.status == "in_progress"' }, + config: { condition: P`campaignRecord.status == "planning" || campaignRecord.status == "in_progress"` }, }, { id: 'query_leads', type: 'get_record', label: 'Find Eligible Leads', @@ -101,7 +102,7 @@ export const CampaignEnrollmentFlow: Flow = { }, { id: 'check_not_enrolled', type: 'decision', label: 'New Member?', - config: { condition: 'existingMember == null' }, + config: { condition: P`existingMember == null` }, }, { id: 'create_campaign_member', type: 'create_record', label: 'Add to Campaign', @@ -114,7 +115,7 @@ export const CampaignEnrollmentFlow: Flow = { edges: [ { id: 'b1', source: 'find_existing_member', target: 'check_not_enrolled', type: 'default' }, // Already enrolled → no edge → next lead. - { id: 'b2', source: 'check_not_enrolled', target: 'create_campaign_member', type: 'conditional', condition: 'existingMember == null', label: 'Enroll' }, + { id: 'b2', source: 'check_not_enrolled', target: 'create_campaign_member', type: 'conditional', condition: P`existingMember == null`, label: 'Enroll' }, ], }, }, @@ -127,7 +128,7 @@ 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: 'campaignRecord.status == "planning" || campaignRecord.status == "in_progress"', label: 'Open' }, + { id: 'e4', source: 'check_campaign_open', target: 'query_leads', type: 'conditional', condition: P`campaignRecord.status == "planning" || 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/case-csat-followup.flow.ts b/src/flows/case-csat-followup.flow.ts index 9f9b8e9a..9c037504 100644 --- a/src/flows/case-csat-followup.flow.ts +++ b/src/flows/case-csat-followup.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -34,7 +35,7 @@ export const CaseCsatFollowupFlow: Flow = { config: { objectName: 'crm_case', triggerType: 'record-after-update', - condition: 'record.status == "closed" && (previous == null || previous.status != "closed")', + condition: P`record.status == "closed" && (previous == null || previous.status != "closed")`, }, }, { diff --git a/src/flows/case-escalation.flow.ts b/src/flows/case-escalation.flow.ts index e09452cf..d4b11a3c 100644 --- a/src/flows/case-escalation.flow.ts +++ b/src/flows/case-escalation.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -42,9 +43,8 @@ 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: - 'record.priority == "critical" && record.escalated_date == null' - + ' && record.status != "escalated" && record.status != "resolved" && record.status != "closed"', + condition: P`record.priority == "critical" && record.escalated_date == null + && record.status != "escalated" && record.status != "resolved" && record.status != "closed"`, }, }, { diff --git a/src/flows/contact-welcome.flow.ts b/src/flows/contact-welcome.flow.ts index d75e85d2..0b6bc72f 100644 --- a/src/flows/contact-welcome.flow.ts +++ b/src/flows/contact-welcome.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -31,7 +32,7 @@ export const ContactWelcomeFlow: Flow = { config: { objectName: 'crm_contact', triggerType: 'record-after-create', - condition: 'record.owner != null && record.email_opt_out != true', + condition: P`record.owner != null && record.email_opt_out != true`, }, }, { diff --git a/src/flows/contract-renewal.flow.ts b/src/flows/contract-renewal.flow.ts index aaca1758..b4948616 100644 --- a/src/flows/contract-renewal.flow.ts +++ b/src/flows/contract-renewal.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -58,7 +59,7 @@ export const ContractRenewalFlow: Flow = { nodes: [ { id: 'check_notice_window', type: 'decision', label: 'Within Notice Window?', - config: { condition: 'timestamp(currentContract.end_date) <= daysFromNow(int(currentContract.renewal_notice_days))' }, + config: { condition: P`timestamp(currentContract.end_date) <= daysFromNow(int(currentContract.renewal_notice_days))` }, }, { // Idempotency gate: the sweep matches the same contract every @@ -79,7 +80,7 @@ export const ContractRenewalFlow: Flow = { }, { id: 'check_not_reminded', type: 'decision', label: 'First Reminder?', - config: { condition: 'existingRenewalTask == null' }, + config: { condition: P`existingRenewalTask == null` }, }, { id: 'create_renewal_task', type: 'create_record', label: 'Create Renewal Task', @@ -108,7 +109,7 @@ export const ContractRenewalFlow: Flow = { }, { id: 'check_auto_renewal', type: 'decision', label: 'Auto-Renewal On?', - config: { condition: 'currentContract.auto_renewal == true' }, + config: { condition: P`currentContract.auto_renewal == true` }, }, { // Second gate: never open a second renewal opportunity while one @@ -127,7 +128,7 @@ export const ContractRenewalFlow: Flow = { }, { id: 'check_no_open_renewal', type: 'decision', label: 'No Open Renewal Deal?', - config: { condition: 'existingRenewalOpp == null' }, + config: { condition: P`existingRenewalOpp == null` }, }, { id: 'create_renewal_opp', type: 'create_record', label: 'Open Renewal Opportunity', @@ -149,14 +150,14 @@ 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: 'timestamp(currentContract.end_date) <= daysFromNow(int(currentContract.renewal_notice_days))', label: 'In window' }, + { id: 'b1', source: 'check_notice_window', target: 'find_existing_task', type: 'conditional', condition: P`timestamp(currentContract.end_date) <= daysFromNow(int(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: 'existingRenewalTask == null', label: 'First reminder' }, + { 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: 'currentContract.auto_renewal == true', label: 'Auto-renew' }, + { id: 'b6', source: 'check_auto_renewal', target: 'find_existing_renewal_opp', type: 'conditional', condition: P`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: 'existingRenewalOpp == null', label: 'Open renewal deal' }, + { 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/demo-bootstrap.flow.ts b/src/flows/demo-bootstrap.flow.ts index 50044f58..8c528c45 100644 --- a/src/flows/demo-bootstrap.flow.ts +++ b/src/flows/demo-bootstrap.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -120,7 +121,7 @@ export const DemoBootstrapFlow: Flow = { }, { id: 'has_user', type: 'decision', label: 'Any user yet?', - config: { condition: 'vars.firstUser != null' }, + config: { condition: P`vars.firstUser != null` }, }, ...TARGETS.flatMap((t) => [t.find, t.loop]), { id: 'end', type: 'end', label: 'End' }, @@ -134,12 +135,12 @@ export const DemoBootstrapFlow: Flow = { target: CHAIN[i + 1], type: 'default' as const, // The only branch: leaving `has_user` towards the first claim step. - ...(source === 'has_user' ? { condition: 'vars.firstUser != null', label: 'Yes' } : {}), + ...(source === 'has_user' ? { condition: P`vars.firstUser != null`, label: 'Yes' } : {}), })), // Nothing to claim before anyone exists — skip the whole chain. { id: 'e_nouser', source: 'has_user', target: 'end', type: 'default', - condition: 'vars.firstUser == null', label: 'No user yet', + condition: P`vars.firstUser == null`, label: 'No user yet', }, ], }; diff --git a/src/flows/lead-assignment.flow.ts b/src/flows/lead-assignment.flow.ts index 6fc9d179..baba9c9a 100644 --- a/src/flows/lead-assignment.flow.ts +++ b/src/flows/lead-assignment.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -40,7 +41,7 @@ export const LeadAssignmentFlow: Flow = { }, { id: 'check_hot', type: 'decision', label: 'Hot Lead (rating ≥ 4)?', - config: { condition: 'record.rating >= 4' }, + config: { condition: P`record.rating >= 4` }, }, // ── Hot path: 1-day SLA, high-severity alert ─────────────────── @@ -91,8 +92,8 @@ 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: 'record.rating >= 4', label: 'Hot' }, - { id: 'e3', source: 'check_hot', target: 'sla_std', type: 'conditional', condition: 'record.rating < 4', label: 'Standard' }, + { 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' }, { id: 'e4', source: 'sla_hot', target: 'notify_hot', type: 'default' }, { id: 'e5', source: 'notify_hot', target: 'end', type: 'default' }, { id: 'e6', source: 'sla_std', target: 'notify_std', type: 'default' }, diff --git a/src/flows/lead-conversion.flow.ts b/src/flows/lead-conversion.flow.ts index 20a75aac..42035937 100644 --- a/src/flows/lead-conversion.flow.ts +++ b/src/flows/lead-conversion.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -56,7 +57,7 @@ export const LeadConversionFlow: Flow = { }, { id: 'decision_account', type: 'decision', label: 'Account Already Exists?', - config: { condition: 'vars.matchedAccount != null' }, + config: { condition: P`vars.matchedAccount != null` }, }, { // NEW-account branch. outputVariable is `createdAccount`; the assignment @@ -100,7 +101,7 @@ export const LeadConversionFlow: Flow = { }, { id: 'decision_contact', type: 'decision', label: 'Contact Already Exists?', - config: { condition: 'vars.matchedContact != null' }, + config: { condition: P`vars.matchedContact != null` }, }, { // `accountId` is a bare id string from whichever account branch ran. @@ -126,7 +127,7 @@ export const LeadConversionFlow: Flow = { }, { id: 'decision_opportunity', type: 'decision', label: 'Create Opportunity?', - config: { condition: 'vars.createOpportunity == true' }, + config: { condition: P`vars.createOpportunity == true` }, }, { id: 'create_opportunity', type: 'create_record', label: 'Create Opportunity', @@ -189,21 +190,21 @@ export const LeadConversionFlow: Flow = { { id: 'e3', source: 'get_lead', target: 'find_account', type: 'default' }, { id: 'e4', source: 'find_account', target: 'decision_account', type: 'default' }, // Existing account → reuse; no account → create. Both converge on create_contact. - { id: 'e5', source: 'decision_account', target: 'use_existing_account', type: 'default', condition: 'vars.matchedAccount != null', label: 'Existing' }, - { id: 'e6', source: 'decision_account', target: 'create_account', type: 'default', condition: 'vars.matchedAccount == null', label: 'New' }, + { id: 'e5', source: 'decision_account', target: 'use_existing_account', type: 'default', condition: P`vars.matchedAccount != null`, label: 'Existing' }, + { id: 'e6', source: 'decision_account', target: 'create_account', type: 'default', condition: P`vars.matchedAccount == null`, label: 'New' }, { id: 'e7', source: 'create_account', target: 'use_new_account', type: 'default' }, // Both account branches converge on the contact-dedupe lookup. { id: 'e8', source: 'use_new_account', target: 'find_contact', type: 'default' }, { id: 'e9', source: 'use_existing_account', target: 'find_contact', type: 'default' }, { id: 'e10', source: 'find_contact', target: 'decision_contact', type: 'default' }, // Existing contact → reuse; none → create. Both converge on decision_opportunity. - { id: 'e11', source: 'decision_contact', target: 'use_existing_contact', type: 'default', condition: 'vars.matchedContact != null', label: 'Existing' }, - { id: 'e12', source: 'decision_contact', target: 'create_contact', type: 'default', condition: 'vars.matchedContact == null', label: 'New' }, + { id: 'e11', source: 'decision_contact', target: 'use_existing_contact', type: 'default', condition: P`vars.matchedContact != null`, label: 'Existing' }, + { id: 'e12', source: 'decision_contact', target: 'create_contact', type: 'default', condition: P`vars.matchedContact == null`, label: 'New' }, { id: 'e13', source: 'create_contact', target: 'use_new_contact', type: 'default' }, { id: 'e14', source: 'use_new_contact', target: 'decision_opportunity', type: 'default' }, { id: 'e15', source: 'use_existing_contact', target: 'decision_opportunity', type: 'default' }, - { id: 'e16', source: 'decision_opportunity', target: 'create_opportunity', type: 'default', condition: 'vars.createOpportunity == true', label: 'Yes' }, - { id: 'e17', source: 'decision_opportunity', target: 'no_opportunity', type: 'default', condition: 'vars.createOpportunity != true', label: 'No' }, + { id: 'e16', source: 'decision_opportunity', target: 'create_opportunity', type: 'default', condition: P`vars.createOpportunity == true`, label: 'Yes' }, + { id: 'e17', source: 'decision_opportunity', target: 'no_opportunity', type: 'default', condition: P`vars.createOpportunity != true`, label: 'No' }, { id: 'e18', source: 'create_opportunity', target: 'use_new_opportunity', type: 'default' }, { id: 'e18a', source: 'use_new_opportunity', target: 'mark_converted', type: 'default' }, { id: 'e18b', source: 'no_opportunity', target: 'mark_converted', type: 'default' }, diff --git a/src/flows/opportunity-approval.flow.ts b/src/flows/opportunity-approval.flow.ts index 81bbc4ef..1f576e97 100644 --- a/src/flows/opportunity-approval.flow.ts +++ b/src/flows/opportunity-approval.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -57,9 +58,8 @@ 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: - 'record.amount > 100000 && (record.approval_status == "not_required" || record.approval_status == null)' - + ' && record.stage != "closed_won" && record.stage != "closed_lost"', + condition: P`record.amount > 100000 && (record.approval_status == "not_required" || record.approval_status == null) + && record.stage != "closed_won" && record.stage != "closed_lost"`, }, }, { @@ -95,7 +95,7 @@ export const OpportunityApprovalFlow: Flow = { id: 'check_high_value', type: 'decision', label: 'High Value (> $500K)?', - config: { condition: 'oppRecord.amount > 500000' }, + config: { condition: P`oppRecord.amount > 500000` }, }, // ── Tier 2: Sales Director sign-off (deals > $500K only) ──────── @@ -178,8 +178,8 @@ export const OpportunityApprovalFlow: Flow = { { 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: 'oppRecord.amount > 500000', label: 'High value (> $500K)' }, - { id: 'e6', source: 'check_high_value', target: 'mark_approved', type: 'conditional', condition: 'oppRecord.amount <= 500000', label: 'Standard (≤ $500K)' }, + { 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)' }, // Director decision (approval-node branch labels) { id: 'e7', source: 'director_signoff', target: 'mark_approved', type: 'default', label: 'approve' }, diff --git a/src/flows/opportunity-stagnation.flow.ts b/src/flows/opportunity-stagnation.flow.ts index 76e6187a..ddf93042 100644 --- a/src/flows/opportunity-stagnation.flow.ts +++ b/src/flows/opportunity-stagnation.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -79,7 +80,7 @@ export const OpportunityStagnationFlow: Flow = { }, { id: 'check_not_nudged', type: 'decision', label: 'First Nudge?', - config: { condition: 'existingStallTask == null' }, + config: { condition: P`existingStallTask == null` }, }, { // Owner only: `{currentOpp.owner.manager}` cannot traverse a @@ -113,7 +114,7 @@ export const OpportunityStagnationFlow: Flow = { edges: [ { id: 'b1', source: 'find_existing_task', target: 'check_not_nudged', type: 'default' }, // "Already nudged" has no edge, so the loop moves to the next item. - { id: 'b2', source: 'check_not_nudged', target: 'notify_owner', type: 'conditional', condition: 'existingStallTask == null', label: 'First nudge' }, + { id: 'b2', source: 'check_not_nudged', target: 'notify_owner', type: 'conditional', condition: P`existingStallTask == null`, label: 'First nudge' }, { id: 'b3', source: 'notify_owner', target: 'create_followup_task', type: 'default' }, ], }, diff --git a/src/flows/opportunity-won-alert.flow.ts b/src/flows/opportunity-won-alert.flow.ts index 59964acc..992a5eb2 100644 --- a/src/flows/opportunity-won-alert.flow.ts +++ b/src/flows/opportunity-won-alert.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -32,7 +33,7 @@ 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: 'record.stage == "closed_won" && previous.stage != "closed_won" && record.amount > 100000', + condition: P`record.stage == "closed_won" && previous.stage != "closed_won" && record.amount > 100000`, }, }, { diff --git a/src/flows/quote-generation.flow.ts b/src/flows/quote-generation.flow.ts index 0b009af5..ede73305 100644 --- a/src/flows/quote-generation.flow.ts +++ b/src/flows/quote-generation.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -60,7 +61,7 @@ export const QuoteGenerationFlow: Flow = { // a deal already at proposal/negotiation was an illegal self/backward // transition — those deals keep their stage; the quote is still created. id: 'check_stage', type: 'decision', label: 'Can Advance to Proposal?', - config: { condition: 'oppRecord.stage == "prospecting" || oppRecord.stage == "qualification" || oppRecord.stage == "needs_analysis"' }, + config: { condition: P`oppRecord.stage == "prospecting" || oppRecord.stage == "qualification" || oppRecord.stage == "needs_analysis"` }, }, { id: 'update_opportunity', type: 'update_record', label: 'Update Opportunity', @@ -92,8 +93,8 @@ 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: 'oppRecord.stage == "prospecting" || oppRecord.stage == "qualification" || oppRecord.stage == "needs_analysis"', label: 'Advance' }, - { id: 'e4b', source: 'check_stage', target: 'notify_owner', type: 'conditional', condition: 'oppRecord.stage != "prospecting" && oppRecord.stage != "qualification" && oppRecord.stage != "needs_analysis"', label: 'Keep stage' }, + { 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' }, { id: 'e5', source: 'update_opportunity', target: 'notify_owner', type: 'default' }, { id: 'e6', source: 'notify_owner', target: 'end', type: 'default' }, ], diff --git a/src/flows/task-urgent-alert.flow.ts b/src/flows/task-urgent-alert.flow.ts index cd7a8b5f..c92e32b2 100644 --- a/src/flows/task-urgent-alert.flow.ts +++ b/src/flows/task-urgent-alert.flow.ts @@ -1,5 +1,6 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. +import { P } from '@objectstack/spec'; import type * as Automation from '@objectstack/spec/automation'; type Flow = Automation.Flow; @@ -27,7 +28,7 @@ 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: 'record.priority == "urgent" && record.status != "completed"', + condition: P`record.priority == "urgent" && record.status != "completed"`, }, }, { diff --git a/test/actions-flows-integrity.test.ts b/test/actions-flows-integrity.test.ts index 8dc03bf8..f39ba433 100644 --- a/test/actions-flows-integrity.test.ts +++ b/test/actions-flows-integrity.test.ts @@ -354,7 +354,9 @@ describe('case escalation trigger does not fight the close action', () => { // (an afterUpdate) re-triggered this flow, which rewrote the case back // to "escalated" the moment close_case closed it — observed live. const start = nodeOf(flow(name), 'start'); - const condition: string = start?.config?.condition ?? ''; + // Conditions are `Expression` envelopes ({ dialect, source }), not bare + // strings — see the flow-condition guard in metadata-references.test.ts. + const condition: string = start?.config?.condition?.source ?? ''; expect(condition).toContain('record.status != "escalated"'); expect(condition).toContain('record.status != "closed"'); expect(condition).toContain('record.status != "resolved"'); diff --git a/test/metadata-references.test.ts b/test/metadata-references.test.ts index 4cc899d4..cf76b57a 100644 --- a/test/metadata-references.test.ts +++ b/test/metadata-references.test.ts @@ -310,6 +310,66 @@ describe('formula fields are never used as query predicates', () => { }); }); +describe('flow conditions reach the CEL engine', () => { + /** + * `AutomationEngine.evaluateCondition` only routes an `Expression` envelope + * (`{ dialect, source }`) to the CEL engine. A bare string falls through to a + * legacy template path that substitutes `{var}` braces and then compares both + * sides as STRINGS — so `existingStallTask == null` evaluates + * `'existingStallTask' === 'null'` (always false) and `record.rating >= 4` + * evaluates `'record.rating' >= '4'` (always TRUE, because `'r' > '4'`). + * + * Neither errors. The first silently closes an idempotency gate forever; the + * second silently pins a branch open. That is #562, and it went unnoticed + * because the sweep reports success either way. + * + * Author conditions with the `P` tagged template from `@objectstack/spec`. + * Note that `defineFlow()` alone is NOT sufficient: it normalizes the typed + * edge `condition`, but a node's `config` is `z.record(z.unknown())`, so a + * start-node trigger gate would stay a bare string. + */ + const conditionSites = (flow: AnyRec): { where: string; value: unknown }[] => { + const sites: { where: string; value: unknown }[] = []; + for (const rec of walk(flow)) { + // Edges carry `condition` directly; nodes carry it inside `config`. + const at = rec.id ? `"${rec.id}"` : `<${rec.type}>`; + if ('condition' in rec && rec.condition !== undefined) { + sites.push({ where: `flow "${flow.name}" edge/node ${at}`, value: rec.condition }); + } + if (rec.config && typeof rec.config === 'object' && rec.config.condition !== undefined) { + sites.push({ where: `flow "${flow.name}" node ${at} config`, value: rec.config.condition }); + } + } + return sites; + }; + + it('no flow condition is a bare string', () => { + const flows: AnyRec[] = (stack as any).flows ?? []; + const bad: string[] = []; + let seen = 0; + + for (const flow of flows) { + for (const { where, value } of conditionSites(flow)) { + seen++; + if (typeof value === 'string') { + bad.push(`${where}: bare string \`${value}\` — wrap in P\`…\``); + continue; + } + const env = value as AnyRec; + if (env?.dialect !== 'cel') bad.push(`${where}: dialect "${env?.dialect}", expected "cel"`); + if (typeof env?.source !== 'string' || env.source.trim() === '') { + bad.push(`${where}: envelope carries no source`); + } + } + } + + // Guard the guard: a walker that silently stops matching would make this + // test pass by asserting nothing. + expect(seen, 'no flow conditions found — the walker stopped matching').toBeGreaterThan(20); + expect(bad, `flow conditions that never reach the CEL engine:\n ${bad.join('\n ')}`).toEqual([]); + }); +}); + describe('priority queues sort by urgency, not alphabetically', () => { /** * `priority desc` on the select itself compares the raw option strings, so