diff --git a/.changeset/validate-formula-bare-refs.md b/.changeset/validate-formula-bare-refs.md new file mode 100644 index 0000000000..e121f9abb9 --- /dev/null +++ b/.changeset/validate-formula-bare-refs.md @@ -0,0 +1,21 @@ +--- +"@objectstack/formula": patch +"@objectstack/cli": patch +--- + +feat(validate): flag bare field references in record-scoped CEL sites at build time + +A `Field.formula` and an object validation predicate evaluate against the +`record` namespace only — there is no field flattening — so a bare top-level +identifier (`amount`, `status`) resolves to nothing and the expression silently +evaluates to `null` / never fires. This is the silent-at-runtime class behind +the broken example-crm formulas (#1927) and is exactly what AI authors get wrong. + +`validateExpression` now takes an evaluation `scope` and, for `scope: 'record'`, +reports a bare reference with the corrective form (`write record.`). The +check is schema-free and acts only on cel-js's `Unknown variable` fault, so it +cannot false-positive on arithmetic/comparison/null-guard type overloads. Flow +and automation conditions keep the default `scope: 'flattened'` — the record's +fields ARE spread to top-level there (alongside flow variables), so bare refs +are correct and are NOT flagged. `objectstack compile` wires `record` scope for +field formulas and validation predicates; flow conditions stay flattened. diff --git a/examples/app-crm/src/objects/lead.object.ts b/examples/app-crm/src/objects/lead.object.ts index 70792e7f31..bf69288e19 100644 --- a/examples/app-crm/src/objects/lead.object.ts +++ b/examples/app-crm/src/objects/lead.object.ts @@ -104,7 +104,7 @@ export const Lead = ObjectSchema.create({ name: 'lead_score_range', label: 'Lead Score 0–100', description: 'Lead score must be between 0 and 100.', - condition: P`lead_score != null && (lead_score < 0 || lead_score > 100)`, + condition: P`record.lead_score != null && (record.lead_score < 0 || record.lead_score > 100)`, message: 'Lead score must be between 0 and 100.', }, ], diff --git a/examples/app-showcase/src/objects/field-zoo.object.ts b/examples/app-showcase/src/objects/field-zoo.object.ts index ee6704d785..bd697b5e90 100644 --- a/examples/app-showcase/src/objects/field-zoo.object.ts +++ b/examples/app-showcase/src/objects/field-zoo.object.ts @@ -105,7 +105,7 @@ export const FieldZoo = ObjectSchema.create({ // ── Calculated / system ────────────────────────────────────────────── f_formula: Field.formula({ label: 'Formula (number × percent)', - expression: cel`(f_number == null ? 0 : f_number) * (f_percent == null ? 0 : f_percent) / 100`, + expression: cel`(record.f_number == null ? 0 : record.f_number) * (record.f_percent == null ? 0 : record.f_percent) / 100`, }), f_summary: Field.summary({ label: 'Roll-up Summary' }), f_autonumber: Field.autonumber({ label: 'Auto Number' }), diff --git a/examples/app-showcase/src/objects/project.object.ts b/examples/app-showcase/src/objects/project.object.ts index 3d81ba84d8..adae2c88fe 100644 --- a/examples/app-showcase/src/objects/project.object.ts +++ b/examples/app-showcase/src/objects/project.object.ts @@ -51,7 +51,7 @@ export const Project = ObjectSchema.create({ spent: Field.currency({ label: 'Spent', scale: 2, min: 0, defaultValue: 0 }), budget_remaining: Field.formula({ label: 'Budget Remaining', - expression: cel`(budget == null ? 0 : budget) - (spent == null ? 0 : spent)`, + expression: cel`(record.budget == null ? 0 : record.budget) - (record.spent == null ? 0 : record.spent)`, }), start_date: Field.date({ label: 'Start Date' }), end_date: Field.date({ label: 'Target End Date' }), diff --git a/packages/cli/src/utils/validate-expressions.test.ts b/packages/cli/src/utils/validate-expressions.test.ts index b36e65bac3..66433bf242 100644 --- a/packages/cli/src/utils/validate-expressions.test.ts +++ b/packages/cli/src/utils/validate-expressions.test.ts @@ -129,4 +129,67 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => { expect(issues).toHaveLength(1); expect(issues[0].message).toMatch(/invoke_function.*no .*function/i); }); + + // #1928 — bare field references are silently null in `record`-scoped sites + // (field formulas + validation predicates) but correct in flattened flow + // conditions. The validator wires the scope per site. + describe('bare-reference detection by site scope (#1928)', () => { + it('flags a bare reference in a field formula', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_opportunity', + fields: { + amount: { type: 'currency' }, + probability: { type: 'percent' }, + expected_revenue: { type: 'formula', name: 'expected_revenue', formula: 'amount * probability / 100' }, + }, + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].where).toContain("field 'expected_revenue' formula"); + expect(issues[0].message).toMatch(/bare reference `(amount|probability)`/); + }); + + it('flags a bare reference in a validation predicate', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_lead', + fields: { lead_score: { type: 'number' } }, + validations: [{ name: 'lead_score_range', expression: 'lead_score != null && lead_score > 100' }], + }], + }); + expect(issues).toHaveLength(1); + expect(issues[0].where).toContain("validation 'lead_score_range'"); + expect(issues[0].message).toMatch(/bare reference `lead_score`/); + }); + + it('accepts the record-qualified forms', () => { + const issues = validateStackExpressions({ + objects: [{ + name: 'crm_opportunity', + fields: { + amount: { type: 'currency' }, + probability: { type: 'percent' }, + expected_revenue: { type: 'formula', name: 'expected_revenue', formula: 'record.amount * record.probability / 100' }, + }, + validations: [{ name: 'amt', expression: 'record.amount != null && record.amount >= 0' }], + }], + }); + 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' } } }], + flows: [{ + name: 'high_value_deal', + nodes: [ + { id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'amount > 100000 && previous.amount <= 100000' } }, + ], + edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'stage != "closed_won"' }], + }], + }); + expect(issues).toHaveLength(0); + }); + }); }); diff --git a/packages/cli/src/utils/validate-expressions.ts b/packages/cli/src/utils/validate-expressions.ts index 820b574b16..2da5377015 100644 --- a/packages/cli/src/utils/validate-expressions.ts +++ b/packages/cli/src/utils/validate-expressions.ts @@ -57,11 +57,16 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const objects = asArray(stack.objects); const fieldIndex = buildFieldIndex(objects); - const check = (where: string, raw: unknown, objectName?: string): void => { + const check = ( + where: string, + raw: unknown, + objectName?: string, + scope: 'record' | 'flattened' = 'flattened', + ): void => { if (raw == null) return; const fields = objectName ? fieldIndex.get(objectName) : undefined; const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string }, - objectName ? { objectName, fields } : undefined); + objectName ? { objectName, fields, scope } : { scope }); for (const e of res.errors) issues.push({ where, message: e.message, source: e.source }); }; @@ -126,8 +131,9 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { const validations = obj.validations ?? obj.validationRules; for (const rule of asArray(validations)) { const where = `object '${objectName}' · validation '${(rule.name as string) ?? '?'}'`; - // Common predicate keys across rule shapes. - check(where, rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula, objectName); + // Common predicate keys across rule shapes. Validation predicates are + // `record`-scoped — no field flattening — so bare refs are flagged (#1928). + check(where, rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula, objectName, 'record'); } // Field-level formulas (computed fields) reference the same object. const fields = obj.fields; @@ -136,9 +142,10 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] { : (fields && typeof fields === 'object' ? Object.values(fields as AnyRec) as AnyRec[] : []); for (const f of fieldList) { if (f && typeof f === 'object' && f.formula) { - // formulas are `value` role (any return type), still CEL. + // 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) } : undefined); + objectName ? { objectName, fields: fieldIndex.get(objectName), scope: 'record' } : { scope: 'record' }); for (const e of res.errors) { issues.push({ where: `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`, message: e.message, source: e.source }); } diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 4e852aa5e8..1a31a9f5cc 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -41,6 +41,71 @@ function buildEnv(now: () => Date): Environment { return registerStdLib(env, now); } +/** + * Namespace roots that a `record`-scoped CEL site may legitimately reference. + * Declared as `map` (dyn values) so member access (`record.foo`) and any + * arithmetic/comparison on it defers to runtime — the strict env faults ONLY on + * an *undeclared* top-level identifier, i.e. a bare field reference. Generous on + * purpose: an unknown root is a missed catch, a missing root is a false positive + * that would break the build, so we err toward declaring more. + */ +const SCOPE_ROOTS = [ + 'record', 'previous', 'input', 'output', 'os', 'vars', 'variables', + 'automation', 'context', 'args', 'item', 'env', 'user', 'step', 'result', + 'trigger', 'event', 'payload', 'data', 'params', 'config', 'settings', +] as const; + +/** + * A `record`-scoped environment (`unlistedVariablesAreDyn: false`) for detecting + * bare field references. It reuses the real stdlib so function calls don't fault; + * only undeclared *variables* do. Built once — `parse`/`check` do not mutate it. + */ +let scopedEnv: Environment | undefined; +function getScopedEnv(): Environment { + if (scopedEnv) return scopedEnv; + const env = new Environment({ + unlistedVariablesAreDyn: false, + enableOptionalTypes: true, + limits: DEFAULT_LIMITS, + }); + registerStdLib(env, () => new Date(0)); + for (const root of SCOPE_ROOTS) { + try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ } + } + scopedEnv = env; + return env; +} + +/** + * In a `record`-scoped CEL site — a `Field.formula` or an object validation + * predicate — the evaluation scope binds only the `record`/`previous`/… *namespaces* + * (no field flattening). A bare top-level identifier like `amount` or `status` + * therefore resolves to nothing and the expression silently evaluates to `null` + * / never fires (#1928, the class behind #1927's broken formulas). Returns the + * first such bare reference, or `null`. + * + * Acts ONLY on cel-js's `Unknown variable: X` fault, so it cannot false-positive + * on arithmetic/comparison overloads — and it must NOT be applied to flow / + * automation conditions, where the record's fields ARE flattened to top-level + * and bare references are correct. + */ +export function detectBareReference(source: string): string | null { + if (typeof source !== 'string' || !source.trim()) return null; + try { + const result = getScopedEnv().parse(source).check?.() as + | { valid: boolean; error?: { message?: string } } + | undefined; + if (result && result.valid === false) { + const m = /Unknown variable:\s*([A-Za-z_$][\w$]*)/.exec(result.error?.message ?? ''); + if (m) return m[1]; + } + } catch { + // Parse/other faults are the syntax checker's job (celEngine.compile); this + // helper only reports the undeclared-variable case. + } + 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 1f8f1567b3..fb478f4ecf 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -80,6 +80,45 @@ describe('validateExpression (ADR-0032)', () => { }); }); + // #1928 — a bare top-level identifier is a silent bug in a `record`-scoped + // site (formula field / validation predicate) but correct in a `flattened` + // flow/automation condition. The validator must distinguish by `scope`. + describe('bare-reference detection by scope (#1928)', () => { + it('flags a bare field reference in a record-scoped predicate', () => { + const r = validateExpression('predicate', 'lead_score != null && lead_score > 100', { scope: 'record' }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toMatch(/bare reference `lead_score`/); + expect(r.errors[0].message).toMatch(/record\.lead_score/); + }); + + it('flags a bare reference in a record-scoped value (formula) expression', () => { + const r = validateExpression('value', '(budget == null ? 0 : budget) - (spent == null ? 0 : spent)', { scope: 'record' }); + expect(r.ok).toBe(false); + expect(r.errors[0].message).toMatch(/bare reference `(budget|spent)`/); + }); + + it('accepts the record-qualified form in a record-scoped site', () => { + const r = validateExpression('value', '(record.budget == null ? 0 : record.budget) - (record.spent == null ? 0 : record.spent)', { scope: 'record' }); + expect(r.ok).toBe(true); + }); + + it('does NOT flag bare references in a flattened (flow) condition', () => { + // The record's fields are flattened to top-level for flow conditions, and + // flow variables share that namespace, so bare refs are correct here. + expect(validateExpression('predicate', 'status == "done" && previous.status != "done"', { scope: 'flattened' }).ok).toBe(true); + expect(validateExpression('predicate', 'budget > 100000', { scope: 'flattened' }).ok).toBe(true); + expect(validateExpression('predicate', 'expiring_deals.length > 0', { scope: 'flattened' }).ok).toBe(true); + }); + + it('defaults to flattened scope (no bare-ref flag) when scope is unset', () => { + expect(validateExpression('predicate', 'status == "done"').ok).toBe(true); + }); + + it('does not flag a null-guard on a record-qualified field (no type false-positive)', () => { + expect(validateExpression('predicate', 'record.lead_score != null && record.lead_score > 100', { scope: 'record' }).ok).toBe(true); + }); + }); + 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 c37078462c..a274fc7cda 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 } from './cel-engine'; +import { celEngine, detectBareReference } from './cel-engine'; import { templateEngine } from './template-engine'; export type FieldRole = 'predicate' | 'value' | 'template'; @@ -36,6 +36,21 @@ export interface ExprSchemaHint { objectName?: string; /** Known top-level field names, so `record.` can be checked. */ fields?: readonly string[]; + /** + * Evaluation scope of the authoring site — determines whether a bare top-level + * identifier is legal (#1928): + * - `'record'` → the record is bound only as the `record` namespace, with + * no field flattening (`Field.formula`, object validation + * predicates). A bare `amount` resolves to nothing and the + * expression silently evaluates to `null` / never fires, so + * it MUST be written `record.amount`. We flag bare refs. + * - `'flattened'` → the record's own fields are spread to top-level alongside + * flow variables (flow / automation conditions), so bare + * `status` is correct. We do NOT flag bare refs here — flow + * variables are not schema-knowable, so a bare identifier + * can't be soundly told apart from a typo. (Default.) + */ + scope?: 'record' | 'flattened'; } export interface ExprValidationError { @@ -169,6 +184,21 @@ export function validateExpression( }); } else { checkFieldExistence(source, schema, errors); + // In a `record`-scoped site a bare top-level identifier is a silent bug — + // it must be `record.` (#1928). Only flagged here; flow/automation + // conditions (`scope: 'flattened'`, the default) legitimately use bare refs. + if (schema?.scope === 'record') { + const bare = detectBareReference(source); + if (bare) { + errors.push({ + source, + message: + `bare reference \`${bare}\` — a formula/validation expression binds the record as the ` + + `\`record\` namespace, not at top level, so \`${bare}\` resolves to nothing and the ` + + `expression silently evaluates to null. Write \`record.${bare}\`.`, + }); + } + } } return { ok: errors.length === 0, errors }; }