Skip to content

Commit 417b6ac

Browse files
os-zhuangclaude
andauthored
feat(validate): did-you-mean warnings for field typos in flow conditions (#1928) (#1933)
Flow/automation conditions flatten the record's fields to top-level, so a bare `status` is correct — but a bare NON-field identifier is either a flow variable or a typo. When it's a near-miss of a known field, the build now emits a non-blocking `did you mean X?` warning instead of staying silent, closing the silent-skip gap for misspelled trigger-condition fields (#1877 family) without the false-positive risk of a hard gate (a real flow variable won't be edit-distance-close to a field name, so it stays quiet). - `ExprValidationResult` gains `warnings`; `firstUndeclaredReference(source, knownFields)` generalizes the bare-ref detector (record scope = no fields, flattened scope = trigger object's fields declared so only non-field refs surface). - `ExprIssue` gains `severity`; `objectstack compile` prints warnings and fails only on errors. Verified: a typo'd flow condition warns + still builds; all example apps build clean with zero warnings (flow variables like `expiring_deals` stay quiet). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 61f730e commit 417b6ac

7 files changed

Lines changed: 183 additions & 27 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
"@objectstack/formula": patch
3+
"@objectstack/cli": patch
4+
---
5+
6+
feat(validate): advisory did-you-mean warnings for likely field typos in flow conditions
7+
8+
Adds a non-blocking warning channel to build-time expression validation (#1928
9+
tier 3). Flow / automation conditions flatten the record's fields to top-level,
10+
so a bare `status` is correct — but a bare NON-field identifier is either a flow
11+
variable or a typo. When it is a near-miss of a known field (edit distance), the
12+
build now emits a `did you mean \`status\`?` warning instead of staying silent,
13+
WITHOUT failing the build (a genuine flow variable won't be close to a field
14+
name, so it stays quiet). `ExprValidationResult` gains a `warnings` array and
15+
`ExprIssue` a `severity`; `objectstack compile` prints warnings and only fails on
16+
errors. This closes the silent-skip gap for misspelled trigger-condition fields
17+
(the #1877 family) without the false-positive risk of a hard gate.

packages/cli/src/commands/compile.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,19 +135,29 @@ export default class Compile extends Command {
135135
// located, corrective message instead of a silent runtime `false`.
136136
if (!flags.json) printStep('Validating expressions (ADR-0032)...');
137137
const exprIssues = validateStackExpressions(result.data as Record<string, unknown>);
138-
if (exprIssues.length > 0) {
138+
const exprErrors = exprIssues.filter((i) => i.severity !== 'warning');
139+
const exprWarnings = exprIssues.filter((i) => i.severity === 'warning');
140+
if (exprErrors.length > 0) {
139141
if (flags.json) {
140-
console.log(JSON.stringify({ success: false, error: 'expression validation failed', issues: exprIssues }));
142+
console.log(JSON.stringify({ success: false, error: 'expression validation failed', issues: exprErrors, warnings: exprWarnings }));
141143
this.exit(1);
142144
}
143145
console.log('');
144-
printError(`Expression validation failed (${exprIssues.length} issue${exprIssues.length > 1 ? 's' : ''})`);
145-
for (const i of exprIssues.slice(0, 50)) {
146+
printError(`Expression validation failed (${exprErrors.length} issue${exprErrors.length > 1 ? 's' : ''})`);
147+
for (const i of exprErrors.slice(0, 50)) {
146148
console.log(` • ${i.where}: ${i.message}`);
147149
console.log(` source: \`${i.source}\``);
148150
}
149151
this.exit(1);
150152
}
153+
// Advisory expression warnings (#1928 tier 3) — surfaced, never fatal.
154+
if (exprWarnings.length > 0 && !flags.json) {
155+
printWarning(`Expression warnings (${exprWarnings.length})`);
156+
for (const i of exprWarnings.slice(0, 50)) {
157+
console.log(` • ${i.where}: ${i.message}`);
158+
console.log(` source: \`${i.source}\``);
159+
}
160+
}
151161

152162
// 3c. Widget-binding diagnostics (issues #1719/#1721) — semantic checks
153163
// that need the widget's `dataset` reference resolved to its dataset

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,5 +191,46 @@ describe('validateStackExpressions (ADR-0032 build-time)', () => {
191191
});
192192
expect(issues).toHaveLength(0);
193193
});
194+
195+
// #1928 tier 3 — a likely field typo in a flow condition is a non-blocking warning.
196+
it('warns (severity=warning) on a likely field typo in a flow condition', () => {
197+
const issues = validateStackExpressions({
198+
objects: [{ name: 'crm_opportunity', fields: { stage: { type: 'select' }, amount: { type: 'currency' } } }],
199+
flows: [{
200+
name: 'opp_won',
201+
nodes: [
202+
{ id: 'start', type: 'start', config: { objectName: 'crm_opportunity', condition: 'stagee == "closed_won"' } },
203+
],
204+
edges: [],
205+
}],
206+
});
207+
expect(issues).toHaveLength(1);
208+
expect(issues[0].severity).toBe('warning');
209+
expect(issues[0].message).toMatch(/did you mean `stage`/);
210+
});
211+
212+
it('does not warn when the bare ref is far from any field (likely a flow variable)', () => {
213+
const issues = validateStackExpressions({
214+
objects: [{ name: 'crm_opportunity', fields: { stage: { type: 'select' } } }],
215+
flows: [{
216+
name: 'renewal',
217+
nodes: [{ id: 'start', type: 'start', config: { objectName: 'crm_opportunity' } }],
218+
edges: [{ id: 'e1', source: 'start', target: 'end', condition: 'expiring_deals.length > 0' }],
219+
}],
220+
});
221+
expect(issues).toHaveLength(0);
222+
});
223+
224+
it('tags record-scoped bare-ref issues as errors', () => {
225+
const issues = validateStackExpressions({
226+
objects: [{
227+
name: 'crm_lead',
228+
fields: { lead_score: { type: 'number' } },
229+
validations: [{ name: 'r', expression: 'lead_score > 100' }],
230+
}],
231+
});
232+
expect(issues).toHaveLength(1);
233+
expect(issues[0].severity).toBe('error');
234+
});
194235
});
195236
});

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ export interface ExprIssue {
2020
where: string;
2121
message: string;
2222
source: string;
23+
/**
24+
* `error` fails the build (e.g. a bare ref in a record-scoped formula). `warning`
25+
* is advisory and never fails it (e.g. a possible field typo in a flattened flow
26+
* condition, which might be a flow variable). Absent ⇒ treat as `error`.
27+
*/
28+
severity?: 'error' | 'warning';
2329
}
2430

