fix(flows): unbreak main — contract_renewal throws; rewrite the two tests #565 turned red

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

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

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

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;

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);
+  });
+});

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', () => {