From 8df119f6c6f636f7038c8fff29f381733625a6cf Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 08:43:15 +0000 Subject: [PATCH] fix(validations): guard null-aborting predicates, unify end_after_start, drop duplicate revenue rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three families of validation declarations that `os validate` accepts and no test ever evaluated, so each failed silently rather than erroring. Unguarded CEL on crm_product: strict CEL aborts on `dyn < int`, so an unguarded comparison skips the rule instead of failing it. `price_positive` and `cost_less_than_price` lacked the `!= null` guard that `quote_line_item.unit_price_positive` already models — and `cost` is absent on every seeded product, so that warning had never evaluated on a single row. end_after_start drift: campaign used `<`, accepting a campaign that ends the day it starts while its own message promised "must be after"; forecast used `<` with an "on or after" message contradicting its rule name; contract was already correct. All three now reject `end == start` and say "must be after". revenue_positive was declared twice with diverging wording — the validation said "must be positive", the hook threw "greater than or equal to 0", and both compared `< 0`, so the hook's wording was the accurate one. The duplicate declaration is dropped; the hook stays as the single, tested enforcement point. No seed data changes state under any of these: no seeded product has a negative list price or a cost, and no seeded campaign or forecast has end == start. New guards live in test/object-validation-predicates.test.ts — a repo-wide null-guard sweep over 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), with a companion test that fails if the entry goes stale. Refs #514 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019sj147MYMYNJEs26HDC6fw --- ...ion-predicate-guards-and-operator-drift.md | 43 ++++ src/objects/account.object.ts | 30 +-- src/objects/campaign.object.ts | 5 +- src/objects/forecast.object.ts | 10 +- src/objects/product.object.ts | 10 +- test/object-validation-predicates.test.ts | 202 ++++++++++++++++++ 6 files changed, 280 insertions(+), 20 deletions(-) create mode 100644 .changeset/validation-predicate-guards-and-operator-drift.md create mode 100644 test/object-validation-predicates.test.ts diff --git a/.changeset/validation-predicate-guards-and-operator-drift.md b/.changeset/validation-predicate-guards-and-operator-drift.md new file mode 100644 index 00000000..7c637128 --- /dev/null +++ b/.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 < 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. diff --git a/src/objects/account.object.ts b/src/objects/account.object.ts index b525f3f7..5a961aa7 100644 --- a/src/objects/account.object.ts +++ b/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 < 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 diff --git a/src/objects/campaign.object.ts b/src/objects/campaign.object.ts index 7f85f6f3..aafa838d 100644 --- a/src/objects/campaign.object.ts +++ b/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', diff --git a/src/objects/forecast.object.ts b/src/objects/forecast.object.ts index 54235ad0..b6148f3f 100644 --- a/src/objects/forecast.object.ts +++ b/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', diff --git a/src/objects/product.object.ts b/src/objects/product.object.ts index d56845cf..55ebad90 100644 --- a/src/objects/product.object.ts +++ b/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 < 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`, }, ], }); diff --git a/test/object-validation-predicates.test.ts b/test/object-validation-predicates.test.ts new file mode 100644 index 00000000..c4b25284 --- /dev/null +++ b/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 < 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; + +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(); + 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 = { + '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/); + }); +});