2531
type AnyRec = Record<string, unknown>;
@@ -67,7 +73,8 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
6773
const fields = objectName ? fieldIndex.get(objectName) : undefined;
6874
const res = validateExpression('predicate', raw as string | { dialect?: string; source?: string },
6975
objectName ? { objectName, fields, scope } : { scope });
70-
for (const e of res.errors) issues.push({ where, message: e.message, source: e.source });
76+
for (const e of res.errors) issues.push({ where, message: e.message, source: e.source, severity: 'error' });
77+
for (const w of res.warnings) issues.push({ where, message: w.message, source: w.source, severity: 'warning' });
7178
};
7279

7380
// ── Flows ──────────────────────────────────────────────────────────
@@ -146,9 +153,9 @@ export function validateStackExpressions(stack: AnyRec): ExprIssue[] {
146153
// `record`-scoped — `record.<field>`, never bare — so flag bare refs (#1928).
147154
const res = validateExpression('value', f.formula as string | { dialect?: string; source?: string },
148155
objectName ? { objectName, fields: fieldIndex.get(objectName), scope: 'record' } : { scope: 'record' });
149-
for (const e of res.errors) {
150-
issues.push({ where: `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`, message: e.message, source: e.source });
151-
}
156+
const fieldWhere = `object '${objectName}' · field '${(f.name as string) ?? '?'}' formula`;
157+
for (const e of res.errors) issues.push({ where: fieldWhere, message: e.message, source: e.source, severity: 'error' });
158+
for (const w of res.warnings) issues.push({ where: fieldWhere, message: w.message, source: w.source, severity: 'warning' });
152159
}
153160
}
154161
}

