diff --git a/.changeset/console-3b2e4d98d904.md b/.changeset/console-3b2e4d98d904.md new file mode 100644 index 0000000000..eaaabf3961 --- /dev/null +++ b/.changeset/console-3b2e4d98d904.md @@ -0,0 +1,11 @@ +--- +"@objectstack/console": minor +--- + +Console (objectui) refreshed to `3b2e4d98d904`. Frontend changes in this range: + +- fix(list): route remaining system-field groupings through shared classifier (#2706) +- feat(console): user-import wizard defaults to the `auto` password policy (tracks framework#3236) (#2701) +- feat(flow-designer): schema-driven keyValue + numberList mapping (#3304) (#2708) + +objectui range: `0318118e02fd...3b2e4d98d904` diff --git a/.changeset/formula-null-guard-floor-date-arith-3306.md b/.changeset/formula-null-guard-floor-date-arith-3306.md new file mode 100644 index 0000000000..70438c1dfc --- /dev/null +++ b/.changeset/formula-null-guard-floor-date-arith-3306.md @@ -0,0 +1,15 @@ +--- +"@objectstack/formula": minor +--- + +**Stored `Field.formula` fields that compute dates/durations no longer silently evaluate to `null` (#3306).** Three independent CEL gaps made shipped template formulas (e.g. `hr_employee.tenure_years`, `hr_time_off_request.days`) return `null` with no parse/build/runtime error: + +1. **The null-guard idiom `cond ? : null` now compiles and evaluates.** cel-js's ternary type-unifier rejects a concrete `int`/`double`/`string` branch against `null` — so even `true ? 5 : null` faulted *"Ternary branches must have the same type"* and the whole formula nulled. A `Field.formula` is inherently nullable and the catalog blesses both ternary and `== null`, so this is the canonical "compute value, else blank" shape. An AST pre-pass (mirroring the #3183 temporal-equality rewrite) wraps the non-null branch in `dyn(...)` — value-preserving, null-branch-only, idempotent — so it type-checks and runs. Applied in `compile()`, `evaluate()`, and the build soundness check alike. + +2. **`floor(x)` / `ceil(x)` are now registered** (parallel to `round`/`abs`) and advertised in the catalog. They round toward −∞ / +∞, so `floor(-1.2) == -2` — NOT interchangeable with integer division's round-toward-zero. Previously `floor(...)` faulted `found no matching overload` and the formula nulled. + +3. **Date arithmetic is now a build-time ERROR instead of a silent runtime `null`.** `record.end_date - record.start_date + 1`, `today() + 30`, `record.date + n` type-check clean (operands are `dyn`) but always fault at runtime and never recover (a date string is not numeric, so hydration can't rescue it). The build soundness check now types `date`/`datetime` fields as `google.protobuf.Timestamp` and flags date/duration **arithmetic against a number** with a corrective message pointing at `daysBetween(a, b)` / `daysFromNow(n)` / `addDays(d, n)` / `addMonths(d, n)`. Sound by construction — ordering (`date < today()`, `date < "2026-01-01"` string-lex), equality (#3183), and string concatenation (`"Due: " + date`) are all runtime-tolerated and never flagged; only arithmetic against a number is. A `!= null` guard on a date field no longer masks the inner fault (`== null` no-op overloads registered in the check-only env). + +> **Heads-up for downstream:** (3) adds a NEW build-time error. A stored formula or predicate doing arithmetic on a `date`/`datetime` field (`end - start + 1`, `today() + 30`) that previously built (and nulled at runtime) will now fail `objectstack build` / `validateStackExpressions` with a message telling you to use `daysBetween` / `daysFromNow` / `addDays`. This only fires for genuinely-broken expressions that already returned `null`. + +Fixes #3306. diff --git a/.objectui-sha b/.objectui-sha index deb881863e..0ccbc49f39 100644 --- a/.objectui-sha +++ b/.objectui-sha @@ -1 +1 @@ -0318118e02fd0ef377492f3b09b687a84cccc908 +3b2e4d98d904d695a8372c394d46b81673011270 diff --git a/packages/formula/src/cel-engine.test.ts b/packages/formula/src/cel-engine.test.ts index f45e4b258c..91f723c39b 100644 --- a/packages/formula/src/cel-engine.test.ts +++ b/packages/formula/src/cel-engine.test.ts @@ -465,9 +465,17 @@ describe('celEngine', () => { expect(r).toEqual({ ok: true, value: true }); }); - it('abs / round / min / max', () => { + it('abs / round / floor / ceil / min / max', () => { expect(celEngine.evaluate(cel('abs(record.x)'), { record: { x: -3.5 } })).toEqual({ ok: true, value: 3.5 }); expect(celEngine.evaluate(cel('round(2.6)'), {})).toEqual({ ok: true, value: 3 }); + expect(celEngine.evaluate(cel('floor(6.7)'), {})).toEqual({ ok: true, value: 6 }); + expect(celEngine.evaluate(cel('ceil(6.1)'), {})).toEqual({ ok: true, value: 7 }); + // floor/ceil round toward −∞/+∞, NOT toward zero — the whole reason they are + // not interchangeable with integer division (#3306). + expect(celEngine.evaluate(cel('floor(-1.2)'), {})).toEqual({ ok: true, value: -2 }); + expect(celEngine.evaluate(cel('ceil(-1.2)'), {})).toEqual({ ok: true, value: -1 }); + // A record number field arrives as a cel-js `double`; `floor` must accept it. + expect(celEngine.evaluate(cel('floor(record.x / 365.0)'), { record: { x: 2318 } })).toEqual({ ok: true, value: 6 }); expect(celEngine.evaluate(cel('min(record.a, record.b)'), { record: { a: 3, b: 7 } })).toEqual({ ok: true, value: 3 }); expect(celEngine.evaluate(cel('max(record.a, record.b)'), { record: { a: 3, b: 7 } })).toEqual({ ok: true, value: 7 }); }); @@ -495,6 +503,7 @@ describe('celEngine', () => { daysBetween: 'daysBetween(today(), daysFromNow(7))', date: 'date("2026-03-15")', addDays: 'addDays(today(), 7)', addMonths: 'addMonths(today(), 3)', datetime: 'datetime("2026-03-15T08:00:00Z")', abs: 'abs(-3.5)', round: 'round(2.6)', + floor: 'floor(6.7)', ceil: 'ceil(6.1)', min: 'min(1, 2)', max: 'max(1, 2)', upper: 'upper("hi")', lower: 'lower("HI")', trim: 'trim(" x ")', contains: 'contains("hello", "ell")', startsWith: 'startsWith("hi", "h")', endsWith: 'endsWith("hi", "i")', matches: 'matches("a1", "a.")', joinNonEmpty: 'joinNonEmpty(["a", "b"], "-")', @@ -588,4 +597,46 @@ describe('celEngine', () => { .toEqual({ ok: true, value: false }); }); }); + + // #3306 — the blessed null-guard idiom `cond ? : null`. cel-js's ternary + // unifier rejects a concrete branch against `null`; the engine's AST rewrite + // wraps the non-null branch in `dyn(...)` so it compiles AND evaluates, and the + // null branch still yields null. + describe('null-guarded formula idiom (#3306)', () => { + const now = new Date('2026-07-20T00:00:00Z'); + + it('`cond ? : null` evaluates instead of silently nulling', () => { + expect(celEngine.evaluate(cel('true ? 5 : null'), {})).toEqual({ ok: true, value: 5 }); + expect(celEngine.evaluate(cel('false ? 5 : null'), {})).toEqual({ ok: true, value: null }); + // null-first order (either branch may be the null literal). + expect(celEngine.evaluate(cel('true ? null : 5'), {})).toEqual({ ok: true, value: null }); + }); + + it('compiles the idiom that used to fault type-checking', () => { + // Was: ERR[type] "Ternary branches must have the same type, got 'int' and 'null'". + expect(celEngine.compile('true ? 5 : null').ok).toBe(true); + expect(celEngine.compile('cond ? daysBetween(a, b) + 1 : null').ok).toBe(true); + }); + + it('the shipped hr templates now compute (tenure_years, time_off.days)', () => { + // tenure_years — daysBetween/365 integer division, null when hire_date unset. + const tenure = cel('record.hire_date != null ? daysBetween(record.hire_date, today()) / 365 : null'); + expect(celEngine.evaluate(tenure, { now, record: { hire_date: '2020-03-15' } })) + .toEqual({ ok: true, value: 6 }); + expect(celEngine.evaluate(tenure, { now, record: { hire_date: null } })) + .toEqual({ ok: true, value: null }); + // time_off.days — inclusive calendar-day span. + const days = cel('record.start_date != null && record.end_date != null ? daysBetween(record.start_date, record.end_date) + 1 : null'); + expect(celEngine.evaluate(days, { now, record: { start_date: '2026-06-20', end_date: '2026-06-24' } })) + .toEqual({ ok: true, value: 5 }); + }); + + it('leaves a genuine type mismatch and same-typed / non-null ternaries alone', () => { + // Not a null-guard → still an honest type error (we only relax `… : null`). + expect(celEngine.compile('true ? "a" : 5').ok).toBe(false); + // Same-typed branches never needed the rewrite and are unchanged. + expect(celEngine.evaluate(cel('true ? "a" : "b"'), {})).toEqual({ ok: true, value: 'a' }); + expect(celEngine.evaluate(cel('false ? 1 : 2'), {})).toEqual({ ok: true, value: 2 }); + }); + }); }); diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 9739e69110..f3d4161311 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -170,11 +170,50 @@ export function detectBareReference(source: string): string | null { /** * The CEL type a field is declared as for the Tier-4 type-soundness check - * (#1928). Deliberately coarse: only genuinely-scalar, non-numeric-intent - * fields are pinned to a concrete type; everything the runtime rescues stays - * `dyn` and can therefore never fault. See {@link firstTypeMismatch}. + * (#1928). Deliberately coarse: only genuinely-scalar fields whose *misuse + * always faults the runtime* are pinned to a concrete type; everything the + * runtime rescues stays `dyn` and can therefore never fault. + * - `string`/`bool` — text/boolean fields; arithmetic/ordering against a number + * faults (unless a text value happens to be numeric) → advisory warning. + * - `timestamp` — `date`/`datetime` fields; ARITHMETIC against a number always + * nulls at runtime (`date − date + 1`, `today() + 30`, #3306) → hard error. + * Ordering/equality/concatenation of a date field is runtime-tolerated and + * is never flagged (see {@link firstTypeMismatch}). + * See {@link firstTypeMismatch}. */ -export type FieldCelType = 'string' | 'bool' | 'dyn'; +export type FieldCelType = 'string' | 'bool' | 'timestamp' | 'dyn'; + +/** The cel-js type name a {@link FieldCelType} declares in a typed env. */ +function celTypeName(t: FieldCelType): string { + return t === 'timestamp' ? 'google.protobuf.Timestamp' : t; +} + +/** CEL types produced by date subtraction / temporal fns — arith on these nulls. */ +const DATE_CEL_TYPES = new Set(['google.protobuf.Timestamp', 'google.protobuf.Duration']); +/** Numeric CEL types — the RHS that turns date arithmetic into a runtime fault. */ +const NUMERIC_CEL_TYPES = new Set(['int', 'uint', 'double']); +/** Arithmetic operators (excludes ordering `< > <= >=`, which dates tolerate). */ +const ARITH_OPS = new Set(['+', '-', '*', '/', '%']); + +/** Concrete types the soundness env pins — each needs a `== null` overload below. */ +const NULL_GUARD_TYPES = ['string', 'bool', 'google.protobuf.Timestamp', 'google.protobuf.Duration']; + +/** + * cel-js has no ` == null` overload, so a `record. != null` + * guard faults the typed check for any pinned field — and because `check()` reports + * only its FIRST error, that fault MASKS a real arithmetic fault later in the same + * expression (the ubiquitous `guard ? : null` shape, #3306). Register + * no-op `== null` overloads (this env is check-only, never evaluated; `!=` desugars + * to `==`) so the guard type-checks and the real fault surfaces. `==`/`!=` are never + * flagged themselves ({@link UNSOUND_OVERLOAD_RE} excludes them), so this only + * unmasks — it can never introduce a false positive. + */ +function registerNullComparisons(env: Environment): void { + for (const t of NULL_GUARD_TYPES) { + try { env.registerOperator(`${t} == null`, () => false); } catch { /* already/invalid — ignore */ } + try { env.registerOperator(`null == ${t}`, () => false); } catch { /* already/invalid — ignore */ } + } +} /** * A `no such overload` fault for an ARITHMETIC (`+ - * / %`) or ORDERING @@ -213,13 +252,14 @@ function buildTypedEnv( limits: DEFAULT_LIMITS, }); registerStdLib(env, () => new Date(0)); + registerNullComparisons(env); for (const root of SCOPE_ROOTS) { try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ } } // Fields are bound bare at top level; a name that collides with a root // (unlikely) is skipped by the duplicate guard. for (const [name, t] of Object.entries(fieldCelTypes)) { - try { env.registerVariable(name, t); } catch { /* duplicate / reserved — ignore */ } + try { env.registerVariable(name, celTypeName(t)); } catch { /* duplicate / reserved — ignore */ } } return env; } @@ -229,8 +269,9 @@ function buildTypedEnv( limits: DEFAULT_LIMITS, }); registerStdLib(env, () => new Date(0)); + registerNullComparisons(env); const fields: Record = {}; - for (const [name, t] of Object.entries(fieldCelTypes)) fields[name] = t; + for (const [name, t] of Object.entries(fieldCelTypes)) fields[name] = celTypeName(t); try { env.registerType('OsRecordScope', { fields }); } catch { /* invalid field name — ignore */ } // The record namespaces carry the typed struct; every other root stays a // `map` (dyn members) so a reference through it never faults. @@ -277,15 +318,26 @@ function offendingField( * as a NON-blocking warning. * * Soundness (the ADR-0032 design law — never flag what the runtime tolerates): - * - Number / currency / percent / date / datetime fields are declared `dyn`, - * because the runtime rescues every mixed case for them — `registerOperator` - * for `double`×`int` arithmetic and the string-hydration retry for - * numeric-string / ISO-date values — so they can never fault here. + * - Number / currency / percent fields are declared `dyn`, because the runtime + * rescues every mixed case for them — `registerOperator` for `double`×`int` + * arithmetic and the string-hydration retry for numeric strings — so they can + * never fault here. + * - `date`/`datetime` fields are `timestamp`, but ONLY ARITHMETIC (`+ − * / %`) + * against a NUMBER is flagged (#3306): `date − date + 1`, `date + n`, `today() + * + 30` all null at runtime and never recover (a date string is not numeric, + * so hydration can't rescue it). Ordering (`date < today()` → Timestamp` sites; `'flattened'` for bare-field flow/automation @@ -295,28 +347,40 @@ export function firstTypeMismatch( source: string, fieldCelTypes: Readonly>, scope: 'record' | 'flattened' = 'record', -): { operator: string; operands: string; celType: FieldCelType; field: string | null } | null { +): { operator: string; operands: string; celType: FieldCelType; field: string | null; category: 'date-arith' | 'type-mismatch' } | null { if (typeof source !== 'string' || !source.trim()) return null; // An all-`dyn` record can never fault an overload — skip the parse entirely. - if (!Object.values(fieldCelTypes).some((t) => t === 'string' || t === 'bool')) return null; + if (!Object.values(fieldCelTypes).some((t) => t === 'string' || t === 'bool' || t === 'timestamp')) return null; try { + // A null-guarded numeric branch (`cond ? n : null`) faults cel-js's ternary + // unifier *before* the check reaches an inner date-arith overload; rewrite it + // first (same pass the runtime uses) so the real fault surfaces (#3306). const env = buildTypedEnv(fieldCelTypes, scope); - const result = env.parse(source).check?.() as + const result = env.parse(rewriteNullableTernary(source)).check?.() as | { valid?: boolean; error?: { message?: string } } | undefined; if (!result || result.valid !== false) return null; const m = UNSOUND_OVERLOAD_RE.exec(result.error?.message ?? ''); if (!m) return null; const operator = m[2]; + const operands = `${m[1]} ${operator} ${m[3]}`; + // #3306 — date/duration ARITHMETIC against a number: always nulls, hard error. + if (ARITH_OPS.has(operator) + && ((DATE_CEL_TYPES.has(m[1]) && NUMERIC_CEL_TYPES.has(m[3])) + || (NUMERIC_CEL_TYPES.has(m[1]) && DATE_CEL_TYPES.has(m[3])))) { + return { + operator, operands, celType: 'timestamp', category: 'date-arith', + field: offendingField(source, fieldCelTypes, 'timestamp', scope), + }; + } + // #1928 — text/boolean field in arithmetic/ordering against a number: warning. const celType: FieldCelType | null = m[1] === 'string' || m[1] === 'bool' ? (m[1] as FieldCelType) : m[3] === 'string' || m[3] === 'bool' ? (m[3] as FieldCelType) : null; if (!celType) return null; return { - operator, - operands: `${m[1]} ${operator} ${m[3]}`, - celType, + operator, operands, celType, category: 'type-mismatch', field: offendingField(source, fieldCelTypes, celType, scope), }; } catch { @@ -437,6 +501,95 @@ function rememberRewrite(source: string, rewritten: string): void { temporalRewriteCache.set(source, rewritten); } +/** True when `node` is the CEL `null` literal (`{ op: 'value', args: null }`). */ +function isNullLiteral(node: unknown): boolean { + return isCelNode(node) && node.op === 'value' && node.args === null; +} + +/** True when `node` is already a `dyn(...)` call — so the wrap is idempotent. */ +function isDynCall(node: unknown): boolean { + return isCelNode(node) && node.op === 'call' + && Array.isArray(node.args) && node.args[0] === 'dyn'; +} + +/** Wrap a branch in `dyn(...)` so a concrete-typed branch unifies with `null`. */ +function wrapInDyn(node: CelNode): CelNode { + return { op: 'call', args: ['dyn', [node]] }; +} + +/** + * #3306 — make the blessed null-guard idiom `cond ? : null` compile and + * evaluate. cel-js's ternary type-unifier requires both branches to share a type, + * and a concrete `int`/`double`/`string` branch does NOT unify with `null` — so + * even `true ? 5 : null` faults *"Ternary branches must have the same type"* and + * the whole formula silently evaluates to null. But a `Field.formula` is inherently + * nullable — `guard ? value : null` is the canonical "compute value, else blank" + * shape (and the catalog blesses both ternary and `== null`). We restore it by + * wrapping the non-null branch in `dyn(...)`: `dyn(x)` returns `x` unchanged at + * runtime and only relaxes its STATIC type to `dyn`, which unifies with `null`. + * + * The rewrite is: + * - **null-only** — fires ONLY when exactly one ternary branch is a `null` + * literal, so a genuine mismatch (`cond ? "a" : 5`) is left to error as before; + * - **value-preserving** — `dyn(x)` never changes the runtime value, and the + * wrapped branch's own sub-expression is still type-checked (an inner + * date-arith fault still surfaces — the soundness gate relies on this); + * - **idempotent** — a branch already `dyn(...)` (or itself `null`) is not + * re-wrapped, and nested ternaries are handled by the recursive walk. + * + * Returns the (possibly rewritten) source; only reserializes when a rewrite + * actually happened. Memoized; a parse fault returns the source unchanged. + */ +export function rewriteNullableTernary(source: string): string { + if (typeof source !== 'string' || !source.trim()) return source; + const cached = nullableTernaryCache.get(source); + if (cached !== undefined) return cached; + // Cheap gate: a rewrite needs a ternary AND a `null` literal branch. + if (!source.includes('?') || !source.includes('null')) { + rememberNullableRewrite(source, source); + return source; + } + let ast: unknown; + try { + ast = (recordScopeEnv ??= buildScopedEnv([])).parse(source).ast; + } catch { + rememberNullableRewrite(source, source); + return source; + } + let changed = false; + const visit = (node: unknown): void => { + if (!isCelNode(node)) return; + if (node.op === '?:' && Array.isArray(node.args) && node.args.length === 3) { + const args = node.args as unknown[]; + const left = args[1]; + const right = args[2]; + // Exactly one branch is `null` → wrap the other so the pair unifies to `dyn`. + // Skip when the non-null branch is already `dyn(...)` or itself a null literal. + if (isNullLiteral(right) && !isNullLiteral(left) && isCelNode(left) && !isDynCall(left)) { + args[1] = wrapInDyn(left); changed = true; + } else if (isNullLiteral(left) && !isNullLiteral(right) && isCelNode(right) && !isDynCall(right)) { + args[2] = wrapInDyn(right); changed = true; + } + } + if (Array.isArray(node.args)) for (const child of node.args) visit(child); + }; + visit(ast); + const out = changed ? serialize(ast as Parameters[0]) : source; + rememberNullableRewrite(source, out); + return out; +} + +/** Bounded memo of source → null-guard-rewritten source (#3306). */ +const nullableTernaryCache = new Map(); +const NULLABLE_TERNARY_CACHE_MAX = 500; +function rememberNullableRewrite(source: string, rewritten: string): void { + if (nullableTernaryCache.size >= NULLABLE_TERNARY_CACHE_MAX) { + const first = nullableTernaryCache.keys().next().value; + if (first !== undefined) nullableTernaryCache.delete(first); + } + nullableTernaryCache.set(source, rewritten); +} + /** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */ function coerce(value: unknown): unknown { if (typeof value === 'bigint') { @@ -541,7 +694,10 @@ export const celEngine: DialectEngine = { // We use a wall-clock now() here purely for parse-time stdlib // type-checking; the function is never actually called. const env = buildEnv(() => new Date(0)); - const compiled = env.parse(source); + // #3306 — accept the null-guard idiom `cond ? : null`, which cel-js's + // ternary unifier otherwise rejects (`int`/`double`/`string` ≠ `null`). Same + // rewrite the runtime uses, so build and eval agree on what is valid. + const compiled = env.parse(rewriteNullableTernary(source)); // Surface check errors eagerly. cel-js's `check()` returns a // `TypeCheckResult` object (`{ valid, type?, error? }`) — NOT an array — // so the type fault (including `found no matching overload for 'PRIOR(dyn)'` @@ -590,7 +746,10 @@ export const celEngine: DialectEngine = { // temporal function (`date(record.d) == today()`), so a `Field.date` string // matches the Timestamp instead of silently never equalling it. No-op (and // no reserialize) for any source without such a comparison. - const evalSource = rewriteTemporalEquality(source); + // #3306 — then relax the null-guard idiom `cond ? : null` so a + // nullable numeric/string formula evaluates instead of faulting cel-js's + // ternary unifier. Both rewrites are no-ops for sources that don't need them. + const evalSource = rewriteNullableTernary(rewriteTemporalEquality(source)); try { const raw = env.evaluate(evalSource, scope); return { ok: true, value: coerce(raw) as T }; diff --git a/packages/formula/src/stdlib.ts b/packages/formula/src/stdlib.ts index bfc242f56f..ab4a228097 100644 --- a/packages/formula/src/stdlib.ts +++ b/packages/formula/src/stdlib.ts @@ -202,6 +202,14 @@ export function registerStdLib( // ── Numbers ────────────────────────────────────────────────────────── .registerFunction('abs(dyn): double', (x: unknown) => Math.abs(Number(x))) .registerFunction('round(dyn): int', (x: unknown) => BigInt(Math.round(Number(x)))) + // `floor`/`ceil` round toward −∞ / +∞ (NOT toward zero like integer division), + // so `floor(-1.2) == -2`, `ceil(-1.2) == -1`. Registered because they are + // universally-expected math primitives that authors (and LLMs) reach for by + // reflex — omitting them makes `floor(...)` fault and the formula silently + // null (#3306). Return `int` to match `round`, coercing the arg with + // `Number(...)` (a record number arrives as cel-js `double`). + .registerFunction('floor(dyn): int', (x: unknown) => BigInt(Math.floor(Number(x)))) + .registerFunction('ceil(dyn): int', (x: unknown) => BigInt(Math.ceil(Number(x)))) // min/max return the smaller/larger operand verbatim (type preserved) rather // than a coerced copy, so `min(record.a, record.b)` keeps int-ness when both // are ints. Comparison is numeric. diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index 3d2e9406fb..e363361e27 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -269,6 +269,70 @@ describe('validateExpression (ADR-0032)', () => { }); }); + // #3306 — date arithmetic (`date − date + 1`, `date + n`) type-checks clean (the + // operands are `dyn` at compile) but ALWAYS nulls at runtime and never recovers, + // so — unlike the advisory text/bool warning — it is a HARD ERROR that blocks the + // build. Only arithmetic is flagged; ordering / equality / concatenation of a + // date field are runtime-tolerated and stay clean (the design law). + describe('date-arithmetic errors (#3306)', () => { + const schema = { + objectName: 'hr_time_off_request', + fields: ['start_date', 'end_date', 'hire_date', 'note'] as const, + fieldTypes: { start_date: 'date', end_date: 'date', hire_date: 'datetime', note: 'text' }, + scope: 'record', + } as const; + + it('errors on `date − date + n` — the shipped `time_off.days` bug', () => { + const r = validateExpression('value', '(record.end_date - record.start_date) + 1', schema); + expect(r.ok).toBe(false); + expect(r.errors).toHaveLength(1); + expect(r.errors[0].message).toMatch(/date arithmetic/i); + expect(r.errors[0].message).toMatch(/daysBetween/); + }); + + it('errors even when the arithmetic is behind a `!= null` guard (the real template shape)', () => { + // The `!= null` guard on a date field must not mask the inner arithmetic fault. + const r = validateExpression( + 'value', + 'record.start_date != null && record.end_date != null ? (record.end_date - record.start_date) + 1 : null', + schema, + ); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toMatch(/date arithmetic/i); + }); + + it('errors on `date + n` (author meant `addDays`)', () => { + expect(validateExpression('value', 'record.hire_date + 30', schema).ok).toBe(false); + }); + + it('does NOT flag runtime-tolerated date uses (ordering, string-literal, equality, concat)', () => { + // Ordering vs a temporal fn → Timestamp { + const r = validateExpression( + 'value', + 'record.start_date != null && record.end_date != null ? daysBetween(record.start_date, record.end_date) + 1 : null', + schema, + ); + expect(r.ok).toBe(true); + expect(r.errors).toHaveLength(0); + }); + }); + describe('introspection', () => { it('reports the dialect + scope for a field role', () => { expect(expectedDialect('predicate')).toBe('cel'); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 868f634d57..1fd6dd92a0 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -92,13 +92,12 @@ export interface ExprValidationResult { } /** - * #1928 tier 4 — spec field type → the CEL type it is declared as for the - * type-soundness check. ONLY genuinely-scalar, non-numeric-intent types are - * pinned to a concrete type (`string` / `bool`); every other type — numbers, - * dates, selects (option values may be numeric codes), lookups, media, JSON — - * maps to `dyn` so it can never fault (the runtime rescues all of those). Any - * field type absent from this map is treated as `dyn`. Keeping the map narrow - * is the source of the check's near-zero false-positive rate. + * #1928 / #3306 — spec field type → the CEL type it is declared as for the + * type-soundness check. Pinned to a concrete type ONLY where a specific misuse + * always faults the runtime; numbers/currency/percent, selects (option values + * may be numeric codes), lookups, media, JSON stay `dyn` because the runtime + * rescues them. Any field type absent from this map is treated as `dyn`. Keeping + * the map narrow is the source of the check's near-zero false-positive rate. */ const SPEC_TYPE_TO_CEL: Readonly> = { // Free text — arithmetic / ordering against a number is (almost) always a bug. @@ -106,6 +105,10 @@ const SPEC_TYPE_TO_CEL: Readonly> = { phone: 'string', markdown: 'string', html: 'string', richtext: 'string', // Booleans — arithmetic / ordering against a number ALWAYS faults at runtime. boolean: 'bool', toggle: 'bool', + // Dates — ARITHMETIC against a number always nulls (`date − date + 1`, `date + n`, + // #3306). Only arithmetic is flagged; ordering / equality / concatenation of a + // date field are runtime-tolerated (see `firstTypeMismatch`). + date: 'timestamp', datetime: 'timestamp', }; /** Map an object's field-type hints onto the CEL types the soundness check uses. */ @@ -118,29 +121,51 @@ function toCelFieldTypes(fieldTypes: Readonly>): Record` vs bare - * field binding, and shapes the referenced form in the message. + * #1928 / #3306 — a type-soundness verdict for an expression, or `null` when + * sound. Two categories, with different severities: + * - `date-arith` (**error**): arithmetic on a date field against a number + * (`date − date + 1`, `today() + 30`) — always nulls at runtime and never + * recovers, so it blocks the build. + * - `type-mismatch` (**warning**): a text/boolean field used with an + * arithmetic/ordering operator against a number — nulls unless the text value + * happens to be numeric, so it stays advisory. + * `scope` selects `record.` vs bare field binding, shaping the message. */ -function typeSoundnessWarning( +function typeSoundnessIssue( source: string, fieldTypes: Readonly>, scope: 'record' | 'flattened', -): ExprValidationError | null { +): { issue: ExprValidationError; severity: 'error' | 'warning' } | null { const mismatch = firstTypeMismatch(source, toCelFieldTypes(fieldTypes), scope); if (!mismatch) return null; - const held = mismatch.celType === 'bool' ? 'a boolean' : 'text'; const ref = mismatch.field ? (scope === 'record' ? `\`record.${mismatch.field}\`` : `\`${mismatch.field}\``) : null; + if (mismatch.category === 'date-arith') { + const subject = ref ? `${ref} is a date` : 'a date field'; + return { + severity: 'error', + issue: { + source, + message: + `date arithmetic \`${mismatch.operands}\` — ${subject}, and CEL can't do arithmetic on ` + + `dates: this faults at runtime, so the field silently evaluates to null. Use ` + + `\`daysBetween(a, b)\` for the span in whole days, and \`daysFromNow(n)\` / ` + + `\`addDays(d, n)\` / \`addMonths(d, n)\` to shift a date.`, + }, + }; + } + const held = mismatch.celType === 'bool' ? 'a boolean' : 'text'; const subject = ref ? `${ref} holds ${held}` : `${held === 'a boolean' ? 'a boolean' : 'a text'} field`; return { - source, - message: - `type mismatch \`${mismatch.operands}\` — ${subject} but is used with \`${mismatch.operator}\` ` + - `against a number. This faults at runtime, so the expression silently evaluates to null ` + - `(unless the value happens to be numeric). Use a number field, or drop the arithmetic/comparison.`, + severity: 'warning', + issue: { + source, + message: + `type mismatch \`${mismatch.operands}\` — ${subject} but is used with \`${mismatch.operator}\` ` + + `against a number. This faults at runtime, so the expression silently evaluates to null ` + + `(unless the value happens to be numeric). Use a number field, or drop the arithmetic/comparison.`, + }, }; } @@ -323,15 +348,14 @@ export function validateExpression( `expression silently evaluates to null. Write \`record.${bare}\`.`, }); } else if (schema.fieldTypes) { - // #1928 tier 4 — with per-field types in hand, flag a text/boolean field - // used with an arithmetic/ordering operator against a number: it faults - // the runtime overload and the expression silently evaluates to null. - // Advisory (never blocks the build): the runtime CAN succeed if a text - // value happens to be numeric, so this is a warning, not an error. Only - // runs when there is no bare-ref error (the typed check needs the - // canonical `record.` form). - const w = typeSoundnessWarning(source, schema.fieldTypes, 'record'); - if (w) warnings.push(w); + // #1928 / #3306 — with per-field types in hand, flag a type-unsound + // operator use that faults at runtime and silently nulls: a text/boolean + // field arithmetic'd/ordered against a number (advisory warning — a + // numeric text value can succeed), or date-field arithmetic (hard error — + // always nulls). Only runs when there is no bare-ref error (the typed + // check needs the canonical `record.` form). + const r = typeSoundnessIssue(source, schema.fieldTypes, 'record'); + if (r) (r.severity === 'error' ? errors : warnings).push(r.issue); } } else if (schema?.fields && schema.fields.length > 0) { // Flattened flow/automation condition: the record's fields ARE bound at @@ -352,13 +376,13 @@ export function validateExpression( }); } } - // #1928 tier 4 — the same type-soundness check, for bare-field conditions: - // a text/boolean field compared/arithmetic'd against a number faults at - // runtime. Flow variables stay `dyn` (never flagged); equality is - // runtime-safe (never flagged). Advisory only. + // #1928 / #3306 — the same type-soundness check, for bare-field conditions: + // a text/boolean field compared/arithmetic'd against a number (advisory), or + // date-field arithmetic (error). Flow variables stay `dyn` (never flagged); + // equality/ordering of a date is runtime-safe (never flagged). if (schema.fieldTypes) { - const w = typeSoundnessWarning(source, schema.fieldTypes, 'flattened'); - if (w) warnings.push(w); + const r = typeSoundnessIssue(source, schema.fieldTypes, 'flattened'); + if (r) (r.severity === 'error' ? errors : warnings).push(r.issue); } } } @@ -445,7 +469,7 @@ export const CEL_STDLIB_FUNCTIONS: string[] = [ // Dates (registered stdlib) 'now', 'today', 'daysFromNow', 'daysAgo', 'daysBetween', 'addDays', 'addMonths', 'date', 'datetime', // Numbers (registered stdlib) - 'abs', 'round', 'min', 'max', + 'abs', 'round', 'floor', 'ceil', 'min', 'max', // Strings (registered stdlib) 'upper', 'lower', 'trim', 'contains', 'startsWith', 'endsWith', 'matches', 'joinNonEmpty', // Collections / null-ish (registered stdlib) diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index ebbf0e72c6..57ea136ff8 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -178,6 +178,47 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues).toHaveLength(0); }); + // #3306 — a formula field doing date arithmetic type-checks clean (dyn operands) + // but nulls at runtime. The stack gate must turn it RED with a corrective + // message — this is the exact shape that shipped in the `hr` template. + it('flags a formula field that does date arithmetic (the shipped time_off.days bug)', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'hr_time_off_request', + fields: { + start_date: { type: 'date' }, + end_date: { type: 'date' }, + days: { + type: 'formula', name: 'days', + formula: 'record.start_date != null && record.end_date != null ? (record.end_date - record.start_date) + 1 : null', + }, + }, + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('error'); + expect(issues[0].where).toContain("field 'days' formula"); + expect(issues[0].message).toMatch(/date arithmetic/i); + expect(issues[0].message).toMatch(/daysBetween/); + }); + + it('accepts the daysBetween rewrite of that formula', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'hr_time_off_request', + fields: { + start_date: { type: 'date' }, + end_date: { type: 'date' }, + days: { + type: 'formula', name: 'days', + formula: 'record.start_date != null && record.end_date != null ? daysBetween(record.start_date, record.end_date) + 1 : null', + }, + }, + }], + }); + expect(issues).toHaveLength(0); + }); + it('does NOT flag bare references in a flow condition (flattened scope)', () => { const issues = validateStackExpressions({ objects: [{ name: 'crm_opportunity', fields: { stage: { type: 'select' }, amount: { type: 'currency' } } }], diff --git a/skills/objectstack-formula/SKILL.md b/skills/objectstack-formula/SKILL.md index ab15d8d4f3..a52994dc55 100644 --- a/skills/objectstack-formula/SKILL.md +++ b/skills/objectstack-formula/SKILL.md @@ -143,12 +143,15 @@ test asserts every entry resolves at runtime, so this table stays in sync with i | `addMonths(d, n)` | timestamp | Shift **any** date by `n` months; clamps to month-end (`addMonths(date('2026-01-31'), 1)` → Feb 28) | | `date(s)` / `datetime(s)` | timestamp | Parse an ISO date / date-time string to a timestamp | +> **No date arithmetic.** Do NOT write `end - start`, `date + n`, or `today() + 30` — CEL has no numeric arithmetic on dates, so these fault and the field silently nulls (the build now rejects them). Use `daysBetween(start, end)` for a span in days, and `daysFromNow(n)` / `addDays(d, n)` / `addMonths(d, n)` to shift a date. Inclusive day span: `daysBetween(record.start_date, record.end_date) + 1`. Tenure in years: `daysBetween(record.hire_date, today()) / 365`. + **Numbers** | Function | Returns | Notes | |:---|:---|:---| | `abs(x)` | double | Absolute value | | `round(x)` | int | Round to the nearest integer | +| `floor(x)` / `ceil(x)` | int | Round toward −∞ / +∞ (`floor(-1.2)` = −2, not −1) | | `min(a, b)` / `max(a, b)` | dyn | Smaller / larger operand (numeric comparison) | **Strings**