fix(validations): guard null-aborting predicates, unify end_after_start, drop duplicate revenue rule

.changeset/validation-predicate-guards-and-operator-drift.md

@@ -0,0 +1,43 @@
+---
+'hotcrm': patch
+---
+
+Repair three classes of silently-inert validation declarations (#514, items 3, 7 and 12)
+
+Every rule below is metadata that `os validate` accepts and that no test ever
+evaluated, so each failed without erroring.
+
+**Unguarded CEL predicates on `crm_product` (item 3).** Strict CEL aborts on
+`dyn<null> < int`, which makes an unguarded comparison skip the rule entirely
+instead of failing it. `price_positive` (`record.list_price < 0`) and
+`cost_less_than_price` (`record.cost >= record.list_price`) both lacked the
+`!= null` guard that `quote_line_item.unit_price_positive` already models.
+`cost` is absent on every seeded product, so the cost warning had never
+evaluated on a single row. Both operands are now guarded. No seed data changes
+state: no seeded product has a negative list price or a cost at all.
+
+**`end_after_start` operator drift (item 12).** The same rule had three
+spellings. `crm_campaign` used `<`, accepting a campaign that ends the day it
+starts while its own message promised "End Date must be after Start Date".
+`crm_forecast.period_end_after_start` also used `<`, with an "on or after"
+message contradicting its rule name. `crm_contract` was already correct. All
+three now use `<=` in the violation predicate and say "must be after"; forecast
+periods are months or quarters, so rejecting a zero-length period is the
+intended reading. No seeded campaign or forecast has `end == start`.
+
+**Duplicated `revenue_positive` (item 7).** `crm_account` declared a validation
+saying "Annual Revenue must be positive" while `account.hook.ts` threw "must be
+greater than or equal to 0" for the same condition — the two disagreed about
+whether zero was allowed, though both compared `< 0` (it is allowed). The
+duplicate declaration is removed; the hook remains the single enforcement
+point, and it is the tested one. This is behaviour-visible only in the error
+message a client sees for a negative revenue, which is now consistently the
+hook's.
+
+New guards land in `test/object-validation-predicates.test.ts`: a repo-wide
+sweep that null-guards every operand of every ordering comparison in every
+object validation, the date-range twins pinned to one operator and one wording,
+and a single-enforcement-point check for `annual_revenue`. The sweep carries
+one documented exception — `opportunity_line_item.unit_price_positive`, the
+remaining half of item 3 — and a companion test fails if that entry ever goes
+stale.

src/objects/account.object.ts

@@ -1,4 +1,4 @@
-import { F, P, cel } from '@objectstack/spec';
+import { F, cel } from '@objectstack/spec';
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 import { ObjectSchema, Field } from '@objectstack/spec/data';
@@ -232,20 +232,20 @@ export const Account = ObjectSchema.create({
   },
 
   // Validation Rules
-  validations: [
-    {
-      name: 'revenue_positive',
-      type: 'script',
-      severity: 'error',
-      message: 'Annual Revenue must be positive',
-      // Null-guard: strict CEL cannot compare dyn<null> < int, and an
-      // unguarded predicate aborts evaluation (rule silently skipped).
-      condition: P`record.annual_revenue != null && record.annual_revenue < 0`,
-    },
-    // `account_name_unique` (type: 'unique') was removed in 7.6 — uniqueness now
-    // lives on the `name` index above (unique: true).
-  ],
-  
+  // This object declares none. Two entries used to live here:
+  //
+  // - `account_name_unique` (type: 'unique') was removed in 7.6 — uniqueness
+  //   now lives on the `name` index above (unique: true).
+  // - `revenue_positive` was removed in #514 (item 7) as a duplicate. It
+  //   restated a check `account.hook.ts` already performs on beforeInsert /
+  //   beforeUpdate, and the two disagreed in wording: the validation said
+  //   "Annual Revenue must be positive" while the hook said "greater than or
+  //   equal to 0". Both compared `< 0`, so the hook's wording was the accurate
+  //   one and zero has always been allowed. The hook is now the single
+  //   enforcement point, and it is the tested one — see
+  //   `test/hooks-runtime-sales.test.ts`, which executes the handler, whereas
+  //   the declaration was never evaluated by any test.
+
   // Workflow Rules
   // NOTE: object `workflows[]` were removed in @objectstack 7.7. Field-updates
   // moved to this object's *.hook.ts; scheduled status-flips & notifications

src/objects/campaign.object.ts

@@ -232,7 +232,10 @@ export const Campaign = ObjectSchema.create({
       type: 'script',
       severity: 'error',
       message: 'End Date must be after Start Date',
-      condition: P`record.end_date != null && record.start_date != null && record.end_date < record.start_date`,
+      // `<=`, not `<`: the message promises "after", so a campaign that ends
+      // the day it starts is a violation. Matches `crm_contract`'s twin rule
+      // and `crm_forecast.period_end_after_start` (#514 item 12).
+      condition: P`record.end_date != null && record.start_date != null && record.end_date <= record.start_date`,
     },
     {
       name: 'actual_cost_within_budget',

src/objects/forecast.object.ts

@@ -200,8 +200,14 @@ export const Forecast = ObjectSchema.create({
       name: 'period_end_after_start',
       type: 'script',
       severity: 'error',
-      message: 'Period End must be on or after Period Start.',
-      condition: P`record.period_end != null && record.period_start != null && record.period_end < record.period_start`,
+      message: 'Period End must be after Period Start.',
+      // `<=`, not `<`: the rule name says "after" and a forecast period is a
+      // month or a quarter, never a single instant, so `period_end ==
+      // period_start` is a zero-length period rather than a valid one. Same
+      // operator as the `end_after_start` twins on campaign/contract, which
+      // this rule used to diverge from in both operator and wording
+      // ("on or after") — #514 item 12.
+      condition: P`record.period_end != null && record.period_start != null && record.period_end <= record.period_start`,
     },
     {
       name: 'snapshot_amounts_non_negative',

src/objects/product.object.ts

@@ -219,14 +219,20 @@ export const Product = ObjectSchema.create({
       type: 'script',
       severity: 'error',
       message: 'List Price must be positive',
-      condition: P`record.list_price < 0`,
+      // Null-guard: strict CEL cannot compare dyn<null> < int, and an
+      // unguarded predicate aborts evaluation (rule silently skipped) — the
+      // hazard written up on `account.object.ts`. `list_price` is `required`,
+      // but the rule also runs on partial updates that omit it.
+      condition: P`record.list_price != null && record.list_price < 0`,
     },
     {
       name: 'cost_less_than_price',
       type: 'script',
       severity: 'warning',
       message: 'Cost should be less than List Price',
-      condition: P`record.cost >= record.list_price`,
+      // Both operands need the guard: `cost` is optional and absent on every
+      // seeded product, so this warning has never once evaluated.
+      condition: P`record.cost != null && record.list_price != null && record.cost >= record.list_price`,
     },
   ],
 });

test/object-validation-predicates.test.ts

@@ -0,0 +1,202 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect } from 'vitest';
+import { readFileSync } from 'node:fs';
+import { join } from 'node:path';
+import stack from '../objectstack.config';
+import { REPO_ROOT } from './helpers/repo-root';
+
+/**
+ * Guards for the CEL predicates inside object `validations[]`.
+ *
+ * A validation rule is metadata: `os validate` checks that it has a name, a
+ * severity and a condition, but never that the condition can actually
+ * *evaluate*. Two failure modes have shipped repeatedly because of that, and
+ * both are silent — the rule does not error, it simply never fires:
+ *
+ *   1. **Unguarded comparison.** Strict CEL aborts on `dyn<null> < int`, so
+ *      `record.list_price < 0` evaluates to nothing at all whenever the field
+ *      is empty. The rule looks enforced and is not. The hazard is written up
+ *      at `account.object.ts` and the correct shape is modelled by
+ *      `quote_line_item.object.ts` — every operand of a relational comparison
+ *      carries its own `!= null` guard in the same expression.
+ *
+ *   2. **Operator drift between twins.** The same rule copied across objects
+ *      picks up a different comparison operator each time, so `end == start`
+ *      is rejected on one object and accepted on another — and the message
+ *      shown to the user stops matching what the predicate does.
+ *
+ * Neither is visible in review without reading the CEL character by character,
+ * which is exactly why they survived. See #514 (items 3 and 12).
+ */
+
+type AnyRec = Record<string, any>;
+
+const objects: AnyRec[] = (stack as any).objects ?? [];
+
+/** `P` compiles to `{ dialect: 'cel', source }`; older rules 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 '';
+}
+
+/** Every `type: 'script'` validation in the stack, flattened with its owner. */
+const scriptRules = objects.flatMap((obj) =>
+  ((obj.validations ?? []) as AnyRec[])
+    .filter((v) => v.type === 'script')
+    .map((v) => ({
+      object: obj.name as string,
+      rule: v.name as string,
+      id: `${obj.name}.${v.name}`,
+      message: String(v.message ?? ''),
+      source: celSource(v.condition),
+    })),
+);
+
+/**
+ * Operands of a relational comparison (`<`, `<=`, `>`, `>=`).
+ *
+ * `!=` and `==` are deliberately not matched: equality against `null` is the
+ * guard itself, and CEL evaluates it fine on an absent field. Only the
+ * ordering operators abort.
+ */
+const OPERAND = String.raw`record\.\w+|previous\.\w+|\w+\([^()]*\)|-?[\d.]+|"[^"]*"`;
+const RELATIONAL = new RegExp(
+  String.raw`(${OPERAND})\s*(<=|>=|<|>)\s*(${OPERAND})`,
+  'g',
+);
+
+/** Record fields that the rule compares with an ordering operator. */
+function comparedFields(source: string): string[] {
+  const fields = new Set<string>();
+  for (const [, lhs, , rhs] of source.matchAll(RELATIONAL)) {
+    for (const operand of [lhs, rhs]) {
+      const field = /^record\.(\w+)$/.exec(operand)?.[1];
+      if (field) fields.add(field);
+    }
+  }
+  return [...fields];
+}
+
+/** Fields compared but never null-guarded in the same expression. */
+function unguardedFields(source: string): string[] {
+  return comparedFields(source).filter(
+    (field) => !new RegExp(String.raw`record\.${field}\s*!=\s*null`).test(source),
+  );
+}
+
+/**
+ * Rules still shipping an unguarded comparison, with the reason.
+ *
+ * This is a shrinking list, not a permanent exemption: the entry below is the
+ * remaining half of #514 item 3, carved into its own change because it touches
+ * a different object family. The "no stale entries" test keeps this honest —
+ * once that rule grows its guard, this map must be emptied or CI fails.
+ */
+const KNOWN_UNGUARDED: Record<string, string> = {
+  'crm_opportunity_line_item.unit_price_positive':
+    '#514 item 3, opportunity_line_item half — tracked separately',
+};
+
+describe('validation predicates are null-guarded', () => {
+  it('finds script validations to check at all', () => {
+    // A typo in the flattening above would turn every assertion below into a
+    // vacuous pass over an empty list.
+    expect(scriptRules.length).toBeGreaterThan(15);
+    expect(scriptRules.some((r) => r.object === 'crm_product')).toBe(true);
+  });
+
+  it('guards every operand of every ordering comparison', () => {
+    const offenders = scriptRules
+      .filter((r) => !(r.id in KNOWN_UNGUARDED))
+      .map((r) => ({ id: r.id, unguarded: unguardedFields(r.source) }))
+      .filter((r) => r.unguarded.length > 0);
+
+    expect(offenders).toEqual([]);
+  });
+
+  it('keeps no stale entries in the known-unguarded list', () => {
+    const stale = Object.keys(KNOWN_UNGUARDED).filter((id) => {
+      const rule = scriptRules.find((r) => r.id === id);
+      return !rule || unguardedFields(rule.source).length === 0;
+    });
+
+    expect(stale).toEqual([]);
+  });
+
+  it('guards both operands of the product cost/price comparison', () => {
+    // The regression that motivated the sweep: `cost` is absent on every
+    // seeded product, so the warning never fired on any real row.
+    const rule = scriptRules.find((r) => r.id === 'crm_product.cost_less_than_price');
+    expect(rule).toBeDefined();
+    expect(unguardedFields(rule!.source)).toEqual([]);
+    expect(comparedFields(rule!.source).sort()).toEqual(['cost', 'list_price']);
+  });
+});
+
+/**
+ * The three date-range twins, unified on one operator.
+ *
+ * All three assert "end must come strictly after start", so the *violation*
+ * predicate — validations fire when the condition is true — is `end <= start`.
+ * Campaign used `<`, which accepted a zero-length campaign while its own
+ * message promised "must be after"; forecast used `<` with an "on or after"
+ * message that its rule name (`period_end_after_start`) contradicted.
+ */
+const DATE_RANGE_RULES = [
+  { id: 'crm_campaign.end_after_start', end: 'end_date', start: 'start_date' },
+  { id: 'crm_contract.end_after_start', end: 'end_date', start: 'start_date' },
+  { id: 'crm_forecast.period_end_after_start', end: 'period_end', start: 'period_start' },
+] as const;
+
+describe('end-after-start rules agree on one operator', () => {
+  it.each(DATE_RANGE_RULES)('$id rejects end == start', ({ id, end, start }) => {
+    const rule = scriptRules.find((r) => r.id === id);
+    expect(rule, `${id} is missing`).toBeDefined();
+    expect(rule!.source).toContain(`record.${end} <= record.${start}`);
+  });
+
+  it.each(DATE_RANGE_RULES)('$id null-guards both dates', ({ id, end, start }) => {
+    const rule = scriptRules.find((r) => r.id === id);
+    expect(comparedFields(rule!.source).sort()).toEqual([end, start].sort());
+    expect(unguardedFields(rule!.source)).toEqual([]);
+  });
+
+  it.each(DATE_RANGE_RULES)('$id says "after", never "on or after"', ({ id }) => {
+    const rule = scriptRules.find((r) => r.id === id);
+    // The message is the user-visible contract; "on or after" would now be a
+    // lie, since the predicate rejects equality.
+    expect(rule!.message).toMatch(/must be after/i);
+    expect(rule!.message).not.toMatch(/on or after/i);
+  });
+});
+
+/**
+ * `annual_revenue >= 0` is enforced in exactly one place.
+ *
+ * It used to be enforced twice — an object validation saying "must be
+ * positive" and a `beforeInsert`/`beforeUpdate` throw saying "must be greater
+ * than or equal to 0" — which disagreed about whether 0 was allowed even
+ * though both compared `< 0`. The hook is the surviving enforcement point
+ * (`test/hooks-runtime-sales.test.ts` executes it); the duplicate declaration
+ * is gone. See #514 item 7.
+ */
+describe('annual_revenue has a single enforcement point', () => {
+  it('declares no revenue validation on crm_account', () => {
+    const account = objects.find((o) => o.name === 'crm_account');
+    expect(account, 'crm_account is missing').toBeDefined();
+    const revenueRules = ((account!.validations ?? []) as AnyRec[]).filter((v) =>
+      celSource(v.condition).includes('annual_revenue'),
+    );
+    expect(revenueRules).toEqual([]);
+  });
+
+  it('still enforces it in the account hook', () => {
+    const hook = readFileSync(join(REPO_ROOT, 'src/objects/account.hook.ts'), 'utf8');
+    expect(hook).toMatch(/input\.annual_revenue\s*<\s*0/);
+    expect(hook).toMatch(/greater than or equal to 0/);
+  });
+});