From 5143d9d18baf50fc81bdbc48408b69be6e1346df Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 06:27:30 +0000 Subject: [PATCH 1/2] feat(formula,lint): advisory type-soundness warnings for expressions (#1928 tier 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01Hnji7EEYR2mGt6pY53a8Bm --- .changeset/tier4-type-soundness-warnings.md | 30 +++++ packages/formula/src/cel-engine.ts | 119 ++++++++++++++++++ packages/formula/src/validate.test.ts | 76 +++++++++++ packages/formula/src/validate.ts | 59 ++++++++- .../lint/src/validate-expressions.test.ts | 51 ++++++++ packages/lint/src/validate-expressions.ts | 37 +++++- 6 files changed, 369 insertions(+), 3 deletions(-) create mode 100644 .changeset/tier4-type-soundness-warnings.md diff --git a/.changeset/tier4-type-soundness-warnings.md b/.changeset/tier4-type-soundness-warnings.md new file mode 100644 index 0000000000..e5923cffef --- /dev/null +++ b/.changeset/tier4-type-soundness-warnings.md @@ -0,0 +1,30 @@ +--- +"@objectstack/formula": minor +"@objectstack/lint": minor +--- + +feat(formula,lint): advisory type-soundness warnings for formula/predicate expressions (#1928 tier 4) + +Closes the last open guardrail from #1928. 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** with the offending field and a corrective message. + +Honours the ADR-0032 design law — the checker only flags what the runtime +would also fail: + +- Number / currency / percent / date / datetime fields are declared `dyn`, so + the cases the runtime rescues never warn — `record.amount / 100` (the #1930 + `registerOperator` fix), `record.due == today()` and numeric-string / ISO-date + values (the string-hydration retry), and numeric-coded `select` option values. +- Equality (`==` / `!=`) is excluded: a heterogeneous equality is runtime-safe + (evaluates to `false`), never a fault. + +New `firstTypeMismatch` export in `@objectstack/formula` (and an optional +`fieldTypes` hint on `validateExpression`); `@objectstack/lint`'s +`validateStackExpressions` threads each object's field types into every +record-scoped site (formula fields, validation rules, action / hook / sharing +predicates). Warnings are advisory in `objectstack build` / `validate` +(fatal only under `--strict`), matching the tier-3 channel. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 27e8653c38..10d0503e62 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -167,6 +167,125 @@ export function detectBareReference(source: string): string | null { return firstUndeclaredReference(source); } +/** + * 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}. + */ +export type FieldCelType = 'string' | 'bool' | 'dyn'; + +/** + * A `no such overload` fault for an ARITHMETIC (`+ - * / %`) or ORDERING + * (`< > <= >=`) operator, with the two operand types captured. Equality + * (`==` / `!=`) is intentionally excluded: cel-js's checker faults on a + * heterogeneous equality (`string == int`) but the runtime evaluates it + * cleanly to `false` — so a fault there is NOT a runtime failure and must not + * warn. Linear (no nested quantifiers) — no ReDoS. Operand types are `[\w.]+` + * (e.g. `string`, `int`, `google.protobuf.Timestamp`); the operator token is + * punctuation, so the two never overlap. + */ +const UNSOUND_OVERLOAD_RE = /no such overload:\s*([\w.]+)\s*(<=|>=|<|>|\+|-|\*|\/|%)\s*([\w.]+)/; + +/** + * A `record`-typed environment where each field carries a concrete CEL type + * (`string`/`bool`) or `dyn`. Member access (`record.`) then resolves to + * the field's type, so cel-js's checker faults an arithmetic/ordering operator + * applied across incompatible types. Built per call — cheap, and only used at + * build time. + */ +function buildTypedRecordEnv(fieldCelTypes: Readonly>): Environment { + const env = new Environment({ + unlistedVariablesAreDyn: false, + enableOptionalTypes: true, + limits: DEFAULT_LIMITS, + }); + registerStdLib(env, () => new Date(0)); + const fields: Record = {}; + for (const [name, t] of Object.entries(fieldCelTypes)) fields[name] = 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. + for (const root of ['record', 'previous', 'input']) { + try { env.registerVariable(root, 'OsRecordScope'); } catch { /* duplicate — ignore */ } + } + for (const root of SCOPE_ROOTS) { + try { env.registerVariable(root, 'map'); } catch { /* already typed above / duplicate — ignore */ } + } + return env; +} + +/** + * The first `record.` (or `previous.`/`input.`) reference in `source` + * whose declared CEL type matches `celType` — best-effort attribution of an + * overload fault to the offending field. Returns `null` if none is found. + */ +function offendingField( + source: string, + fieldCelTypes: Readonly>, + celType: FieldCelType, +): string | null { + for (const [name, t] of Object.entries(fieldCelTypes)) { + if (t !== celType) continue; + // Word-bounded so `amount` does not match `amount_total`. + if (new RegExp(`(?:record|previous|input)\\.${name}(?![\\w$])`).test(source)) return name; + } + return null; +} + +/** + * Tier-4 type-soundness (#1928): detect a `record`-scoped expression that + * type-checks structurally but faults a runtime operator overload because a + * text (`string`) or boolean (`bool`) field is used with an arithmetic or + * ordering operator against a number. Such an expression evaluates to `null` + * at runtime (unless the text value happens to be numeric), so it is surfaced + * 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. + * - Equality (`==` / `!=`) is excluded ({@link UNSOUND_OVERLOAD_RE}): a + * heterogeneous equality is runtime-safe. + * + * Returns the operand types, the faulting operator, the concrete operand CEL + * type, and (best-effort) the offending field — or `null` when type-sound. + */ +export function firstTypeMismatch( + source: string, + fieldCelTypes: Readonly>, +): { operator: string; operands: string; celType: FieldCelType; field: string | null } | 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; + try { + const env = buildTypedRecordEnv(fieldCelTypes); + const result = env.parse(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 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, + field: offendingField(source, fieldCelTypes, celType), + }; + } catch { + // A parse/other fault is the syntax checker's job (celEngine.compile); this + // helper only reports a clean type-soundness verdict. + return null; + } +} + /** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */ function coerce(value: unknown): unknown { if (typeof value === 'bigint') { diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index 951332f98b..b07a9ea5e2 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -153,6 +153,82 @@ describe('validateExpression (ADR-0032)', () => { }); }); + // #1928 tier 4 — a text/boolean field used with an arithmetic or ordering + // operator against a number faults at runtime (silent null). With per-field + // types the validator surfaces this as a NON-blocking warning, and — the + // design law — never flags a case the runtime tolerates (number/date fields, + // equality, string concat, null-guards). + describe('type-soundness warnings (#1928 tier 4)', () => { + const schema = { + objectName: 'crm_opportunity', + fields: ['name', 'amount', 'is_active', 'due', 'priority', 'title'] as const, + fieldTypes: { + name: 'text', title: 'textarea', amount: 'currency', + is_active: 'boolean', due: 'date', priority: 'select', + }, + scope: 'record', + } as const; + + it('warns (does not error) on a text field used in arithmetic against a number', () => { + const r = validateExpression('value', 'record.name * 2', schema); + expect(r.ok).toBe(true); + expect(r.errors).toHaveLength(0); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/type mismatch/i); + expect(r.warnings[0].message).toMatch(/record\.name/); + expect(r.warnings[0].message).toMatch(/evaluates to null/); + }); + + it('warns on a text field ordered against a number', () => { + const r = validateExpression('predicate', 'record.title >= 5', schema); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/record\.title/); + }); + + it('warns on a boolean field used in arithmetic (always faults at runtime)', () => { + const r = validateExpression('value', 'record.is_active + 1', schema); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/boolean/i); + expect(r.warnings[0].message).toMatch(/record\.is_active/); + }); + + it('does NOT warn on number/currency arithmetic with an int literal (#1930 runtime fix)', () => { + // currency → dyn, so `amount / 100`, `amount * 2 - 50` never fault. + expect(validateExpression('value', 'record.amount / 100', schema).warnings).toHaveLength(0); + expect(validateExpression('value', 'record.amount * 2 - 50', schema).warnings).toHaveLength(0); + }); + + it('does NOT warn on a date field compared to today()/daysFromNow()', () => { + expect(validateExpression('predicate', 'record.due <= daysFromNow(30)', schema).warnings).toHaveLength(0); + expect(validateExpression('predicate', 'record.due == today()', schema).warnings).toHaveLength(0); + }); + + it('does NOT warn on a select field ordered against a number (option values may be numeric codes)', () => { + // select → dyn, so `priority >= 3` (a numeric-coded picklist) is not flagged. + expect(validateExpression('predicate', 'record.priority >= 3', schema).warnings).toHaveLength(0); + }); + + it('does NOT warn on heterogeneous equality (runtime-safe, returns false)', () => { + expect(validateExpression('predicate', 'record.name == 5', schema).warnings).toHaveLength(0); + expect(validateExpression('predicate', 'record.name != 5', schema).warnings).toHaveLength(0); + }); + + it('does NOT warn on string concatenation or a null-guard', () => { + expect(validateExpression('value', 'record.name + record.title', schema).warnings).toHaveLength(0); + expect(validateExpression('predicate', 'record.amount != null && record.amount > 0', schema).warnings).toHaveLength(0); + }); + + it('does not run without field types, or in a flattened (flow) scope', () => { + // No fieldTypes → nothing to check. + expect(validateExpression('value', 'record.name * 2', { objectName: 'crm_opportunity', fields: schema.fields, scope: 'record' }).warnings).toHaveLength(0); + // Flattened flow conditions reference fields bare and carry flow variables; + // the typed check is intentionally record-scope only. + expect(validateExpression('predicate', 'name * 2', { ...schema, scope: 'flattened' }).warnings).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 bd3fa6157a..9b11fcd601 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -17,7 +17,7 @@ * This validator detects that specific mistake and returns the exact fix. */ -import { celEngine, firstUndeclaredReference, inferCelType } from './cel-engine'; +import { celEngine, firstUndeclaredReference, firstTypeMismatch, inferCelType, type FieldCelType } from './cel-engine'; import { templateEngine } from './template-engine'; export type FieldRole = 'predicate' | 'value' | 'template'; @@ -36,6 +36,15 @@ export interface ExprSchemaHint { objectName?: string; /** Known top-level field names, so `record.` can be checked. */ fields?: readonly string[]; + /** + * #1928 tier 4 — field name → spec field type (`'text'`, `'currency'`, + * `'boolean'`, `'date'`, …). Enables the advisory type-soundness check: a + * text or boolean field used with an arithmetic/ordering operator against a + * number faults at runtime and the expression silently evaluates to `null`, + * so it is surfaced as a NON-blocking warning. Absent ⇒ the check is skipped. + * Only consulted for `scope: 'record'` sites (where refs are `record.`). + */ + fieldTypes?: Readonly>; /** * Evaluation scope of the authoring site — determines whether a bare top-level * identifier is legal (#1928): @@ -82,6 +91,32 @@ export interface ExprValidationResult { warnings: ExprValidationError[]; } +/** + * #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. + */ +const SPEC_TYPE_TO_CEL: Readonly> = { + // Free text — arithmetic / ordering against a number is (almost) always a bug. + text: 'string', textarea: 'string', email: 'string', url: 'string', + phone: 'string', markdown: 'string', html: 'string', richtext: 'string', + // Booleans — arithmetic / ordering against a number ALWAYS faults at runtime. + boolean: 'bool', toggle: 'bool', +}; + +/** Map an object's field-type hints onto the CEL types the soundness check uses. */ +function toCelFieldTypes(fieldTypes: Readonly>): Record { + const out: Record = {}; + for (const [name, specType] of Object.entries(fieldTypes)) { + out[name] = SPEC_TYPE_TO_CEL[specType] ?? 'dyn'; + } + return out; +} + /** A bare `{x}` that is NOT part of a `{{x}}` mustache hole. */ const SINGLE_BRACE_RE = /(?:^|[^{])\{\s*([A-Za-z_$][\w.$]*)\s*\}(?!\})/; /** `record.` / `previous.` head references for field-existence. */ @@ -260,6 +295,28 @@ export function validateExpression( `\`record\` namespace, not at top level, so \`${bare}\` resolves to nothing and the ` + `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 mismatch = firstTypeMismatch(source, toCelFieldTypes(schema.fieldTypes)); + if (mismatch) { + const held = mismatch.celType === 'bool' ? 'a boolean' : 'text'; + const subject = mismatch.field + ? `\`record.${mismatch.field}\` holds ${held}` + : `${held === 'a boolean' ? 'a boolean' : 'a text'} field`; + warnings.push({ + 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.`, + }); + } } } else if (schema?.fields && schema.fields.length > 0) { // Flattened flow/automation condition: the record's fields ARE bound at diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index ba07f48ec3..6fb1f19e71 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -234,6 +234,57 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { }); }); + // #1928 tier 4 — a text/boolean field used with an arithmetic/ordering + // operator against a number is a silent-null bug; the lint surfaces it as a + // non-blocking warning, threading each object's field types into the checker. + describe('type-soundness warnings (#1928 tier 4)', () => { + it('warns on a formula that does arithmetic on a text field', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_lead', + fields: { + company: { type: 'text' }, + score: { type: 'formula', formula: 'record.company * 2' }, + }, + }], + }); + const w = issues.filter(i => i.severity === 'warning'); + expect(w).toHaveLength(1); + expect(w[0].where).toMatch(/formula/); + expect(w[0].message).toMatch(/type mismatch/i); + expect(w[0].message).toMatch(/record\.company/); + }); + + it('does not flag number arithmetic or date comparison (runtime-sound)', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_opportunity', + fields: { + amount: { type: 'currency' }, + probability: { type: 'percent' }, + close_date: { type: 'date' }, + expected: { type: 'formula', formula: 'record.amount * record.probability / 100' }, + }, + validations: [{ name: 'future', expression: 'record.close_date >= today()' }], + }], + }); + expect(issues).toHaveLength(0); + }); + + it('warns on a validation predicate ordering a text field against a number', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_lead', + fields: { title: { type: 'text' } }, + validations: [{ name: 'r', expression: 'record.title > 5' }], + }], + }); + const w = issues.filter(i => i.severity === 'warning'); + expect(w).toHaveLength(1); + expect(w[0].message).toMatch(/record\.title/); + }); + }); + describe('action visible/disabled predicates (record-scoped) — #2183 class', () => { it('flags a bare-field `visible` on a stack action (the trap that hid Mark Done)', () => { const issues = validateStackExpressions({ diff --git a/packages/lint/src/validate-expressions.ts b/packages/lint/src/validate-expressions.ts index 8a0b7a1f46..3611c6dfdf 100644 --- a/packages/lint/src/validate-expressions.ts +++ b/packages/lint/src/validate-expressions.ts @@ -55,6 +55,35 @@ function buildFieldIndex(objects: AnyRec[]): Map { return idx; } +/** + * object name → (field name → field type), for the #1928 tier-4 type-soundness + * check. Handles both `fields` shapes (array of `{name, type}` and name-keyed + * map). Fields with a non-string `type` are simply omitted (treated as `dyn`). + */ +function buildFieldTypeIndex(objects: AnyRec[]): Map> { + const idx = new Map>(); + for (const obj of objects) { + const name = typeof obj.name === 'string' ? obj.name : undefined; + if (!name) continue; + const fields = obj.fields; + const types: Record = {}; + if (Array.isArray(fields)) { + for (const f of fields as AnyRec[]) { + const fn = (f as AnyRec)?.name; + const ft = (f as AnyRec)?.type; + if (typeof fn === 'string' && typeof ft === 'string') types[fn] = ft; + } + } else if (fields && typeof fields === 'object') { + for (const [fn, def] of Object.entries(fields as AnyRec)) { + const ft = (def as AnyRec)?.type; + if (typeof ft === 'string') types[fn] = ft; + } + } + idx.set(name, types); + } + return idx; +} + /** * Validate every predicate in the stack. Returns the list of issues (empty = * clean). Caller decides how to surface / whether to fail the build. @@ -63,6 +92,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const issues: ExprIssue[] = []; const objects = asArray(stack.objects); const fieldIndex = buildFieldIndex(objects); + const fieldTypeIndex = buildFieldTypeIndex(objects); const check = ( where: string, @@ -72,8 +102,11 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { ): void => { if (raw == null) return; const fields = objectName ? fieldIndex.get(objectName) : undefined; + // Field types feed the #1928 tier-4 soundness warning; only consulted for + // `record`-scoped sites, so it is harmless to pass for flattened ones too. + const fieldTypes = objectName ? fieldTypeIndex.get(objectName) : undefined; const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, - objectName ? { objectName, fields, scope } : { scope }); + objectName ? { objectName, fields, fieldTypes, scope } : { scope }); for (const e of res.errors) issues.push({ where, message: e.message, source: e.source, severity: 'error' }); for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' }); }; @@ -193,7 +226,7 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { // formulas are `value` role (any return type), still CEL. They are // `record`-scoped — `record.`, never bare — so flag bare refs (#1928). const res = validateExpression('value', f.formula as string | { dialect?: string; source?: string }, - objectName ? { objectName, fields: fieldIndex.get(objectName), scope: 'record' } : { scope: 'record' }); + objectName ? { objectName, fields: fieldIndex.get(objectName), fieldTypes: fieldTypeIndex.get(objectName), scope: 'record' } : { scope: 'record' }); const fieldWhere = `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`; for (const e of res.errors) issues.push({ where: fieldWhere, message: e.message, source: e.source, severity: 'error' }); for (const w of res.warnings) issues.push({ where: fieldWhere, message: w.message, source: w.source, severity: 'warning' }); From 826edb12c6db02d9aad84fcff6744aabbd867b81 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 06:44:50 +0000 Subject: [PATCH 2/2] feat(formula,lint): extend tier-4 type-soundness to flattened flow conditions (#1928) `firstTypeMismatch` gains a `scope` param: in addition to record-scoped sites (`record.`), it now covers bare-field flow/automation conditions (`status - 1`, `is_active + 1`). The flattened env binds each field as a top-level typed variable with `unlistedVariablesAreDyn: true`, so flow variables stay `dyn` and are never flagged; equality stays excluded. `validateExpression` runs the check in the flattened branch too, so the lint's flow-condition validation surfaces the same advisory warning. Message uses the bare `field` form in flattened scope, `record.field` in record scope. Tests: formula 219 (+4 flattened cases), lint 232 (+2 flow-condition cases). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Hnji7EEYR2mGt6pY53a8Bm --- .changeset/tier4-type-soundness-warnings.md | 18 +++-- packages/formula/src/cel-engine.ts | 65 +++++++++++++++---- packages/formula/src/validate.test.ts | 45 +++++++++++-- packages/formula/src/validate.ts | 51 +++++++++++---- .../lint/src/validate-expressions.test.ts | 27 ++++++++ 5 files changed, 169 insertions(+), 37 deletions(-) diff --git a/.changeset/tier4-type-soundness-warnings.md b/.changeset/tier4-type-soundness-warnings.md index e5923cffef..628827e21a 100644 --- a/.changeset/tier4-type-soundness-warnings.md +++ b/.changeset/tier4-type-soundness-warnings.md @@ -22,9 +22,15 @@ would also fail: - Equality (`==` / `!=`) is excluded: a heterogeneous equality is runtime-safe (evaluates to `false`), never a fault. -New `firstTypeMismatch` export in `@objectstack/formula` (and an optional -`fieldTypes` hint on `validateExpression`); `@objectstack/lint`'s -`validateStackExpressions` threads each object's field types into every -record-scoped site (formula fields, validation rules, action / hook / sharing -predicates). Warnings are advisory in `objectstack build` / `validate` -(fatal only under `--strict`), matching the tier-3 channel. +New `firstTypeMismatch(source, fieldCelTypes, scope)` export in +`@objectstack/formula` (and an optional `fieldTypes` hint on +`validateExpression`); `@objectstack/lint`'s `validateStackExpressions` threads +each object's field types into every checked site: + +- **record-scoped** sites (`record.`) — formula fields, validation rules, + action / hook / sharing predicates; +- **flattened** flow / automation conditions (bare `field`) — where flow + variables stay `dyn` and are never flagged, and equality stays runtime-safe. + +Warnings are advisory in `objectstack build` / `validate` (fatal only under +`--strict`), matching the tier-3 channel. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 10d0503e62..8e12e7079f 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -188,13 +188,40 @@ export type FieldCelType = 'string' | 'bool' | 'dyn'; const UNSOUND_OVERLOAD_RE = /no such overload:\s*([\w.]+)\s*(<=|>=|<|>|\+|-|\*|\/|%)\s*([\w.]+)/; /** - * A `record`-typed environment where each field carries a concrete CEL type - * (`string`/`bool`) or `dyn`. Member access (`record.`) then resolves to - * the field's type, so cel-js's checker faults an arithmetic/ordering operator - * applied across incompatible types. Built per call — cheap, and only used at - * build time. + * A typed environment for the soundness check. Each field carries a concrete + * CEL type (`string`/`bool`) or `dyn`, so cel-js's checker faults an + * arithmetic/ordering operator applied across incompatible types. The `scope` + * mirrors how the authoring site binds fields: + * - `'record'` → `record.` member access, via a typed struct on the + * `record`/`previous`/`input` namespaces (formula fields, + * validations, action/hook/sharing predicates). + * - `'flattened'` → bare `` top-level variables (flow / automation + * conditions). Unlisted identifiers stay `dyn` + * (`unlistedVariablesAreDyn: true`) so a flow variable never + * faults — only a typed field misused does. + * Built per call — cheap, and only used at build time. */ -function buildTypedRecordEnv(fieldCelTypes: Readonly>): Environment { +function buildTypedEnv( + fieldCelTypes: Readonly>, + scope: 'record' | 'flattened', +): Environment { + if (scope === 'flattened') { + const env = new Environment({ + unlistedVariablesAreDyn: true, + enableOptionalTypes: true, + limits: DEFAULT_LIMITS, + }); + registerStdLib(env, () => new Date(0)); + 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 */ } + } + return env; + } const env = new Environment({ unlistedVariablesAreDyn: false, enableOptionalTypes: true, @@ -216,19 +243,26 @@ function buildTypedRecordEnv(fieldCelTypes: Readonly` (or `previous.`/`input.`) reference in `source` - * whose declared CEL type matches `celType` — best-effort attribution of an - * overload fault to the offending field. Returns `null` if none is found. + * The first field reference in `source` whose declared CEL type matches + * `celType` — best-effort attribution of an overload fault to the offending + * field. In `'record'` scope it looks for `record.` (or `previous.`/ + * `input.`); in `'flattened'` scope for a bare `` not preceded by a dot. + * Returns `null` if none is found. */ function offendingField( source: string, fieldCelTypes: Readonly>, celType: FieldCelType, + scope: 'record' | 'flattened', ): string | null { for (const [name, t] of Object.entries(fieldCelTypes)) { if (t !== celType) continue; - // Word-bounded so `amount` does not match `amount_total`. - if (new RegExp(`(?:record|previous|input)\\.${name}(?![\\w$])`).test(source)) return name; + // Word-bounded so `amount` does not match `amount_total`; in flattened + // scope the leading lookbehind excludes a member ref like `previous.amount`. + const re = scope === 'flattened' + ? new RegExp(`(?` sites; `'flattened'` for bare-field flow/automation + * conditions. */ export function firstTypeMismatch( source: string, fieldCelTypes: Readonly>, + scope: 'record' | 'flattened' = 'record', ): { operator: string; operands: string; celType: FieldCelType; field: string | null } | 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; try { - const env = buildTypedRecordEnv(fieldCelTypes); + const env = buildTypedEnv(fieldCelTypes, scope); const result = env.parse(source).check?.() as | { valid?: boolean; error?: { message?: string } } | undefined; @@ -277,7 +316,7 @@ export function firstTypeMismatch( operator, operands: `${m[1]} ${operator} ${m[3]}`, celType, - field: offendingField(source, fieldCelTypes, celType), + field: offendingField(source, fieldCelTypes, celType, scope), }; } catch { // A parse/other fault is the syntax checker's job (celEngine.compile); this diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index b07a9ea5e2..56242441d8 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -220,12 +220,49 @@ describe('validateExpression (ADR-0032)', () => { expect(validateExpression('predicate', 'record.amount != null && record.amount > 0', schema).warnings).toHaveLength(0); }); - it('does not run without field types, or in a flattened (flow) scope', () => { + it('does not run without field types', () => { // No fieldTypes → nothing to check. expect(validateExpression('value', 'record.name * 2', { objectName: 'crm_opportunity', fields: schema.fields, scope: 'record' }).warnings).toHaveLength(0); - // Flattened flow conditions reference fields bare and carry flow variables; - // the typed check is intentionally record-scope only. - expect(validateExpression('predicate', 'name * 2', { ...schema, scope: 'flattened' }).warnings).toHaveLength(0); + }); + }); + + // #1928 tier 4 (flattened) — the same soundness check for bare-field flow / + // automation conditions. Fields are bound bare (`status - 1`); flow variables + // stay `dyn` and are never flagged. + describe('type-soundness warnings — flattened flow conditions (#1928 tier 4)', () => { + const schema = { + objectName: 'crm_opportunity', + fields: ['stage', 'amount', 'is_active', 'title'] as const, + fieldTypes: { stage: 'select', amount: 'currency', is_active: 'boolean', title: 'text' }, + scope: 'flattened', + } as const; + + it('warns on a bare text field used in arithmetic against a number', () => { + const r = validateExpression('predicate', 'title - 1 > 0', schema); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/type mismatch/i); + // Bare form — not `record.title`. + expect(r.warnings[0].message).toMatch(/`title`/); + expect(r.warnings[0].message).not.toMatch(/record\.title/); + }); + + it('warns on a bare boolean field used in arithmetic', () => { + const r = validateExpression('predicate', 'is_active + 1 > 0', schema); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/boolean/i); + }); + + it('does NOT flag a flow variable (unlisted → dyn) or number/date fields', () => { + // `expiring_count` is not a schema field → dyn → no fault. + expect(validateExpression('predicate', 'expiring_count * 2 > 10', schema).warnings).toHaveLength(0); + expect(validateExpression('predicate', 'amount / 100 > 5', schema).warnings).toHaveLength(0); + }); + + it('does NOT flag a correct bare condition, equality, or a select comparison', () => { + expect(validateExpression('predicate', 'stage == "closed_won" && amount > 1000', schema).warnings).toHaveLength(0); + expect(validateExpression('predicate', 'title == "VIP"', schema).warnings).toHaveLength(0); }); }); diff --git a/packages/formula/src/validate.ts b/packages/formula/src/validate.ts index 9b11fcd601..868f634d57 100644 --- a/packages/formula/src/validate.ts +++ b/packages/formula/src/validate.ts @@ -117,6 +117,33 @@ function toCelFieldTypes(fieldTypes: Readonly>): Record` vs bare + * field binding, and shapes the referenced form in the message. + */ +function typeSoundnessWarning( + source: string, + fieldTypes: Readonly>, + scope: 'record' | 'flattened', +): ExprValidationError | 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; + 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.`, + }; +} + /** A bare `{x}` that is NOT part of a `{{x}}` mustache hole. */ const SINGLE_BRACE_RE = /(?:^|[^{])\{\s*([A-Za-z_$][\w.$]*)\s*\}(?!\})/; /** `record.` / `previous.` head references for field-existence. */ @@ -303,20 +330,8 @@ export function validateExpression( // 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 mismatch = firstTypeMismatch(source, toCelFieldTypes(schema.fieldTypes)); - if (mismatch) { - const held = mismatch.celType === 'bool' ? 'a boolean' : 'text'; - const subject = mismatch.field - ? `\`record.${mismatch.field}\` holds ${held}` - : `${held === 'a boolean' ? 'a boolean' : 'a text'} field`; - warnings.push({ - 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.`, - }); - } + const w = typeSoundnessWarning(source, schema.fieldTypes, 'record'); + if (w) warnings.push(w); } } else if (schema?.fields && schema.fields.length > 0) { // Flattened flow/automation condition: the record's fields ARE bound at @@ -337,6 +352,14 @@ 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. + if (schema.fieldTypes) { + const w = typeSoundnessWarning(source, schema.fieldTypes, 'flattened'); + if (w) warnings.push(w); + } } } return { ok: errors.length === 0, errors, warnings }; diff --git a/packages/lint/src/validate-expressions.test.ts b/packages/lint/src/validate-expressions.test.ts index 6fb1f19e71..02cd256183 100644 --- a/packages/lint/src/validate-expressions.test.ts +++ b/packages/lint/src/validate-expressions.test.ts @@ -283,6 +283,33 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(w).toHaveLength(1); expect(w[0].message).toMatch(/record\.title/); }); + + it('warns on a flattened flow condition doing arithmetic on a bare text field', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { title: { type: 'text' }, amount: { type: 'currency' } } }], + flows: [{ + name: 'opp_flow', + nodes: [{ id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'title * 2 > 10' } }], + edges: [], + }], + }); + const w = issues.filter(i => i.severity === 'warning'); + expect(w).toHaveLength(1); + expect(w[0].message).toMatch(/type mismatch/i); + expect(w[0].message).toMatch(/`title`/); + }); + + it('does not flag a numeric flow condition or a flow variable', () => { + const issues = validateStackExpressions({ + objects: [{ name: 'crm_opportunity', fields: { amount: { type: 'currency' } } }], + flows: [{ + name: 'opp_flow', + nodes: [{ id: 'start', type: 'start', config: { objectName: 'crm_opportunity' } }], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'amount / 100 > 5 && expiring_count * 2 > 3' }], + }], + }); + expect(issues).toHaveLength(0); + }); }); describe('action visible/disabled predicates (record-scoped) — #2183 class', () => {