Skip to content

Commit ff0a87a

Browse files
os-zhuangclaude
andauthored
feat(validate): flag bare field refs in record-scoped CEL sites (#1928) (#1931)
A Field.formula and an object validation predicate evaluate against the `record` namespace only (no field flattening), so a bare top-level identifier (`amount`, `status`) resolves to nothing and the expression silently evaluates to null / never fires — the silent-at-runtime class behind #1927. `validateExpression` gains an evaluation `scope`; for `scope: 'record'` it reports a bare reference with the corrective `record.<field>` form. It acts only on cel-js's `Unknown variable` fault, so it cannot false-positive on arithmetic/comparison/null-guard overloads. Flow & 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 NOT flagged. `objectstack compile` wires record scope for field formulas + validation predicates; flow conditions stay flattened. Also fixes three latent bare-ref bugs surfaced by the new check: - crm_lead validation `lead_score_range` (rule silently never fired) - showcase field_zoo `f_formula`, showcase project `budget_remaining` (null) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 82c7438 commit ff0a87a

9 files changed

Lines changed: 235 additions & 10 deletions

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
"@objectstack/formula": patch
3+
"@objectstack/cli": patch
4+
---
5+
6+
feat(validate): flag bare field references in record-scoped CEL sites at build time
7+
8+
A `Field.formula` and an object validation predicate evaluate against the
9+
`record` namespace only — there is no field flattening — so a bare top-level
10+
identifier (`amount`, `status`) resolves to nothing and the expression silently
11+
evaluates to `null` / never fires. This is the silent-at-runtime class behind
12+
the broken example-crm formulas (#1927) and is exactly what AI authors get wrong.
13+
14+
`validateExpression` now takes an evaluation `scope` and, for `scope: 'record'`,
15+
reports a bare reference with the corrective form (`write record.<field>`). The
16+
check is schema-free and acts only on cel-js's `Unknown variable` fault, so it
17+
cannot false-positive on arithmetic/comparison/null-guard type overloads. Flow
18+
and automation conditions keep the default `scope: 'flattened'` — the record's
19+
fields ARE spread to top-level there (alongside flow variables), so bare refs
20+
are correct and are NOT flagged. `objectstack compile` wires `record` scope for
21+
field formulas and validation predicates; flow conditions stay flattened.

examples/app-crm/src/objects/lead.object.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ export const Lead = ObjectSchema.create({
104104
name: 'lead_score_range',
105105
label: 'Lead Score 0–100',
106106
description: 'Lead score must be between 0 and 100.',
107-
condition: P`lead_score != null && (lead_score < 0 || lead_score > 100)`,
107+
condition: P`record.lead_score != null && (record.lead_score < 0 || record.lead_score > 100)`,
108108
message: 'Lead score must be between 0 and 100.',
109109
},
110110
],

examples/app-showcase/src/objects/field-zoo.object.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ export const FieldZoo = ObjectSchema.create({
105105
// ── Calculated / system ──────────────────────────────────────────────
106106
f_formula: Field.formula({
107107
label: 'Formula (number × percent)',
108-
expression: cel`(f_number == null ? 0 : f_number) * (f_percent == null ? 0 : f_percent) / 100`,
108+
expression: cel`(record.f_number == null ? 0 : record.f_number) * (record.f_percent == null ? 0 : record.f_percent) / 100`,
109109
}),
110110
f_summary: Field.summary({ label: 'Roll-up Summary' }),
111111
f_autonumber: Field.autonumber({ label: 'Auto Number' }),

examples/app-showcase/src/objects/project.object.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export const Project = ObjectSchema.create({
5151
spent: Field.currency({ label: 'Spent', scale: 2, min: 0, defaultValue: 0 }),
5252
budget_remaining: Field.formula({
5353
label: 'Budget Remaining',
54-
expression: cel`(budget == null ? 0 : budget) - (spent == null ? 0 : spent)`,
54+
expression: cel`(record.budget == null ? 0 : record.budget) - (record.spent == null ? 0 : record.spent)`,
5555
}),
5656
start_date: Field.date({ label: 'Start Date' }),
5757
end_date: Field.date({ label: 'Target End Date' }),

packages/cli/src/utils/validate-expressions.test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,4 +129,67 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
129129
expect(issues).toHaveLength(1);
130130
expect(issues[0].message).toMatch(/invoke_function.*no .*function/i);
131131
});
132+
133+
// #1928 — bare field references are silently null in `record`-scoped sites
134+
// (field formulas + validation predicates) but correct in flattened flow
135+
// conditions. The validator wires the scope per site.
136+
describe('bare-reference detection by site scope (#1928)', () => {
137+
it('flags a bare reference in a field formula', () => {
138+
const issues = validateStackExpressions({
139+
objects: [{
140+
name: 'crm_opportunity',
141+
fields: {
142+
amount: { type: 'currency' },
143+
probability: { type: 'percent' },
144+
expected_revenue: { type: 'formula', name: 'expected_revenue', formula: 'amount * probability / 100' },
145+
},
146+
}],
147+
});
148+
expect(issues).toHaveLength(1);
149+
expect(issues[0].where).toContain("field 'expected_revenue' formula");
150+
expect(issues[0].message).toMatch(/bare reference `(amount|probability)`/);
151+
});
152+
153+
it('flags a bare reference in a validation predicate', () => {
154+
const issues = validateStackExpressions({
155+
objects: [{
156+
name: 'crm_lead',
157+
fields: { lead_score: { type: 'number' } },
158+
validations: [{ name: 'lead_score_range', expression: 'lead_score != null && lead_score > 100' }],
159+
}],
160+
});
161+
expect(issues).toHaveLength(1);
162+
expect(issues[0].where).toContain("validation 'lead_score_range'");
163+
expect(issues[0].message).toMatch(/bare reference `lead_score`/);
164+
});
165+
166+
it('accepts the record-qualified forms', () => {
167+
const issues = validateStackExpressions({
168+
objects: [{
169+
name: 'crm_opportunity',
170+
fields: {
171+
amount: { type: 'currency' },
172+
probability: { type: 'percent' },
173+
expected_revenue: { type: 'formula', name: 'expected_revenue', formula: 'record.amount * record.probability / 100' },
174+
},
175+
validations: [{ name: 'amt', expression: 'record.amount != null && record.amount >= 0' }],
176+
}],
177+
});
178+
expect(issues).toHaveLength(0);
179+
});
180+
181+
it('does NOT flag bare references in a flow condition (flattened scope)', () => {
182+
const issues = validateStackExpressions({
183+
objects: [{ name: 'crm_opportunity', fields: { stage: { type: 'select' }, amount: { type: 'currency' } } }],
184+
flows: [{
185+
name: 'high_value_deal',
186+
nodes: [
187+
{ id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'amount > 100000 && previous.amount <= 100000' } },
188+
],
189+
edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'stage != "closed_won"' }],
190+
}],
191+
});
192+
expect(issues).toHaveLength(0);
193+
});
194+
});
132195
});