packages/formula/src/cel-engine.ts

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,7 @@ const SCOPE_ROOTS = [
6060
* bare field references. It reuses the real stdlib so function calls don't fault;
6161
* only undeclared *variables* do. Built once — `parse`/`check` do not mutate it.
6262
*/
63-
let scopedEnv: Environment | undefined;
64-
function getScopedEnv(): Environment {
65-
if (scopedEnv) return scopedEnv;
63+
function buildScopedEnv(knownFields: readonly string[]): Environment {
6664
const env = new Environment({
6765
unlistedVariablesAreDyn: false,
6866
enableOptionalTypes: true,
@@ -72,10 +70,20 @@ function getScopedEnv(): Environment {
7270
for (const root of SCOPE_ROOTS) {
7371
try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ }
7472
}
75-
scopedEnv = env;
73+
// `knownFields` are declared as `dyn` so they (and member/arith/compare on
74+
// them) never fault — only a genuinely-undeclared top-level identifier does.
75+
// Empty for a record-scope site (any bare field is a bug); the trigger
76+
// object's fields for a flattened flow condition (only a NON-field bare ref —
77+
// a typo or flow variable — is then interesting).
78+
for (const field of knownFields) {
79+
try { env.registerVariable(field, 'dyn'); } catch { /* duplicate / reserved — ignore */ }
80+
}
7681
return env;
7782
}
7883

84+
// Roots-only env reused for the common record-scope check (no per-call rebuild).
85+
let recordScopeEnv: Environment | undefined;
86+
7987
/**
8088
* In a `record`-scoped CEL site — a `Field.formula` or an object validation
8189
* predicate — the evaluation scope binds only the `record`/`previous`/… *namespaces*
@@ -89,10 +97,16 @@ function getScopedEnv(): Environment {
8997
* automation conditions, where the record's fields ARE flattened to top-level
9098
* and bare references are correct.
9199
*/
92-
export function detectBareReference(source: string): string | null {
100+
export function firstUndeclaredReference(
101+
source: string,
102+
knownFields: readonly string[] = [],
103+
): string | null {
93104
if (typeof source !== 'string' || !source.trim()) return null;
94105
try {
95-
const result = getScopedEnv().parse(source).check?.() as
106+
const env = knownFields.length === 0
107+
? (recordScopeEnv ??= buildScopedEnv([]))
108+
: buildScopedEnv(knownFields);
109+
const result = env.parse(source).check?.() as
96110
| { valid: boolean; error?: { message?: string } }
97111
| undefined;
98112
if (result && result.valid === false) {
@@ -106,6 +120,11 @@ export function detectBareReference(source: string): string | null {
106120
return null;
107121
}
108122

123+
/** @deprecated use {@link firstUndeclaredReference} with no fields. */
124+
export function detectBareReference(source: string): string | null {
125+
return firstUndeclaredReference(source);
126+
}
127+
109128
/** Coerce cel-js's BigInt-flavored return into spec-friendly JS values. */
110129
function coerce(value: unknown): unknown {
111130
if (typeof value === 'bigint') {

packages/formula/src/validate.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,40 @@ describe('validateExpression (ADR-0032)', () => {
119119
});
120120
});
121121

122+
// #1928 tier 3 — flattened flow conditions reference fields bare, so a bare
123+
// ref is not an error. A bare NON-field that is a near-miss of a known field
124+
// is a likely typo → non-blocking warning (ok stays true).
125+
describe('flow-condition typo warnings (#1928 tier 3)', () => {
126+
const fields = ['stage', 'amount', 'status'] as const;
127+
128+
it('warns (does not error) on a likely field typo in a flattened condition', () => {
129+
const r = validateExpression('predicate', 'stagee == "closed_won"', { objectName: 'crm_opportunity', fields, scope: 'flattened' });
130+
expect(r.ok).toBe(true);
131+
expect(r.errors).toHaveLength(0);
132+
expect(r.warnings).toHaveLength(1);
133+
expect(r.warnings[0].message).toMatch(/`stagee` is not a field/);
134+
expect(r.warnings[0].message).toMatch(/did you mean `stage`/);
135+
});
136+
137+
it('does not warn on a correct bare field reference', () => {
138+
const r = validateExpression('predicate', 'stage == "closed_won" && previous.stage != "closed_won"', { objectName: 'crm_opportunity', fields, scope: 'flattened' });
139+
expect(r.ok).toBe(true);
140+
expect(r.warnings).toHaveLength(0);
141+
});
142+
143+
it('does not warn on a flow variable that is far from any field name', () => {
144+
const r = validateExpression('predicate', 'expiring_deals.length > 0', { objectName: 'crm_opportunity', fields, scope: 'flattened' });
145+
expect(r.ok).toBe(true);
146+
expect(r.warnings).toHaveLength(0);
147+
});
148+
149+
it('emits no warnings without a field list (nothing to compare against)', () => {
150+
const r = validateExpression('predicate', 'stagee == "x"', { scope: 'flattened' });
151+
expect(r.ok).toBe(true);
152+
expect(r.warnings).toHaveLength(0);
153+
});
154+
});
155+
122156
describe('introspection', () => {
123157
it('reports the dialect + scope for a field role', () => {
124158
expect(expectedDialect('predicate')).toBe('cel');

packages/formula/src/validate.ts

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

2323
export type FieldRole = 'predicate' | 'value' | 'template';
@@ -46,9 +46,11 @@ export interface ExprSchemaHint {
4646
* it MUST be written `record.amount`. We flag bare refs.
4747
* - `'flattened'` → the record's own fields are spread to top-level alongside
4848
* 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.)
49+
* `status` is correct and is NOT an error. Flow variables
50+
* are not schema-knowable, so a non-field bare identifier
51+
* can't be soundly told apart from a typo — but when one is
52+
* a near-miss of a known field we emit a non-blocking
53+
* did-you-mean *warning*. (Default.)
5254
*/
5355
scope?: 'record' | 'flattened';
5456
}
@@ -63,6 +65,13 @@ export interface ExprValidationError {
6365
export interface ExprValidationResult {
6466
ok: boolean;
6567
errors: ExprValidationError[];
68+
/**
69+
* Non-blocking advisories (#1928 tier 3): a likely-typo'd field reference in a
70+
* flattened flow condition. Never affects `ok` — callers surface these without
71+
* failing the build, since a bare identifier there may legitimately be a flow
72+
* variable.
73+
*/
74+
warnings: ExprValidationError[];
6675
}
6776

6877
/** A bare `{x}` that is NOT part of a `{{x}}` mustache hole. */
@@ -149,14 +158,15 @@ export function validateExpression(
149158
): ExprValidationResult {
150159
const { dialect, source } = toSource(input);
151160
const errors: ExprValidationError[] = [];
152-
if (!source.trim()) return { ok: true, errors };
161+
const warnings: ExprValidationError[] = [];
162+
if (!source.trim()) return { ok: true, errors, warnings };
153163

154164
if (role === 'template') {
155165
// Templates must be the `template` dialect (or untyped string). Reject a
156166
// CEL envelope mistakenly placed in a text field.
157167
if (dialect && dialect !== 'template') {
158168
errors.push({ source, message: `expected a text template but got a \`${dialect}\` expression.` });
159-
return { ok: false, errors };
169+
return { ok: false, errors, warnings };
160170
}
161171
const compiled = templateEngine.compile(source);
162172
if (!compiled.ok) {
@@ -165,13 +175,13 @@ export function validateExpression(
165175
// A single `{x}` in a template is the legacy/deprecated form (ADR-0032 §3).
166176
const hint = SINGLE_BRACE_RE.test(source) ? bracesHintForTemplate(source) : null;
167177
if (hint) errors.push({ source, message: hint });
168-
return { ok: errors.length === 0, errors };
178+
return { ok: errors.length === 0, errors, warnings };
169179
}
170180

171181
// predicate | value → CEL
172182
if (dialect && dialect !== 'cel') {
173183
errors.push({ source, message: `expected a CEL expression but got a \`${dialect}\` dialect.` });
174-
return { ok: false, errors };
184+
return { ok: false, errors, warnings };
175185
}
176186
const compiled = celEngine.compile(source);
177187
if (!compiled.ok) {
@@ -184,11 +194,10 @@ export function validateExpression(
184194
});
185195
} else {
186196
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.
190197
if (schema?.scope === 'record') {
191-
const bare = detectBareReference(source);
198+
// In a `record`-scoped site a bare top-level identifier is a silent bug —
199+
// it must be `record.<field>` (#1928). Hard error.
200+
const bare = firstUndeclaredReference(source);
192201
if (bare) {
193202
errors.push({
194203
source,
@@ -198,9 +207,28 @@ export function validateExpression(
198207
`expression silently evaluates to null. Write \`record.${bare}\`.`,
199208
});
200209
}
210+
} else if (schema?.fields && schema.fields.length > 0) {
211+
// Flattened flow/automation condition: the record's fields ARE bound at
212+
// top-level, so a bare ref is normally correct. But a *non-field* bare
213+
// identifier is either a flow variable or a typo. When it is a near-miss
214+
// of a known field, warn (did-you-mean) WITHOUT failing the build —
215+
// a genuine flow variable won't be edit-distance-close to a field. (#1928)
216+
const unknown = firstUndeclaredReference(source, schema.fields);
217+
if (unknown) {
218+
const suggestion = nearest(unknown, schema.fields);
219+
if (suggestion) {
220+
warnings.push({
221+
source,
222+
message:
223+
`\`${unknown}\` is not a field of \`${schema.objectName ?? 'the trigger object'}\` — ` +
224+
`did you mean \`${suggestion}\`? (flow conditions reference fields bare, e.g. \`${suggestion} == …\`). ` +
225+
`If \`${unknown}\` is a flow variable this is safe to ignore.`,
226+
});
227+
}
228+
}
201229
}
202230
}
203-
return { ok: errors.length === 0, errors };
231+
return { ok: errors.length === 0, errors, warnings };
204232
}
205233

206234
function bracesHintForTemplate(source: string): string {

0 commit comments

Comments
 (0)