Skip to content

Commit 5143d9d

Browse files
committed
feat(formula,lint): advisory type-soundness warnings for expressions (#1928 tier 4)
A `Field.formula` or record-scoped predicate that uses a text or boolean field with an arithmetic (`+ - * / %`) or ordering (`< > <= >=`) operator against a number faults the runtime overload and silently evaluates to null (e.g. `record.title * 2`, `record.is_active + 1`). The build now surfaces this as a NON-blocking warning naming the offending field. Honours the ADR-0032 design law — flag only what the runtime would also fail: number/currency/percent/date/datetime fields are declared `dyn`, so the cases the runtime rescues never warn (`amount / 100` via registerOperator, `due == today()` and numeric-string/ISO-date values via the string-hydration retry, numeric-coded select options). Equality (`==`/`!=`) is excluded — a heterogeneous equality is runtime-safe. - formula: new `firstTypeMismatch` + optional `fieldTypes` hint on `validateExpression`; a narrow spec-type -> CEL-type map (only free-text -> string, boolean/toggle -> bool; everything else dyn). - lint: `validateStackExpressions` threads each object's field types into every record-scoped site (formula fields, validations, action/hook/sharing predicates). Warnings are advisory in build/validate, fatal only under --strict. Tests: formula 215 (+9), lint 230 (+3). Green on cel-js 8.0.0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hnji7EEYR2mGt6pY53a8Bm
1 parent e057f42 commit 5143d9d

6 files changed

Lines changed: 369 additions & 3 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
"@objectstack/formula": minor
3+
"@objectstack/lint": minor
4+
---
5+
6+
feat(formula,lint): advisory type-soundness warnings for formula/predicate expressions (#1928 tier 4)
7+
8+
Closes the last open guardrail from #1928. A `Field.formula` or record-scoped
9+
predicate that uses a **text or boolean field with an arithmetic (`+ - * / %`)
10+
or ordering (`< > <= >=`) operator against a number** faults the runtime
11+
overload and silently evaluates to `null` (e.g. `record.title * 2`,
12+
`record.is_active + 1`). The build now surfaces this as a **non-blocking
13+
warning** with the offending field and a corrective message.
14+
15+
Honours the ADR-0032 design law — the checker only flags what the runtime
16+
would also fail:
17+
18+
- Number / currency / percent / date / datetime fields are declared `dyn`, so
19+
the cases the runtime rescues never warn — `record.amount / 100` (the #1930
20+
`registerOperator` fix), `record.due == today()` and numeric-string / ISO-date
21+
values (the string-hydration retry), and numeric-coded `select` option values.
22+
- Equality (`==` / `!=`) is excluded: a heterogeneous equality is runtime-safe
23+
(evaluates to `false`), never a fault.
24+
25+
New `firstTypeMismatch` export in `@objectstack/formula` (and an optional
26+
`fieldTypes` hint on `validateExpression`); `@objectstack/lint`'s
27+
`validateStackExpressions` threads each object's field types into every
28+
record-scoped site (formula fields, validation rules, action / hook / sharing
29+
predicates). Warnings are advisory in `objectstack build` / `validate`
30+
(fatal only under `--strict`), matching the tier-3 channel.

packages/formula/src/cel-engine.ts

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,125 @@ export function detectBareReference(source: string): string | null {
167167
return firstUndeclaredReference(source);
168168
}
169169

170+
/**
171+
* The CEL type a field is declared as for the Tier-4 type-soundness check
172+
* (#1928). Deliberately coarse: only genuinely-scalar, non-numeric-intent
173+
* fields are pinned to a concrete type; everything the runtime rescues stays
174+
* `dyn` and can therefore never fault. See {@link firstTypeMismatch}.
175+
*/
176+
export type FieldCelType = 'string' | 'bool' | 'dyn';
177+
178+
/**
179+
* A `no such overload` fault for an ARITHMETIC (`+ - * / %`) or ORDERING
180+
* (`< > <= >=`) operator, with the two operand types captured. Equality
181+
* (`==` / `!=`) is intentionally excluded: cel-js's checker faults on a
182+
* heterogeneous equality (`string == int`) but the runtime evaluates it
183+
* cleanly to `false` — so a fault there is NOT a runtime failure and must not
184+
* warn. Linear (no nested quantifiers) — no ReDoS. Operand types are `[\w.]+`
185+
* (e.g. `string`, `int`, `google.protobuf.Timestamp`); the operator token is
186+
* punctuation, so the two never overlap.
187+
*/
188+
const UNSOUND_OVERLOAD_RE = /no such overload:\s*([\w.]+)\s*(<=|>=|<|>|\+|-|\*|\/|%)\s*([\w.]+)/;
189+
190+
/**
191+
* A `record`-typed environment where each field carries a concrete CEL type
192+
* (`string`/`bool`) or `dyn`. Member access (`record.<field>`) then resolves to
193+
* the field's type, so cel-js's checker faults an arithmetic/ordering operator
194+
* applied across incompatible types. Built per call — cheap, and only used at
195+
* build time.
196+
*/
197+
function buildTypedRecordEnv(fieldCelTypes: Readonly<Record<string, FieldCelType>>): Environment {
198+
const env = new Environment({
199+
unlistedVariablesAreDyn: false,
200+
enableOptionalTypes: true,
201+
limits: DEFAULT_LIMITS,
202+
});
203+
registerStdLib(env, () => new Date(0));
204+
const fields: Record<string, string> = {};
205+
for (const [name, t] of Object.entries(fieldCelTypes)) fields[name] = t;
206+
try { env.registerType('OsRecordScope', { fields }); } catch { /* invalid field name — ignore */ }
207+
// The record namespaces carry the typed struct; every other root stays a
208+
// `map` (dyn members) so a reference through it never faults.
209+
for (const root of ['record', 'previous', 'input']) {
210+
try { env.registerVariable(root, 'OsRecordScope'); } catch { /* duplicate — ignore */ }
211+
}
212+
for (const root of SCOPE_ROOTS) {
213+
try { env.registerVariable(root, 'map'); } catch { /* already typed above / duplicate — ignore */ }
214+
}
215+
return env;
216+
}
217+
218+
/**
219+
* The first `record.<field>` (or `previous.`/`input.`) reference in `source`
220+
* whose declared CEL type matches `celType` — best-effort attribution of an
221+
* overload fault to the offending field. Returns `null` if none is found.
222+
*/
223+
function offendingField(
224+
source: string,
225+
fieldCelTypes: Readonly<Record<string, FieldCelType>>,
226+
celType: FieldCelType,
227+
): string | null {
228+
for (const [name, t] of Object.entries(fieldCelTypes)) {
229+
if (t !== celType) continue;
230+
// Word-bounded so `amount` does not match `amount_total`.
231+
if (new RegExp(`(?:record|previous|input)\\.${name}(?![\\w$])`).test(source)) return name;
232+
}
233+
return null;
234+
}
235+
236+
/**
237+
* Tier-4 type-soundness (#1928): detect a `record`-scoped expression that
238+
* type-checks structurally but faults a runtime operator overload because a
239+
* text (`string`) or boolean (`bool`) field is used with an arithmetic or
240+
* ordering operator against a number. Such an expression evaluates to `null`
241+
* at runtime (unless the text value happens to be numeric), so it is surfaced
242+
* as a NON-blocking warning.
243+
*
244+
* Soundness (the ADR-0032 design law — never flag what the runtime tolerates):
245+
* - Number / currency / percent / date / datetime fields are declared `dyn`,
246+
* because the runtime rescues every mixed case for them — `registerOperator`
247+
* for `double`×`int` arithmetic and the string-hydration retry for
248+
* numeric-string / ISO-date values — so they can never fault here.
249+
* - Equality (`==` / `!=`) is excluded ({@link UNSOUND_OVERLOAD_RE}): a
250+
* heterogeneous equality is runtime-safe.
251+
*
252+
* Returns the operand types, the faulting operator, the concrete operand CEL
253+
* type, and (best-effort) the offending field — or `null` when type-sound.
254+
*/
255+
export function firstTypeMismatch(
256+
source: string,
257+
fieldCelTypes: Readonly<Record<string, FieldCelType>>,
258+
): { operator: string; operands: string; celType: FieldCelType; field: string | null } | null {
259+
if (typeof source !== 'string' || !source.trim()) return null;
260+
// An all-`dyn` record can never fault an overload — skip the parse entirely.
261+
if (!Object.values(fieldCelTypes).some((t) => t === 'string' || t === 'bool')) return null;
262+
try {
263+
const env = buildTypedRecordEnv(fieldCelTypes);
264+
const result = env.parse(source).check?.() as
265+
| { valid?: boolean; error?: { message?: string } }
266+
| undefined;
267+
if (!result || result.valid !== false) return null;
268+
const m = UNSOUND_OVERLOAD_RE.exec(result.error?.message ?? '');
269+
if (!m) return null;
270+
const operator = m[2];
271+
const celType: FieldCelType | null =
272+
m[1] === 'string' || m[1] === 'bool' ? (m[1] as FieldCelType)
273+
: m[3] === 'string' || m[3] === 'bool' ? (m[3] as FieldCelType)
274+
: null;
275+
if (!celType) return null;
276+
return {
277+
operator,
278+
operands: `${m[1]} ${operator} ${m[3]}`,
279+
celType,
280+
field: offendingField(source, fieldCelTypes, celType),
281+
};
282+
} catch {
283+
// A parse/other fault is the syntax checker's job (celEngine.compile); this
284+
// helper only reports a clean type-soundness verdict.
285+
return null;
286+
}
287+
}
288+
170289
/** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */
171290
function coerce(value: unknown): unknown {
172291
if (typeof value === 'bigint') {

packages/formula/src/validate.test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,82 @@ describe('validateExpression (ADR-0032)', () => {
153153
});
154154
});
155155

156+
// #1928 tier 4 — a text/boolean field used with an arithmetic or ordering
157+
// operator against a number faults at runtime (silent null). With per-field
158+
// types the validator surfaces this as a NON-blocking warning, and — the
159+
// design law — never flags a case the runtime tolerates (number/date fields,
160+
// equality, string concat, null-guards).
161+
describe('type-soundness warnings (#1928 tier 4)', () => {
162+
const schema = {
163+
objectName: 'crm_opportunity',
164+
fields: ['name', 'amount', 'is_active', 'due', 'priority', 'title'] as const,
165+
fieldTypes: {
166+
name: 'text', title: 'textarea', amount: 'currency',
167+
is_active: 'boolean', due: 'date', priority: 'select',
168+
},
169+
scope: 'record',
170+
} as const;
171+
172+
it('warns (does not error) on a text field used in arithmetic against a number', () => {
173+
const r = validateExpression('value', 'record.name * 2', schema);
174+
expect(r.ok).toBe(true);
175+
expect(r.errors).toHaveLength(0);
176+
expect(r.warnings).toHaveLength(1);
177+
expect(r.warnings[0].message).toMatch(/type mismatch/i);
178+
expect(r.warnings[0].message).toMatch(/record\.name/);
179+
expect(r.warnings[0].message).toMatch(/evaluates to null/);
180+
});
181+
182+
it('warns on a text field ordered against a number', () => {
183+
const r = validateExpression('predicate', 'record.title >= 5', schema);
184+
expect(r.ok).toBe(true);
185+
expect(r.warnings).toHaveLength(1);
186+
expect(r.warnings[0].message).toMatch(/record\.title/);
187+
});
188+
189+
it('warns on a boolean field used in arithmetic (always faults at runtime)', () => {
190+
const r = validateExpression('value', 'record.is_active + 1', schema);
191+
expect(r.ok).toBe(true);
192+
expect(r.warnings).toHaveLength(1);
193+
expect(r.warnings[0].message).toMatch(/boolean/i);
194+
expect(r.warnings[0].message).toMatch(/record\.is_active/);
195+
});
196+
197+
it('does NOT warn on number/currency arithmetic with an int literal (#1930 runtime fix)', () => {
198+
// currency → dyn, so `amount / 100`, `amount * 2 - 50` never fault.
199+
expect(validateExpression('value', 'record.amount / 100', schema).warnings).toHaveLength(0);
200+
expect(validateExpression('value', 'record.amount * 2 - 50', schema).warnings).toHaveLength(0);
201+
});
202+
203+
it('does NOT warn on a date field compared to today()/daysFromNow()', () => {
204+
expect(validateExpression('predicate', 'record.due <= daysFromNow(30)', schema).warnings).toHaveLength(0);
205+
expect(validateExpression('predicate', 'record.due == today()', schema).warnings).toHaveLength(0);
206+
});
207+
208+
it('does NOT warn on a select field ordered against a number (option values may be numeric codes)', () => {
209+
// select → dyn, so `priority >= 3` (a numeric-coded picklist) is not flagged.
210+
expect(validateExpression('predicate', 'record.priority >= 3', schema).warnings).toHaveLength(0);
211+
});
212+
213+
it('does NOT warn on heterogeneous equality (runtime-safe, returns false)', () => {
214+
expect(validateExpression('predicate', 'record.name == 5', schema).warnings).toHaveLength(0);
215+
expect(validateExpression('predicate', 'record.name != 5', schema).warnings).toHaveLength(0);
216+
});
217+
218+
it('does NOT warn on string concatenation or a null-guard', () => {
219+
expect(validateExpression('value', 'record.name + record.title', schema).warnings).toHaveLength(0);
220+
expect(validateExpression('predicate', 'record.amount != null && record.amount > 0', schema).warnings).toHaveLength(0);
221+
});
222+
223+
it('does not run without field types, or in a flattened (flow) scope', () => {
224+
// No fieldTypes → nothing to check.
225+
expect(validateExpression('value', 'record.name * 2', { objectName: 'crm_opportunity', fields: schema.fields, scope: 'record' }).warnings).toHaveLength(0);
226+
// Flattened flow conditions reference fields bare and carry flow variables;
227+
// the typed check is intentionally record-scope only.
228+
expect(validateExpression('predicate', 'name * 2', { ...schema, scope: 'flattened' }).warnings).toHaveLength(0);
229+
});
230+
});
231+
156232
describe('introspection', () => {
157233
it('reports the dialect + scope for a field role', () => {
158234
expect(expectedDialect('predicate')).toBe('cel');

packages/formula/src/validate.ts

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* This validator detects that specific mistake and returns the exact fix.
1818
*/
1919

20-
import { celEngine, firstUndeclaredReference, inferCelType } from './cel-engine';
20+
import { celEngine, firstUndeclaredReference, firstTypeMismatch, inferCelType, type FieldCelType } from './cel-engine';
2121
import { templateEngine } from './template-engine';
2222

2323
export type FieldRole = 'predicate' | 'value' | 'template';
@@ -36,6 +36,15 @@ export interface ExprSchemaHint {
3636
objectName?: string;
3737
/** Known top-level field names, so `record.<field>` can be checked. */
3838
fields?: readonly string[];
39+
/**
40+
* #1928 tier 4 — field name → spec field type (`'text'`, `'currency'`,
41+
* `'boolean'`, `'date'`, …). Enables the advisory type-soundness check: a
42+
* text or boolean field used with an arithmetic/ordering operator against a
43+
* number faults at runtime and the expression silently evaluates to `null`,
44+
* so it is surfaced as a NON-blocking warning. Absent ⇒ the check is skipped.
45+
* Only consulted for `scope: 'record'` sites (where refs are `record.<field>`).
46+
*/
47+
fieldTypes?: Readonly<Record<string, string>>;
3948
/**
4049
* Evaluation scope of the authoring site — determines whether a bare top-level
4150
* identifier is legal (#1928):
@@ -82,6 +91,32 @@ export interface ExprValidationResult {
8291
warnings: ExprValidationError[];
8392
}
8493

94+
/**
95+
* #1928 tier 4 — spec field type → the CEL type it is declared as for the
96+
* type-soundness check. ONLY genuinely-scalar, non-numeric-intent types are
97+
* pinned to a concrete type (`string` / `bool`); every other type — numbers,
98+
* dates, selects (option values may be numeric codes), lookups, media, JSON —
99+
* maps to `dyn` so it can never fault (the runtime rescues all of those). Any
100+
* field type absent from this map is treated as `dyn`. Keeping the map narrow
101+
* is the source of the check's near-zero false-positive rate.
102+
*/
103+
const SPEC_TYPE_TO_CEL: Readonly<Record<string, FieldCelType>> = {
104+
// Free text — arithmetic / ordering against a number is (almost) always a bug.
105+
text: 'string', textarea: 'string', email: 'string', url: 'string',
106+
phone: 'string', markdown: 'string', html: 'string', richtext: 'string',
107+
// Booleans — arithmetic / ordering against a number ALWAYS faults at runtime.
108+
boolean: 'bool', toggle: 'bool',
109+
};
110+
111+
/** Map an object's field-type hints onto the CEL types the soundness check uses. */
112+
function toCelFieldTypes(fieldTypes: Readonly<Record<string, string>>): Record<string, FieldCelType> {
113+
const out: Record<string, FieldCelType> = {};
114+
for (const [name, specType] of Object.entries(fieldTypes)) {
115+
out[name] = SPEC_TYPE_TO_CEL[specType] ?? 'dyn';
116+
}
117+
return out;
118+
}
119+
85120
/** A bare `{x}` that is NOT part of a `{{x}}` mustache hole. */
86121
const SINGLE_BRACE_RE = /(?:^|[^{])\{\s*([A-Za-z_$][\w.$]*)\s*\}(?!\})/;
87122
/** `record.<field>` / `previous.<field>` head references for field-existence. */
@@ -260,6 +295,28 @@ export function validateExpression(
260295
`\`record\` namespace, not at top level, so \`${bare}\` resolves to nothing and the ` +
261296
`expression silently evaluates to null. Write \`record.${bare}\`.`,
262297
});
298+
} else if (schema.fieldTypes) {
299+
// #1928 tier 4 — with per-field types in hand, flag a text/boolean field
300+
// used with an arithmetic/ordering operator against a number: it faults
301+
// the runtime overload and the expression silently evaluates to null.
302+
// Advisory (never blocks the build): the runtime CAN succeed if a text
303+
// value happens to be numeric, so this is a warning, not an error. Only
304+
// runs when there is no bare-ref error (the typed check needs the
305+
// canonical `record.<field>` form).
306+
const mismatch = firstTypeMismatch(source, toCelFieldTypes(schema.fieldTypes));
307+
if (mismatch) {
308+
const held = mismatch.celType === 'bool' ? 'a boolean' : 'text';
309+
const subject = mismatch.field
310+
? `\`record.${mismatch.field}\` holds ${held}`
311+
: `${held === 'a boolean' ? 'a boolean' : 'a text'} field`;
312+
warnings.push({
313+
source,
314+
message:
315+
`type mismatch \`${mismatch.operands}\` — ${subject} but is used with \`${mismatch.operator}\` ` +
316+
`against a number. This faults at runtime, so the expression silently evaluates to null ` +
317+
`(unless the value happens to be numeric). Use a number field, or drop the arithmetic/comparison.`,
318+
});
319+
}
263320
}
264321
} else if (schema?.fields && schema.fields.length > 0) {
265322
// Flattened flow/automation condition: the record's fields ARE bound at

packages/lint/src/validate-expressions.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,57 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
234234
});
235235
});
236236

237+
// #1928 tier 4 — a text/boolean field used with an arithmetic/ordering
238+
// operator against a number is a silent-null bug; the lint surfaces it as a
239+
// non-blocking warning, threading each object's field types into the checker.
240+
describe('type-soundness warnings (#1928 tier 4)', () => {
241+
it('warns on a formula that does arithmetic on a text field', () => {
242+
const issues = validateStackExpressions({
243+
objects: [{
244+
name: 'crm_lead',
245+
fields: {
246+
company: { type: 'text' },
247+
score: { type: 'formula', formula: 'record.company * 2' },
248+
},
249+
}],
250+
});
251+
const w = issues.filter(i => i.severity === 'warning');
252+
expect(w).toHaveLength(1);
253+
expect(w[0].where).toMatch(/formula/);
254+
expect(w[0].message).toMatch(/type mismatch/i);
255+
expect(w[0].message).toMatch(/record\.company/);
256+
});
257+
258+
it('does not flag number arithmetic or date comparison (runtime-sound)', () => {
259+
const issues = validateStackExpressions({
260+
objects: [{
261+
name: 'crm_opportunity',
262+
fields: {
263+
amount: { type: 'currency' },
264+
probability: { type: 'percent' },
265+
close_date: { type: 'date' },
266+
expected: { type: 'formula', formula: 'record.amount * record.probability / 100' },
267+
},
268+
validations: [{ name: 'future', expression: 'record.close_date >= today()' }],
269+
}],
270+
});
271+
expect(issues).toHaveLength(0);
272+
});
273+
274+
it('warns on a validation predicate ordering a text field against a number', () => {
275+
const issues = validateStackExpressions({
276+
objects: [{
277+
name: 'crm_lead',
278+
fields: { title: { type: 'text' } },
279+
validations: [{ name: 'r', expression: 'record.title > 5' }],
280+
}],
281+
});
282+
const w = issues.filter(i => i.severity === 'warning');
283+
expect(w).toHaveLength(1);
284+
expect(w[0].message).toMatch(/record\.title/);
285+
});
286+
});
287+
237288
describe('action visible/disabled predicates (record-scoped) — #2183 class', () => {
238289
it('flags a bare-field `visible` on a stack action (the trap that hid Mark Done)', () => {
239290
const issues = validateStackExpressions({

0 commit comments

Comments
 (0)