packages/cli/src/utils/validate-expressions.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,16 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
5757
const objects = asArray(stack.objects);
5858
const fieldIndex = buildFieldIndex(objects);
5959

60-
const check = (where: string, raw: unknown, objectName?: string): void => {
60+
const check = (
61+
where: string,
62+
raw: unknown,
63+
objectName?: string,
64+
scope: 'record' | 'flattened' = 'flattened',
65+
): void => {
6166
if (raw == null) return;
6267
const fields = objectName ? fieldIndex.get(objectName) : undefined;
6368
const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string },
64-
objectName ? { objectName, fields } : undefined);
69+
objectName ? { objectName, fields, scope } : { scope });
6570
for (const e of res.errors) issues.push({ where, message: e.message, source: e.source });
6671
};
6772

@@ -126,8 +131,9 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
126131
const validations = obj.validations ?? obj.validationRules;
127132
for (const rule of asArray(validations)) {
128133
const where = `object '${objectName}' · validation '${(rule.name as string) ?? '?'}'`;
129-
// Common predicate keys across rule shapes.
130-
check(where, rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula, objectName);
134+
// Common predicate keys across rule shapes. Validation predicates are
135+
// `record`-scoped — no field flattening — so bare refs are flagged (#1928).
136+
check(where, rule.expression ?? rule.predicate ?? rule.condition ?? rule.formula, objectName, 'record');
131137
}
132138
// Field-level formulas (computed fields) reference the same object.
133139
const fields = obj.fields;
@@ -136,9 +142,10 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
136142
: (fields && typeof fields === 'object' ? Object.values(fields as AnyRec) as AnyRec[] : []);
137143
for (const f of fieldList) {
138144
if (f && typeof f === 'object' && f.formula) {
139-
// formulas are `value` role (any return type), still CEL.
145+
// formulas are `value` role (any return type), still CEL. They are
146+
// `record`-scoped — `record.<field>`, never bare — so flag bare refs (#1928).
140147
const res = validateExpression('value', f.formula as string | { dialect?: string; source?: string },
141-
objectName ? { objectName, fields: fieldIndex.get(objectName) } : undefined);
148+
objectName ? { objectName, fields: fieldIndex.get(objectName), scope: 'record' } : { scope: 'record' });
142149
for (const e of res.errors) {
143150
issues.push({ where: `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`, message: e.message, source: e.source });
144151
}

packages/formula/src/cel-engine.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,71 @@ function buildEnv(now: () => Date): Environment {
4141
return registerNumericCoercions(registerStdLib(env, now));
4242
}
4343

44+
/**
45+
* Namespace roots that a `record`-scoped CEL site may legitimately reference.
46+
* Declared as `map` (dyn values) so member access (`record.foo`) and any
47+
* arithmetic/comparison on it defers to runtime — the strict env faults ONLY on
48+
* an *undeclared* top-level identifier, i.e. a bare field reference. Generous on
49+
* purpose: an unknown root is a missed catch, a missing root is a false positive
50+
* that would break the build, so we err toward declaring more.
51+
*/
52+
const SCOPE_ROOTS = [
53+
'record', 'previous', 'input', 'output', 'os', 'vars', 'variables',
54+
'automation', 'context', 'args', 'item', 'env', 'user', 'step', 'result',
55+
'trigger', 'event', 'payload', 'data', 'params', 'config', 'settings',
56+
] as const;
57+
58+
/**
59+
* A `record`-scoped environment (`unlistedVariablesAreDyn: false`) for detecting
60+
* bare field references. It reuses the real stdlib so function calls don't fault;
61+
* only undeclared *variables* do. Built once — `parse`/`check` do not mutate it.
62+
*/
63+
let scopedEnv: Environment | undefined;
64+
function getScopedEnv(): Environment {
65+
if (scopedEnv) return scopedEnv;
66+
const env = new Environment({
67+
unlistedVariablesAreDyn: false,
68+
enableOptionalTypes: true,
69+
limits: DEFAULT_LIMITS,
70+
});
71+
registerStdLib(env, () => new Date(0));
72+
for (const root of SCOPE_ROOTS) {
73+
try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ }
74+
}
75+
scopedEnv = env;
76+
return env;
77+
}
78+
79+
/**
80+
* In a `record`-scoped CEL site — a `Field.formula` or an object validation
81+
* predicate — the evaluation scope binds only the `record`/`previous`/… *namespaces*
82+
* (no field flattening). A bare top-level identifier like `amount` or `status`
83+
* therefore resolves to nothing and the expression silently evaluates to `null`
84+
* / never fires (#1928, the class behind #1927's broken formulas). Returns the
85+
* first such bare reference, or `null`.
86+
*
87+
* Acts ONLY on cel-js's `Unknown variable: X` fault, so it cannot false-positive
88+
* on arithmetic/comparison overloads — and it must NOT be applied to flow /
89+
* automation conditions, where the record's fields ARE flattened to top-level
90+
* and bare references are correct.
91+
*/
92+
export function detectBareReference(source: string): string | null {
93+
if (typeof source !== 'string' || !source.trim()) return null;
94+
try {
95+
const result = getScopedEnv().parse(source).check?.() as
96+
| { valid: boolean; error?: { message?: string } }
97+
| undefined;
98+
if (result && result.valid === false) {
99+
const m = /Unknown variable:\s*([A-Za-z_$][\w$]*)/.exec(result.error?.message ?? '');
100+
if (m) return m[1];
101+
}
102+
} catch {
103+
// Parse/other faults are the syntax checker's job (celEngine.compile); this
104+
// helper only reports the undeclared-variable case.
105+
}
106+
return null;
107+
}
108+
44109
/** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */
45110
function coerce(value: unknown): unknown {
46111
if (typeof value === 'bigint') {

packages/formula/src/validate.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,45 @@ describe('validateExpression (ADR-0032)', () => {
8080
});
8181
});
8282

