diff --git a/.changeset/loop-nested-flow-conditions.md b/.changeset/loop-nested-flow-conditions.md new file mode 100644 index 00000000..14347106 --- /dev/null +++ b/.changeset/loop-nested-flow-conditions.md @@ -0,0 +1,47 @@ +--- +'hotcrm': patch +--- + +Fix `contract_renewal`'s notice-window gate, which throws instead of +evaluating, and rewrite the two tests that #565 left red on main. + +**The live regression.** #565 correctly wrapped flow conditions as CEL +envelopes so they finally evaluate. That exposed a second defect underneath, in +`contract_renewal`: + +``` +timestamp(currentContract.end_date) <= daysFromNow(int(currentContract.renewal_notice_days)) +``` + +`end_date` is a DATE field and arrives as `YYYY-MM-DD`, but CEL's `timestamp()` +accepts only a full ISO 8601 datetime — it throws `timestamp() requires a +string in ISO 8601 format`. While the condition was a bare string it was never +evaluated at all, so this sat latent; making the envelope real makes it throw +mid-sweep. The sweep therefore still books nothing, having traded a silent +no-op for an exception. Appending `T00:00:00Z` fixes it. Verified load-bearing: +reverting just that change fails four `contract_renewal` tests. + +**The two red tests on main**, both introduced by #563 and both firing exactly +as designed when #565 fixed the behaviour they pinned: + +- `flow-scheduled.test.ts` asserted the stagnation gate stays shut and failed + with the message written for this moment — "gate now opens — rewrite this + test". Rewritten to assert real behaviour: the nudge task and notification + are created, repeated sweeps stay idempotent, and the sweep re-arms once the + previous stall task completes. `contract_renewal` gets the same treatment + (task, notification, `auto_renewal` opportunity, per-contract notice window, + no duplicate renewal deal). +- `flow-record-change.test.ts` asserted `typeof startCondition === 'string'` + for `opportunity_approval`. #565 converted it to an envelope. That assertion + was pinning the *notation* rather than what matters, so it now accepts either + form and checks a non-empty expression exists. + +**Guards added** so the class stays fixed: one pins the engine asymmetry that +makes envelopes necessary (a bare string still evaluates `false` where an +envelope evaluates `true`), and one walks every registered flow — including +nested loops — failing if any loop body reintroduces a bare string condition. +It also asserts the walk found something, so it cannot pass vacuously. + +`campaign_enrollment` gains a runtime suite (eligibility, opt-out, cross-campaign +dedupe, closed-campaign refusal) and leaves `PENDING_FLOWS`, which is now down +to `case_csat_followup` and `demo_bootstrap`. diff --git a/.changeset/task-recurrence-month-clamp.md b/.changeset/task-recurrence-month-clamp.md new file mode 100644 index 00000000..c72779a3 --- /dev/null +++ b/.changeset/task-recurrence-month-clamp.md @@ -0,0 +1,28 @@ +--- +'hotcrm': patch +--- + +Clamp monthly/yearly task recurrence to the last valid day instead of +overflowing into the next month. + +`advanceDate` stepped months with `Date.setMonth`, which rolls a day that does +not exist in the target month forward rather than clamping. A recurring task due +Jan 31 spawned its next occurrence on **Mar 3** — drifting further into the +following month on every occurrence and skipping February outright. Feb 29 on a +leap year had the same shape. + +Month and year steps now move to the 1st before doing the arithmetic and then +clamp the day to the target month's length: Jan 31 → Feb 28 (Feb 29 in a leap +year), Mar 31 → Apr 30, Feb 29 2028 → Feb 28 2029. + +Note the residual behaviour, which is asserted rather than left to be discovered: +each occurrence is computed from the previous due date, not from an anchor day, +so a month-end series settles on the shorter day (Jan 31 → Feb 28 → Mar 28) +instead of returning to the 31st. Preserving the anchor needs a schema change; +the behaviour being replaced drifted forward without bound, so this is a strict +improvement. + +`advanceDate` also moves inside the handler. It was the last module-level helper +in `src/objects/`, and every sibling hook documents why that is unsafe: L2 hook +bodies run body-only in the QuickJS sandbox, where module scope is not +available. diff --git a/src/flows/contract-renewal.flow.ts b/src/flows/contract-renewal.flow.ts index b4948616..2b7ad612 100644 --- a/src/flows/contract-renewal.flow.ts +++ b/src/flows/contract-renewal.flow.ts @@ -59,7 +59,14 @@ export const ContractRenewalFlow: Flow = { nodes: [ { id: 'check_notice_window', type: 'decision', label: 'Within Notice Window?', - config: { condition: P`timestamp(currentContract.end_date) <= daysFromNow(int(currentContract.renewal_notice_days))` }, + // `end_date` is a DATE field and arrives as `YYYY-MM-DD`, but CEL's + // `timestamp()` only accepts a full ISO 8601 datetime and throws + // otherwise ("timestamp() requires a string in ISO 8601 format"). + // Appending the time part is what makes this evaluate instead of + // 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))` }, }, { // Idempotency gate: the sweep matches the same contract every @@ -150,7 +157,7 @@ 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) <= 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 + "T00:00:00Z") <= 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: P`existingRenewalTask == null`, label: 'First reminder' }, { id: 'b4', source: 'create_renewal_task', target: 'notify_owner', type: 'default' }, diff --git a/src/objects/task.hook.ts b/src/objects/task.hook.ts index 3f99c888..e3d74045 100644 --- a/src/objects/task.hook.ts +++ b/src/objects/task.hook.ts @@ -76,16 +76,6 @@ const taskValidation: Hook = { }, }; -/** Advance a date by `interval` units of the given recurrence type. */ -function advanceDate(d: Date, type: string, interval: number): Date { - const next = new Date(d); - if (type === 'daily') next.setDate(next.getDate() + interval); - else if (type === 'weekly') next.setDate(next.getDate() + 7 * interval); - else if (type === 'monthly') next.setMonth(next.getMonth() + interval); - else if (type === 'yearly') next.setFullYear(next.getFullYear() + interval); - return next; -} - /** * Recurring-task generator. * @@ -109,6 +99,45 @@ const taskRecurrence: Hook = { onError: 'log', description: 'On completion of a recurring task, spawn the next occurrence (dates advanced by recurrence_type × interval).', handler: async (ctx: HookContext) => { + /** + * Advance a date by `interval` units of the given recurrence type. + * + * Declared INSIDE the handler, matching every other hook in this directory: + * L2 hook bodies run body-only in the QuickJS sandbox, where module scope + * is not available (cf. opportunity.hook.ts / lead.hook.ts). This was the + * one module-level helper left in the tree. + * + * Month and year steps CLAMP to the last valid day of the target month + * instead of overflowing. `Date.setMonth` rolls a day that does not exist + * forward into the next month — Jan 31 + 1 month landed on Mar 3, so a + * month-end recurring task walked further into the following month on every + * occurrence and, for the 31st, skipped February entirely. + */ + function advanceDate(d: Date, type: string, interval: number): Date { + const next = new Date(d); + if (type === 'daily') { + next.setDate(next.getDate() + interval); + return next; + } + if (type === 'weekly') { + next.setDate(next.getDate() + 7 * interval); + return next; + } + const months = type === 'monthly' ? interval : type === 'yearly' ? 12 * interval : 0; + if (months === 0) return next; + + const day = next.getDate(); + // Move to the 1st first so the month arithmetic can never overflow, then + // clamp the day to the target month's length. + next.setDate(1); + next.setMonth(next.getMonth() + months); + const lastDayOfTargetMonth = new Date( + next.getFullYear(), next.getMonth() + 1, 0, + ).getDate(); + next.setDate(Math.min(day, lastDayOfTargetMonth)); + return next; + } + const { input } = ctx; const previous = ctx.previous; const api = ctx.api as HookApi | undefined; diff --git a/test/flow-campaign-enrollment.test.ts b/test/flow-campaign-enrollment.test.ts new file mode 100644 index 00000000..e235a083 --- /dev/null +++ b/test/flow-campaign-enrollment.test.ts @@ -0,0 +1,104 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { CampaignEnrollmentFlow } from '../src/flows/campaign-enrollment.flow'; +import { makeFlowHarness, type Rec } from './helpers/flow-harness'; + +/** + * campaign_enrollment runtime tests. + * + * This is the console's bulk "enroll leads" screen action. Its dedupe gate sits + * inside a `loop` body, so until the nested condition was authored as an + * explicit CEL envelope it never opened and the action enrolled nobody at all — + * see the regression guard in test/flow-scheduled.test.ts. + * + * Three things here are silently breakable and none is visible to metadata + * validation: the `recordId` input contract, the eligibility filter (an + * opted-out lead must never be enrolled in an email campaign), and the dedupe + * that makes a re-run top up rather than double-enrol. + */ + +const campaign = (over: Rec = {}): Rec => ({ + id: 'cmp1', name: 'Spring Push', status: 'in_progress', ...over, +}); + +const leads = (): Rec[] => [ + { id: 'l_new', status: 'new', is_converted: false, email: 'a@acme.io', email_opt_out: false }, + { id: 'l_new2', status: 'new', is_converted: false, email: 'b@acme.io', email_opt_out: false }, + // Ineligible for various reasons — none may be enrolled. + { id: 'l_optout', status: 'new', is_converted: false, email: 'c@acme.io', email_opt_out: true }, + { id: 'l_converted', status: 'new', is_converted: true, email: 'd@acme.io', email_opt_out: false }, + { id: 'l_noemail', status: 'new', is_converted: false, email: null, email_opt_out: false }, + { id: 'l_other', status: 'qualified', is_converted: false, email: 'e@acme.io', email_opt_out: false }, +]; + +async function enrol(seed: Rec = {}, screen: Rec = { leadStatus: 'new' }) { + const h = makeFlowHarness({ campaign_enrollment: CampaignEnrollmentFlow }, { + crm_campaign: [campaign()], + crm_lead: leads(), + crm_campaign_member: [], + ...seed, + }); + const runId = await h.run('campaign_enrollment', { recordId: 'cmp1' }); + expect(runId, 'campaign_enrollment did not start').toBeTruthy(); + await h.resume(runId!, { recordId: 'cmp1', ...screen }); + return h; +} + +describe('campaign_enrollment — screen action', () => { + it('seeds its input from the console’s `recordId` contract', () => { + const names = (CampaignEnrollmentFlow.variables ?? []).map((v) => v.name); + expect(names, 'the console only seeds `recordId`').toContain('recordId'); + }); + + it('enrols the eligible leads in the chosen status', async () => { + const h = await enrol(); + const members = h.store.crm_campaign_member; + expect(members.map((m) => m.crm_lead).sort()).toEqual(['l_new', 'l_new2']); + for (const m of members) { + expect(m.crm_campaign).toBe('cmp1'); + expect(m.status).toBe('sent'); + expect(m.added_date, 'added_date should be stamped').toBeTruthy(); + } + }); + + it('never enrols an opted-out, converted, email-less or off-status lead', async () => { + const h = await enrol(); + const enrolled = h.store.crm_campaign_member.map((m) => m.crm_lead); + for (const id of ['l_optout', 'l_converted', 'l_noemail', 'l_other']) { + expect(enrolled, `${id} must not be enrolled`).not.toContain(id); + } + }); + + it('tops up rather than double-enrolling on a re-run', async () => { + // Duplicate member rows inflated num_sent and the response rate. + const h = await enrol({ + crm_campaign_member: [{ id: 'm_existing', crm_campaign: 'cmp1', crm_lead: 'l_new', status: 'sent' }], + }); + const forNew = h.store.crm_campaign_member.filter((m) => m.crm_lead === 'l_new'); + expect(forNew, 'l_new was enrolled twice').toHaveLength(1); + // …and the not-yet-enrolled lead still gets added. + expect(h.store.crm_campaign_member.map((m) => m.crm_lead)).toContain('l_new2'); + }); + + it('does not treat an enrolment in ANOTHER campaign as a duplicate', async () => { + const h = await enrol({ + crm_campaign_member: [{ id: 'm_other', crm_campaign: 'cmp_other', crm_lead: 'l_new', status: 'sent' }], + }); + const forNew = h.store.crm_campaign_member.filter( + (m) => m.crm_lead === 'l_new' && m.crm_campaign === 'cmp1', + ); + expect(forNew, 'a member row for a different campaign blocked enrolment').toHaveLength(1); + }); + + it.each(['completed', 'aborted'])('refuses to top up a %s campaign', async (status) => { + // Enrolling into a finished campaign corrupts its final snapshot metrics. + const h = await enrol({ crm_campaign: [campaign({ status })] }); + expect(h.store.crm_campaign_member).toHaveLength(0); + }); + + it('enrols into a campaign still in planning', async () => { + const h = await enrol({ crm_campaign: [campaign({ status: 'planning' })] }); + expect(h.store.crm_campaign_member.length).toBeGreaterThan(0); + }); +}); diff --git a/test/flow-record-change.test.ts b/test/flow-record-change.test.ts index 97837c13..5b5036bf 100644 --- a/test/flow-record-change.test.ts +++ b/test/flow-record-change.test.ts @@ -257,7 +257,20 @@ describe('opportunity_approval — start condition', () => { .find((n) => n.id === 'start')?.config?.condition; it('declares a start condition that gates on the approval-worthy transition', () => { - expect(typeof startCondition, 'opportunity_approval lost its start condition').toBe('string'); + // Accepts either authoring form: a bare string (which the engine wraps + // into a CEL envelope for START conditions) or an explicit + // `{ dialect, source }` envelope. Pinning `typeof === 'string'` made this + // fail the moment the flow was legitimately converted to the envelope + // form — it was asserting the notation, not the thing that matters, which + // is that a gate exists and carries an expression at all. + const source = + typeof startCondition === 'string' + ? startCondition + : (startCondition as { source?: unknown } | undefined)?.source; + expect( + typeof source === 'string' && source.length > 0, + `opportunity_approval lost its start condition (got ${JSON.stringify(startCondition)})`, + ).toBe(true); }); it('does not re-enter for an opportunity already pending approval', () => { diff --git a/test/flow-scheduled.test.ts b/test/flow-scheduled.test.ts index 84f065fc..d596b6da 100644 --- a/test/flow-scheduled.test.ts +++ b/test/flow-scheduled.test.ts @@ -8,6 +8,7 @@ import { ContractRenewalFlow } from '../src/flows/contract-renewal.flow'; import { OpportunityStagnationFlow } from '../src/flows/opportunity-stagnation.flow'; import { QuoteExpirationFlow } from '../src/flows/quote-expiration.flow'; import { TaskDueReminderFlow } from '../src/flows/task-due-reminder.flow'; +import * as allFlows from '../src/flows'; import { makeFlowHarness, type Rec } from './helpers/flow-harness'; /** @@ -293,23 +294,64 @@ describe('opportunity_stagnation — daily stalled-deal nudge', () => { return h; }; - it('selects exactly the open deals past the 14-day threshold', async () => { + it('nudges exactly the open deals past the 14-day threshold', async () => { const h = await run(); - // The loop body issues one crm_task lookup per selected deal, so those - // lookups are the observable record of what the sweep picked up. - const nudgeLookups = h.queries.filter((q) => q.object === 'crm_task'); - expect(nudgeLookups.map((q) => q.where.related_to_opportunity)).toEqual(['o_stalled']); + + expect(h.store.crm_task).toHaveLength(1); + const [task] = h.store.crm_task; + expect(task.subject).toBe('Advance stalled deal: Stalled Deal'); + expect(task.related_to_opportunity).toBe('o_stalled'); + expect(task.related_to_type).toBe('crm_opportunity'); + expect(task.owner).toBe('rep1'); + expect(task.priority).toBe('high'); + expect(task.status).toBe('not_started'); + + expect(h.notifications).toHaveLength(1); + expect(h.notifications[0].to).toContain('rep1'); + expect(String(h.notifications[0].title)).toContain('Stalled Deal'); + expect( + JSON.stringify(h.notifications[0]), + 'a template dot-walked a lookup', + ).not.toContain('undefined'); }); - it('skips fresh, boundary and closed deals', async () => { + it('skips fresh, boundary, closed and null-clock deals', async () => { const h = await run(); - const touched = h.queries - .filter((q) => q.object === 'crm_task') - .map((q) => q.where.related_to_opportunity); + const touched = h.store.crm_task.map((t) => t.related_to_opportunity); for (const id of ['o_fresh', 'o_boundary', 'o_won', 'o_lost', 'o_nullclock']) { - expect(touched, `${id} should not have been swept`).not.toContain(id); + expect(touched, `${id} should not have been nudged`).not.toContain(id); } }); + + it('is idempotent: a second sweep does not pile up duplicate nudges', async () => { + // Without the "already nudged?" gate the daily sweep re-notified and + // re-created an identical task every morning for as long as the deal stayed + // stalled — an unbounded duplicate pile-up. + const h = makeFlowHarness( + { opportunity_stagnation: OpportunityStagnationFlow }, + { crm_opportunity: seedOpps(), crm_task: [] }, + ); + await h.run('opportunity_stagnation', {}, { event: 'schedule' }); + await h.run('opportunity_stagnation', {}, { event: 'schedule' }); + + expect(h.store.crm_task, 'duplicate stall task on the second sweep').toHaveLength(1); + expect(h.notifications, 'duplicate nudge on the second sweep').toHaveLength(1); + }); + + it('re-arms once the previous stall task is completed', async () => { + const h = makeFlowHarness( + { opportunity_stagnation: OpportunityStagnationFlow }, + { + crm_opportunity: seedOpps(), + crm_task: [{ + id: 't_done', related_to_opportunity: 'o_stalled', + subject: 'Advance stalled deal: Stalled Deal', status: 'completed', + }], + }, + ); + await h.run('opportunity_stagnation', {}, { event: 'schedule' }); + expect(h.store.crm_task.filter((t) => t.status === 'not_started')).toHaveLength(1); + }); }); describe('contract_renewal — daily notice-window sweep', () => { @@ -349,101 +391,168 @@ describe('contract_renewal — daily notice-window sweep', () => { contract({ id: 'k_past', end_date: day(-5) }), contract({ id: 'k_draft', status: 'draft' }), ]); - const contractQuery = h.queries.find((x) => x.object === 'crm_contract')!; - const selected = h.store.crm_contract.filter( - (k) => k.status === contractQuery.where.status && - String(k.end_date) >= String(contractQuery.where.end_date.$gte).slice(0, 10) && - String(k.end_date) <= String(contractQuery.where.end_date.$lte).slice(0, 10), - ); - expect(selected).toHaveLength(0); + expect(h.store.crm_task).toHaveLength(0); + expect(h.notifications).toHaveLength(0); + }); + + it('books a renewal task and notifies the owner inside the notice window', async () => { + const h = await run([contract({ end_date: day(+20), renewal_notice_days: 30 })]); + + expect(h.store.crm_task).toHaveLength(1); + const [task] = h.store.crm_task; + expect(task.subject).toBe('Renewal due: contract CTR-1'); + expect(task.related_to_account).toBe('acc1'); + expect(task.owner).toBe('rep1'); + expect(task.priority).toBe('high'); + + expect(h.notifications).toHaveLength(1); + expect(h.notifications[0].to).toContain('rep1'); + expect(String(h.notifications[0].title)).toContain('CTR-1'); + }); + + it('honours each contract’s own renewal_notice_days, not a shared constant', async () => { + // The pre-filter spans 120 days precisely so a 90-day notice period is not + // silently truncated; the per-record decision applies the real window. + const h = await run([ + contract({ id: 'k_early', contract_number: 'CTR-90', end_date: day(+80), renewal_notice_days: 90 }), + contract({ id: 'k_late', contract_number: 'CTR-30', end_date: day(+80), renewal_notice_days: 30 }), + ]); + const subjects = h.store.crm_task.map((t) => t.subject); + expect(subjects, 'the 90-day-notice contract is in window').toContain('Renewal due: contract CTR-90'); + expect(subjects, 'the 30-day-notice contract is still 80 days out').not.toContain('Renewal due: contract CTR-30'); + }); + + it('opens a pre-filled renewal opportunity only when auto_renewal is on', async () => { + const withAuto = await run([contract({ auto_renewal: true })]); + expect(withAuto.store.crm_opportunity).toHaveLength(1); + const [opp] = withAuto.store.crm_opportunity; + expect(opp.name).toBe('Renewal — CTR-1'); + expect(opp.crm_account).toBe('acc1'); + expect(opp.type).toBe('existing_renewal'); + expect(Number(opp.amount)).toBe(90_000); + expect(opp.stage).toBe('proposal'); + + const withoutAuto = await run([contract({ auto_renewal: false })]); + expect(withoutAuto.store.crm_opportunity).toHaveLength(0); + }); + + it('never opens a second renewal deal while one is still in flight', async () => { + const h = await run([contract({ auto_renewal: true })], { + crm_opportunity: [{ + id: 'o_open', crm_account: 'acc1', type: 'existing_renewal', stage: 'negotiation', + }], + }); + expect(h.store.crm_opportunity, 'duplicate renewal deal opened').toHaveLength(1); + }); + + it('is idempotent across repeated sweeps within the same window', async () => { + const h = makeFlowHarness({ contract_renewal: ContractRenewalFlow }, { + crm_contract: [contract({ auto_renewal: true })], crm_task: [], crm_opportunity: [], + }); + await h.run('contract_renewal', {}, { event: 'schedule' }); + await h.run('contract_renewal', {}, { event: 'schedule' }); + expect(h.store.crm_task, 'duplicate renewal task').toHaveLength(1); + expect(h.store.crm_opportunity, 'duplicate renewal deal').toHaveLength(1); + expect(h.notifications, 'duplicate renewal notification').toHaveLength(1); + }); + + it('ignores contracts that are not activated', async () => { + const h = await run([ + contract({ status: 'draft' }), + contract({ id: 'k2', status: 'expired' }), + ]); + expect(h.store.crm_task).toHaveLength(0); + expect(h.notifications).toHaveLength(0); }); }); /** - * KNOWN DEFECT — conditional edges nested inside a `loop` body never fire. + * Regression guard — a bare string condition inside a `loop` body is inert. * * `AutomationEngine.registerFlow` runs `applyConversionsToFlow`, which rewrites * a bare string `condition` into a `{ dialect: 'cel', source }` envelope. That - * pass only walks the flow's TOP-LEVEL `edges`; it does not recurse into the + * pass only walks a flow's TOP-LEVEL `edges`; it does not recurse into the * structured control-flow regions introduced by ADR-0031 (`loop.config.body`). * - * A condition left as a bare string falls through to the engine's legacy - * template path, which substitutes `{var}` templates (there are none here) and - * then STRING-compares the leftover expression text. So: + * A condition left as a bare string in there falls through to the engine's + * legacy template path, which substitutes `{var}` templates (there are none) + * and then STRING-compares the leftover expression text: * * 'existingStallTask == null' → 'existingStallTask' === 'null' → false * - * The gate never opens. Verified against @objectstack/service-automation - * 16.1.0: passing the same expression as an explicit `{dialect:'cel', source}` - * envelope evaluates correctly, which is why every TOP-LEVEL conditional edge - * in this app works and only the loop-nested ones do not. - * - * Three flows carry conditional edges inside a loop body and are therefore - * inert past that gate: `opportunity_stagnation`, `contract_renewal`, and - * `campaign_enrollment`. + * The gate never opens, and the failure is silent: the sweep runs, selects the + * right records, and does nothing. `opportunity_stagnation`, `contract_renewal` + * and `campaign_enrollment` all shipped in that state. * - * These assertions pin the CURRENT behaviour deliberately, so the defect is - * visible and tracked rather than silent. Fixing it — whether by authoring the - * nested conditions as explicit CEL envelopes here, or by making the conversion - * pass recurse upstream — changes what these scheduled sweeps DO in production - * (they would begin creating tasks, opportunities and notifications), so it is - * deliberately NOT bundled into this verification-pipeline change. When it is - * fixed, these three tests go red and must be rewritten to assert the real - * behaviour. + * The fix is to author nested conditions as explicit envelopes. These two tests + * keep it fixed: the first pins the engine asymmetry that makes the envelope + * necessary (so an upstream fix that makes bare strings work shows up as a + * deliberate review rather than a silent behaviour change), and the second + * fails if ANY flow ever reintroduces a bare string inside a loop body. */ -describe('KNOWN DEFECT: loop-nested conditions never evaluate (see comment above)', () => { - it('opportunity_stagnation selects stalled deals but never nudges', async () => { - const h = makeFlowHarness( - { opportunity_stagnation: OpportunityStagnationFlow }, - { - crm_opportunity: [ - { id: 'o_stalled', name: 'Stalled Deal', stage: 'proposal', stage_entry_date: day(-30), owner: 'rep1' }, - ], - crm_task: [], - }, - ); - await h.run('opportunity_stagnation', {}, { event: 'schedule' }); - - // Proof the deal WAS selected — the per-iteration lookup ran… - expect(h.queries.some((q) => q.object === 'crm_task')).toBe(true); - // …but the `existingStallTask == null` gate never opened. - expect(h.store.crm_task, 'gate now opens — rewrite this test').toHaveLength(0); - expect(h.notifications, 'gate now opens — rewrite this test').toHaveLength(0); - }); - - it('contract_renewal selects contracts in window but never books a renewal', async () => { - const h = makeFlowHarness({ contract_renewal: ContractRenewalFlow }, { - crm_contract: [{ - id: 'k1', contract_number: 'CTR-1', status: 'activated', crm_account: 'acc1', - owner: 'rep1', contract_value: 90_000, auto_renewal: true, - renewal_notice_days: 30, end_date: day(+20), - }], - crm_task: [], - crm_opportunity: [], - }); - await h.run('contract_renewal', {}, { event: 'schedule' }); - - expect(h.queries.some((q) => q.object === 'crm_contract')).toBe(true); - // The very first gate (`timestamp(end_date) <= daysFromNow(...)`) compares - // the literal strings 'timestamp(currentContract.end_date)' and - // 'daysFromNow(int(currentContract.renewal_notice_days))', so nothing runs. - expect(h.store.crm_task, 'gate now opens — rewrite this test').toHaveLength(0); - expect(h.store.crm_opportunity, 'gate now opens — rewrite this test').toHaveLength(0); - expect(h.notifications, 'gate now opens — rewrite this test').toHaveLength(0); - }); - - it('an explicit CEL envelope DOES evaluate — this is the available fix', async () => { - // Documents the remedy: the same expression, wrapped, behaves correctly. +describe('loop-nested conditions must be explicit CEL envelopes', () => { + it('the engine still treats a bare string differently from an envelope', () => { const h = makeFlowHarness({}, {}); const evaluate = (condition: unknown) => (h.engine as unknown as { evaluateCondition(c: unknown, v: Map): boolean; }).evaluateCondition(condition, new Map([['existingStallTask', null]])); - expect(evaluate('existingStallTask == null'), 'bare string (loop-nested form)').toBe(false); + // If this ever flips to `true`, the conversion pass (or the legacy path) + // changed upstream — revisit whether the explicit envelopes are still + // needed before relaxing them. + expect(evaluate('existingStallTask == null'), 'bare string, unresolved').toBe(false); expect( evaluate({ dialect: 'cel', source: 'existingStallTask == null' }), 'explicit CEL envelope', ).toBe(true); }); + + it('no flow carries a bare string condition inside a loop body', () => { + /** Every condition reachable inside a `loop` node's body, with its path. */ + const nestedConditions: Array<{ flow: string; where: string; condition: unknown }> = []; + + const visitBody = (flowName: string, loopId: string, body: Rec | undefined) => { + if (!body) return; + for (const node of (body.nodes ?? []) as Rec[]) { + if (node?.config?.condition !== undefined) { + nestedConditions.push({ + flow: flowName, + where: `${loopId}/node:${node.id}`, + condition: node.config.condition, + }); + } + // A loop nested in a loop is subject to the same rule. + if (node?.type === 'loop') visitBody(flowName, `${loopId}/${node.id}`, node.config?.body); + } + for (const edge of (body.edges ?? []) as Rec[]) { + if (edge?.condition !== undefined) { + nestedConditions.push({ + flow: flowName, + where: `${loopId}/edge:${edge.id}`, + condition: edge.condition, + }); + } + } + }; + + for (const flow of Object.values(allFlows) as Rec[]) { + if (!flow || typeof flow !== 'object' || !Array.isArray(flow.nodes)) continue; + for (const node of flow.nodes as Rec[]) { + if (node?.type === 'loop') visitBody(flow.name, node.id, node.config?.body); + } + } + + // Guards the guard: if the walk finds nothing, it is asserting nothing. + expect(nestedConditions.length, 'no loop-nested conditions found — the walk broke').toBeGreaterThan(0); + + const bare = nestedConditions + .filter((c) => typeof c.condition === 'string') + .map((c) => `${c.flow} ${c.where}: ${JSON.stringify(c.condition)}`); + expect( + bare, + 'bare string condition(s) inside a loop body — these never evaluate.\n' + + "Wrap as { dialect: 'cel', source: '…' }:\n " + bare.join('\n '), + ).toEqual([]); + }); }); diff --git a/test/hooks-runtime-service.test.ts b/test/hooks-runtime-service.test.ts index c1951304..04b6a9ba 100644 --- a/test/hooks-runtime-service.test.ts +++ b/test/hooks-runtime-service.test.ts @@ -635,12 +635,17 @@ describe('task_recurrence', () => { it.each([ ['daily', 1, '2026-01-01', '2026-01-02'], ['weekly', 2, '2026-01-01', '2026-01-15'], - // Pins CURRENT behaviour, which is not obviously the desired one: `advanceDate` - // uses `Date.setMonth`, so Jan 31 + 1 month overflows Feb and lands on Mar 3 - // rather than clamping to Feb 28. Worth a follow-up; asserted here so the - // quirk is visible instead of silent. - ['monthly', 1, '2026-01-31', '2026-03-03'], + ['monthly', 1, '2026-01-15', '2026-02-15'], + ['monthly', 3, '2026-01-15', '2026-04-15'], + // Month-end CLAMPS instead of overflowing. `Date.setMonth` used to roll + // Jan 31 + 1 month forward to Mar 3, so a month-end series walked deeper + // into the following month on every occurrence and skipped February. + ['monthly', 1, '2026-01-31', '2026-02-28'], + ['monthly', 1, '2028-01-31', '2028-02-29'], // leap year + ['monthly', 1, '2026-03-31', '2026-04-30'], + ['monthly', 2, '2026-01-31', '2026-03-31'], // target month is long enough ['yearly', 1, '2026-03-01', '2027-03-01'], + ['yearly', 1, '2028-02-29', '2029-02-28'], // leap day → last valid day ])('advances a %s/%i series from %s to %s', async (type, interval, from, to) => { const h = makeHarness({ crm_task: [] }); await completeRecurring(h, { @@ -652,6 +657,27 @@ describe('task_recurrence', () => { expect(next.due_date).toBe(to); }); + it('a month-end series settles on the shorter day rather than walking forward', async () => { + // Each occurrence is computed from the PREVIOUS due date, not from an + // anchor day, so clamping Jan 31 → Feb 28 makes the following occurrence + // Mar 28 rather than Mar 31. That is a deliberate trade: the alternative + // (storing an anchor) is a schema change, and the behaviour being replaced + // was strictly worse — `setMonth` overflow walked Jan 31 → Mar 3 → Apr 3, + // drifting FORWARD and skipping February outright. + const dueDates: string[] = []; + let due = '2026-01-31'; + for (let i = 0; i < 3; i++) { + const h = makeHarness({ crm_task: [] }); + await completeRecurring(h, { + subject: 'Month end', is_recurring: true, + recurrence_type: 'monthly', recurrence_interval: 1, due_date: due, + }); + due = h.rows('crm_task')[0].due_date as string; + dueDates.push(due); + } + expect(dueDates).toEqual(['2026-02-28', '2026-03-28', '2026-04-28']); + }); + it('spawns the next occurrence as not_started so it cannot re-trigger itself', async () => { const h = makeHarness({ crm_task: [] }); await completeRecurring(h, { diff --git a/test/runtime-coverage.test.ts b/test/runtime-coverage.test.ts index 3082732b..84509fba 100644 --- a/test/runtime-coverage.test.ts +++ b/test/runtime-coverage.test.ts @@ -39,6 +39,7 @@ const RUNTIME_TEST_FILES = [ 'flow-scheduled.test.ts', 'flow-record-change.test.ts', 'flow-case-actions.test.ts', + 'flow-campaign-enrollment.test.ts', ]; /** @@ -49,10 +50,6 @@ const RUNTIME_TEST_FILES = [ * the suite too, so it cannot rot. */ const PENDING_FLOWS = new Set([ - // Screen flow driven entirely from the console's campaign UI; its loop-nested - // enrolment gate is inert for the reason documented in flow-scheduled.test.ts, - // so a behavioural test would only pin that defect a third time. - 'campaign_enrollment', // Spans a 24h `wait` node; needs timer-resume support in the harness. 'case_csat_followup', // First-boot demo seeding. Exercised end to end every time the e2e suite