83+
// #1928 — a bare top-level identifier is a silent bug in a `record`-scoped
84+
// site (formula field / validation predicate) but correct in a `flattened`
85+
// flow/automation condition. The validator must distinguish by `scope`.
86+
describe('bare-reference detection by scope (#1928)', () => {
87+
it('flags a bare field reference in a record-scoped predicate', () => {
88+
const r = validateExpression('predicate', 'lead_score != null && lead_score > 100', { scope: 'record' });
89+
expect(r.ok).toBe(false);
90+
expect(r.errors[0].message).toMatch(/bare reference `lead_score`/);
91+
expect(r.errors[0].message).toMatch(/record\.lead_score/);
92+
});
93+
94+
it('flags a bare reference in a record-scoped value (formula) expression', () => {
95+
const r = validateExpression('value', '(budget == null ? 0 : budget) - (spent == null ? 0 : spent)', { scope: 'record' });
96+
expect(r.ok).toBe(false);
97+
expect(r.errors[0].message).toMatch(/bare reference `(budget|spent)`/);
98+
});
99+
100+
it('accepts the record-qualified form in a record-scoped site', () => {
101+
const r = validateExpression('value', '(record.budget == null ? 0 : record.budget) - (record.spent == null ? 0 : record.spent)', { scope: 'record' });
102+
expect(r.ok).toBe(true);
103+
});
104+
105+
it('does NOT flag bare references in a flattened (flow) condition', () => {
106+
// The record's fields are flattened to top-level for flow conditions, and
107+
// flow variables share that namespace, so bare refs are correct here.
108+
expect(validateExpression('predicate', 'status == "done" && previous.status != "done"', { scope: 'flattened' }).ok).toBe(true);
109+
expect(validateExpression('predicate', 'budget > 100000', { scope: 'flattened' }).ok).toBe(true);
110+
expect(validateExpression('predicate', 'expiring_deals.length > 0', { scope: 'flattened' }).ok).toBe(true);
111+
});
112+
113+
it('defaults to flattened scope (no bare-ref flag) when scope is unset', () => {
114+
expect(validateExpression('predicate', 'status == "done"').ok).toBe(true);
115+
});
116+
117+
it('does not flag a null-guard on a record-qualified field (no type false-positive)', () => {
118+
expect(validateExpression('predicate', 'record.lead_score != null && record.lead_score > 100', { scope: 'record' }).ok).toBe(true);
119+
});
120+
});
121+
83122
describe('introspection', () => {
84123
it('reports the dialect + scope for a field role', () => {
85124
expect(expectedDialect('predicate')).toBe('cel');

packages/formula/src/validate.ts

Lines changed: 31 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 } from './cel-engine';
20+
import { celEngine, detectBareReference } from './cel-engine';
2121
import { templateEngine } from './template-engine';
2222

2323
export type FieldRole = 'predicate' | 'value' | 'template';
@@ -36,6 +36,21 @@ export interface ExprSchemaHint {
3636
objectName?: string;
3737
/** Known top-level field names, so `record.<field>` can be checked. */
3838
fields?: readonly string[];
39+
/**
40+
* Evaluation scope of the authoring site — determines whether a bare top-level
41+
* identifier is legal (#1928):
42+
* - `'record'` → the record is bound only as the `record` namespace, with
43+
* no field flattening (`Field.formula`, object validation
44+
* predicates). A bare `amount` resolves to nothing and the
45+
* expression silently evaluates to `null` / never fires, so
46+
* it MUST be written `record.amount`. We flag bare refs.
47+
* - `'flattened'` → the record's own fields are spread to top-level alongside
48+
* flow variables (flow / automation conditions), so bare
49+
* `status` is correct. We do NOT flag bare refs here — flow
50+
* variables are not schema-knowable, so a bare identifier
51+
* can't be soundly told apart from a typo. (Default.)
52+
*/
53+
scope?: 'record' | 'flattened';
3954
}
4055

4156
export interface ExprValidationError {
@@ -169,6 +184,21 @@ export function validateExpression(
169184
});
170185
} else {
171186
checkFieldExistence(source, schema, errors);
187+
// In a `record`-scoped site a bare top-level identifier is a silent bug —
188+
// it must be `record.<field>` (#1928). Only flagged here; flow/automation
189+
// conditions (`scope: 'flattened'`, the default) legitimately use bare refs.
190+
if (schema?.scope === 'record') {
191+
const bare = detectBareReference(source);
192+
if (bare) {
193+
errors.push({
194+
source,
195+
message:
196+
`bare reference \`${bare}\` — a formula/validation expression binds the record as the ` +
197+
`\`record\` namespace, not at top level, so \`${bare}\` resolves to nothing and the ` +
198+
`expression silently evaluates to null. Write \`record.${bare}\`.`,
199+
});
200+
}
201+
}
172202
}
173203
return { ok: errors.length === 0, errors };
174204
}

0 commit comments

Comments
 (0)